diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c42454c3ed..7759873a130 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking + +* (02-client) [\#595](https://github.com/cosmos/ibc-go/pull/595) The client state return value has been removed from `CheckMisbehaviourAndUpdateState`. Light client implementations must update the client state in the client store before returning for valid misbehaviour. * (channel( [\#848](https://github.com/cosmos/ibc-go/pull/848) Added `ChannelId` to MsgChannelOpenInitResponse * (testing( [\#813](https://github.com/cosmos/ibc-go/pull/813) The `ack` argument to the testing function `RelayPacket` has been removed as it is no longer needed. * (testing) [\#774](https://github.com/cosmos/ibc-go/pull/774) Added `ChainID` arg to `SetupWithGenesisValSet` on the testing app. `Coordinator` generated ChainIDs now starts at index 1 diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index 90e9af256d2..3b8f51a9b07 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -23,4 +23,8 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`. +## IBC Light Clients +The `CheckMisbehaviourAndUpdateState` function has been modified. The client state return value has been removed. + +Light clients **must** set the updated client state in the client store before returning for valid misbehaviour. diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index d8085ac719f..388d5aa167b 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -204,12 +204,11 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex return err } - clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, misbehaviour) + err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, misbehaviour) if err != nil { return err } - k.SetClientState(ctx, misbehaviour.GetClientID(), clientState) k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", misbehaviour.GetClientID()) defer func() { diff --git a/modules/core/02-client/legacy/v100/solomachine.go b/modules/core/02-client/legacy/v100/solomachine.go index b9ae2b1005e..08fed1cefba 100644 --- a/modules/core/02-client/legacy/v100/solomachine.go +++ b/modules/core/02-client/legacy/v100/solomachine.go @@ -98,7 +98,7 @@ func (cs *ClientState) CheckHeaderAndUpdateState( // CheckMisbehaviourAndUpdateState panics! func (cs ClientState) CheckMisbehaviourAndUpdateState( _ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.Misbehaviour, -) (exported.ClientState, error) { +) error { panic("legacy solo machine is deprecated!") } diff --git a/modules/core/exported/client.go b/modules/core/exported/client.go index 4dce203bea4..7c1f35017a3 100644 --- a/modules/core/exported/client.go +++ b/modules/core/exported/client.go @@ -59,7 +59,11 @@ type ClientState interface { // Update and Misbehaviour functions CheckHeaderAndUpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, Header) (ClientState, ConsensusState, error) - CheckMisbehaviourAndUpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, Misbehaviour) (ClientState, error) + + // CheckMisbehaviourAndUpdateState must: + // - verify the provided misbehaviour + // - if the misbehaviour is valid, set the client status to frozen + CheckMisbehaviourAndUpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, Misbehaviour) error CheckSubstituteAndUpdateState(ctx sdk.Context, cdc codec.BinaryCodec, subjectClientStore, substituteClientStore sdk.KVStore, substituteClient ClientState) (ClientState, error) // Upgrade functions diff --git a/modules/light-clients/06-solomachine/types/misbehaviour_handle.go b/modules/light-clients/06-solomachine/types/misbehaviour_handle.go index d5a1d57cb57..a347a8e59c0 100644 --- a/modules/light-clients/06-solomachine/types/misbehaviour_handle.go +++ b/modules/light-clients/06-solomachine/types/misbehaviour_handle.go @@ -20,11 +20,11 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( cdc codec.BinaryCodec, clientStore sdk.KVStore, misbehaviour exported.Misbehaviour, -) (exported.ClientState, error) { +) error { soloMisbehaviour, ok := misbehaviour.(*Misbehaviour) if !ok { - return nil, sdkerrors.Wrapf( + return sdkerrors.Wrapf( clienttypes.ErrInvalidClientType, "misbehaviour type %T, expected %T", misbehaviour, &Misbehaviour{}, ) @@ -35,16 +35,18 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( // verify first signature if err := verifySignatureAndData(cdc, cs, soloMisbehaviour, soloMisbehaviour.SignatureOne); err != nil { - return nil, sdkerrors.Wrap(err, "failed to verify signature one") + return sdkerrors.Wrap(err, "failed to verify signature one") } // verify second signature if err := verifySignatureAndData(cdc, cs, soloMisbehaviour, soloMisbehaviour.SignatureTwo); err != nil { - return nil, sdkerrors.Wrap(err, "failed to verify signature two") + return sdkerrors.Wrap(err, "failed to verify signature two") } cs.IsFrozen = true - return &cs, nil + setClientState(clientStore, cdc, &cs) + + return nil } // verifySignatureAndData verifies that the currently registered public key has signed diff --git a/modules/light-clients/06-solomachine/types/misbehaviour_handle_test.go b/modules/light-clients/06-solomachine/types/misbehaviour_handle_test.go index db58b710772..52869b09106 100644 --- a/modules/light-clients/06-solomachine/types/misbehaviour_handle_test.go +++ b/modules/light-clients/06-solomachine/types/misbehaviour_handle_test.go @@ -247,17 +247,25 @@ func (suite *SoloMachineTestSuite) TestCheckMisbehaviourAndUpdateState() { tc := tc suite.Run(tc.name, func() { + // reset suite to create fresh application state + suite.SetupTest() + // setup test tc.setup() - clientState, err := clientState.CheckMisbehaviourAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.store, misbehaviour) + err := clientState.CheckMisbehaviourAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.store, misbehaviour) + + clientState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), exported.Solomachine) if tc.expPass { suite.Require().NoError(err) + suite.Require().True(found) suite.Require().True(clientState.(*types.ClientState).IsFrozen, "client not frozen") } else { suite.Require().Error(err) - suite.Require().Nil(clientState) + if found { + suite.Require().False(clientState.(*types.ClientState).IsFrozen, "client is frozen") + } } }) } diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go index 4c8224bde09..475c1bdd9a6 100644 --- a/modules/light-clients/07-tendermint/types/misbehaviour_handle.go +++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle.go @@ -26,10 +26,10 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( cdc codec.BinaryCodec, clientStore sdk.KVStore, misbehaviour exported.Misbehaviour, -) (exported.ClientState, error) { +) error { tmMisbehaviour, ok := misbehaviour.(*Misbehaviour) if !ok { - return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", misbehaviour, &Misbehaviour{}) + return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", misbehaviour, &Misbehaviour{}) } // The status of the client is checked in 02-client @@ -39,22 +39,22 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) { blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID) if err != nil { - return nil, sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour") + return sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour") } blockID2, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header2.SignedHeader.Commit.BlockID) if err != nil { - return nil, sdkerrors.Wrap(err, "invalid block ID from header 2 in misbehaviour") + return sdkerrors.Wrap(err, "invalid block ID from header 2 in misbehaviour") } // Ensure that Commit Hashes are different if bytes.Equal(blockID1.Hash, blockID2.Hash) { - return nil, sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers block hashes are equal") + return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers block hashes are equal") } } else { // Header1 is at greater height than Header2, therefore Header1 time must be less than or equal to // Header2 time in order to be valid misbehaviour (violation of monotonic time). if tmMisbehaviour.Header1.SignedHeader.Header.Time.After(tmMisbehaviour.Header2.SignedHeader.Header.Time) { - return nil, sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers are not at same height and are monotonically increasing") + return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers are not at same height and are monotonically increasing") } } @@ -66,13 +66,13 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( // Get consensus bytes from clientStore tmConsensusState1, err := GetConsensusState(clientStore, cdc, tmMisbehaviour.Header1.TrustedHeight) if err != nil { - return nil, sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", tmMisbehaviour.Header1) + return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", tmMisbehaviour.Header1) } // Get consensus bytes from clientStore tmConsensusState2, err := GetConsensusState(clientStore, cdc, tmMisbehaviour.Header2.TrustedHeight) if err != nil { - return nil, sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header2 at TrustedHeight: %s", tmMisbehaviour.Header2) + return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header2 at TrustedHeight: %s", tmMisbehaviour.Header2) } // Check the validity of the two conflicting headers against their respective @@ -83,17 +83,18 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( if err := checkMisbehaviourHeader( &cs, tmConsensusState1, tmMisbehaviour.Header1, ctx.BlockTime(), ); err != nil { - return nil, sdkerrors.Wrap(err, "verifying Header1 in Misbehaviour failed") + return sdkerrors.Wrap(err, "verifying Header1 in Misbehaviour failed") } if err := checkMisbehaviourHeader( &cs, tmConsensusState2, tmMisbehaviour.Header2, ctx.BlockTime(), ); err != nil { - return nil, sdkerrors.Wrap(err, "verifying Header2 in Misbehaviour failed") + return sdkerrors.Wrap(err, "verifying Header2 in Misbehaviour failed") } cs.FrozenHeight = FrozenHeight + setClientState(clientStore, cdc, &cs) - return &cs, nil + return nil } // checkMisbehaviourHeader checks that a Header in Misbehaviour is valid misbehaviour given diff --git a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go index da1efc665da..08a6d974a29 100644 --- a/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go +++ b/modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go @@ -410,20 +410,22 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviourAndUpdateState() { suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(ctx, clientID, tc.height2, tc.consensusState2) } - clientState, err := tc.clientState.CheckMisbehaviourAndUpdateState( + err := tc.clientState.CheckMisbehaviourAndUpdateState( ctx, suite.chainA.App.AppCodec(), suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, clientID), // pass in clientID prefixed clientStore tc.misbehaviour, ) + clientState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(ctx, clientID) + if tc.expPass { suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name) - suite.Require().NotNil(clientState, "valid test case %d failed: %s", i, tc.name) + suite.Require().True(found, "valid test case %d failed: %s", i, tc.name) suite.Require().True(!clientState.(*types.ClientState).FrozenHeight.IsZero(), "valid test case %d failed: %s", i, tc.name) } else { suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name) - suite.Require().Nil(clientState, "invalid test case %d passed: %s", i, tc.name) + suite.Require().False(found, "invalid test case %d passed: %s", i, tc.name) } }) } diff --git a/modules/light-clients/07-tendermint/types/store.go b/modules/light-clients/07-tendermint/types/store.go index 785ed77ba97..33c2386bb78 100644 --- a/modules/light-clients/07-tendermint/types/store.go +++ b/modules/light-clients/07-tendermint/types/store.go @@ -43,6 +43,13 @@ var ( KeyIteration = []byte("/iterationKey") ) +// setClientState stores the client state +func setClientState(clientStore sdk.KVStore, cdc codec.BinaryCodec, clientState *ClientState) { + key := host.ClientStateKey() + val := clienttypes.MustMarshalClientState(cdc, clientState) + clientStore.Set(key, val) +} + // SetConsensusState stores the consensus state at the given height. func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensusState *ConsensusState, height exported.Height) { key := host.ConsensusStateKey(height) diff --git a/modules/light-clients/09-localhost/types/client_state.go b/modules/light-clients/09-localhost/types/client_state.go index 1670d4f1e6b..803644a6f68 100644 --- a/modules/light-clients/09-localhost/types/client_state.go +++ b/modules/light-clients/09-localhost/types/client_state.go @@ -93,8 +93,8 @@ func (cs *ClientState) CheckHeaderAndUpdateState( // Thus, CheckMisbehaviourAndUpdateState returns an error for localhost func (cs ClientState) CheckMisbehaviourAndUpdateState( _ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.Misbehaviour, -) (exported.ClientState, error) { - return nil, sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "cannot submit misbehaviour to localhost client") +) error { + return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "cannot submit misbehaviour to localhost client") } // CheckSubstituteAndUpdateState returns an error. The localhost cannot be modified by diff --git a/modules/light-clients/09-localhost/types/client_state_test.go b/modules/light-clients/09-localhost/types/client_state_test.go index a54cc8efe9a..a02350433b1 100644 --- a/modules/light-clients/09-localhost/types/client_state_test.go +++ b/modules/light-clients/09-localhost/types/client_state_test.go @@ -172,9 +172,8 @@ func (suite *LocalhostTestSuite) TestCheckHeaderAndUpdateState() { func (suite *LocalhostTestSuite) TestMisbehaviourAndUpdateState() { clientState := types.NewClientState("chainID", clientHeight) - cs, err := clientState.CheckMisbehaviourAndUpdateState(suite.ctx, nil, nil, nil) + err := clientState.CheckMisbehaviourAndUpdateState(suite.ctx, nil, nil, nil) suite.Require().Error(err) - suite.Require().Nil(cs) } func (suite *LocalhostTestSuite) TestProposedHeaderAndUpdateState() {