Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rm AllowUpdateAfter... check #1118

Merged
merged 7 commits into from
Apr 25, 2022
Merged

fix: rm AllowUpdateAfter... check #1118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
32e5011
update code & test
charleenfei Mar 14, 2022
a5c5580
Merge branch '02-client-refactor' into feat/rm_AllowUpdateAfter
charleenfei Apr 6, 2022
a6b6d9f
update proto and adr026
charleenfei Apr 8, 2022
c34dc07
update CHANGELOG
charleenfei Apr 8, 2022
a064833
update cli docs
charleenfei Apr 8, 2022
9cf6832
update broken milestone link
charleenfei Apr 8, 2022
c61cfd2
update changelog
charleenfei Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 6 additions & 21 deletions modules/light-clients/07-tendermint/types/proposal_handle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,17 @@ import (
)

// CheckSubstituteAndUpdateState will try to update the client with the state of the
// substitute if and only if the proposal passes and one of the following conditions are
// satisfied:
// 1) AllowUpdateAfterMisbehaviour and Status() == Frozen
// 2) AllowUpdateAfterExpiry=true and Status() == Expired
// substitute.
//
// AllowUpdateAfterMisbehaviour and AllowUpdateAfterExpiry have been deprecated, as a code migration
// can overwrite the client and consensus states regardless of the value of these parameters
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
//
// The following must always be true:
// - The substitute client is the same type as the subject client
// - The subject and substitute client states match in all parameters (expect frozen height, latest height, and chain-id)
//
// In case 1) before updating the client, the client will be unfrozen by resetting
// the FrozenHeight to the zero Height. If a client is frozen and AllowUpdateAfterMisbehaviour
// is set to true, the client will be unexpired even if AllowUpdateAfterExpiry is set to false.
// the FrozenHeight to the zero Height.
func (cs ClientState) CheckSubstituteAndUpdateState(
ctx sdk.Context, cdc codec.BinaryCodec, subjectClientStore,
substituteClientStore sdk.KVStore, substituteClient exported.ClientState,
Expand All @@ -39,23 +38,9 @@ func (cs ClientState) CheckSubstituteAndUpdateState(
return nil, sdkerrors.Wrap(clienttypes.ErrInvalidSubstitute, "subject client state does not match substitute client state")
}

switch cs.Status(ctx, subjectClientStore, cdc) {

case exported.Frozen:
if !cs.AllowUpdateAfterMisbehaviour {
return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unfrozen")
}

if cs.Status(ctx, subjectClientStore, cdc) == exported.Frozen {
// unfreeze the client
cs.FrozenHeight = clienttypes.ZeroHeight()

case exported.Expired:
if !cs.AllowUpdateAfterExpiry {
return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unexpired")
}

default:
return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client cannot be updated with proposal")
}

// copy consensus states and processed time from substitute to subject
Expand Down
102 changes: 7 additions & 95 deletions modules/light-clients/07-tendermint/types/proposal_handle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,133 +81,45 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() {
expPass bool
}{
{
name: "not allowed to be updated, not frozen or expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: false,
ExpireClient: false,
expPass: false,
},
{
name: "not allowed to be updated, client is frozen",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: true,
ExpireClient: false,
expPass: false,
},
{
name: "not allowed to be updated, client is expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: false,
ExpireClient: true,
expPass: false,
},
{
name: "not allowed to be updated, client is frozen and expired",
name: "PASS: update checks are deprecated, client is frozen and expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: true,
ExpireClient: true,
expPass: false,
},
{
name: "allowed to be updated only after misbehaviour, not frozen or expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: false,
ExpireClient: false,
expPass: false,
},
{
name: "allowed to be updated only after misbehaviour, client is expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: false,
ExpireClient: true,
expPass: false,
},
{
name: "allowed to be updated only after expiry, not frozen or expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: false,
ExpireClient: false,
expPass: false,
},
{
name: "allowed to be updated only after expiry, client is frozen",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: true,
ExpireClient: false,
expPass: false,
},
{
name: "PASS: allowed to be updated only after misbehaviour, client is frozen",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: true,
ExpireClient: false,
expPass: true,
},
{
name: "PASS: allowed to be updated only after misbehaviour, client is frozen and expired",
name: "PASS: update checks are deprecated, not frozen or expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: true,
ExpireClient: true,
expPass: true,
},
{
name: "PASS: allowed to be updated only after expiry, client is expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: false,
ExpireClient: true,
ExpireClient: false,
expPass: true,
},
{
name: "allowed to be updated only after expiry, client is frozen and expired",
name: "PASS: update checks are deprecated, not frozen or expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: true,
ExpireClient: true,
expPass: false,
},
{
name: "allowed to be updated after expiry and misbehaviour, not frozen or expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: false,
ExpireClient: false,
expPass: false,
expPass: true,
},
{
name: "PASS: allowed to be updated after expiry and misbehaviour, client is frozen",
name: "PASS: update checks are deprecated, client is frozen",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: true,
ExpireClient: false,
expPass: true,
},
{
name: "PASS: allowed to be updated after expiry and misbehaviour, client is expired",
name: "PASS: update checks are deprecated, client is expired",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: I prefer prepending the case name with success: ... rather than PASS: ... personally (just keeping things consistent) :-)

AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: false,
ExpireClient: true,
expPass: true,
},
{
name: "PASS: allowed to be updated after expiry and misbehaviour, client is frozen and expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: true,
ExpireClient: true,
expPass: true,
},
}

for _, tc := range testCases {
Expand Down