refactor: modify CheckMisbehaviourAndUpdateState to set client state

CHANGELOG.md

@@ -42,6 +42,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.
 * (transfer) [\#517](https://github.com/cosmos/ibc-go/pull/517) Separates the ICS 26 callback functions from `AppModule` into a new type `IBCModule` for ICS 20 transfer.
 * (modules/core/02-client) [\#536](https://github.com/cosmos/ibc-go/pull/536) GetSelfConsensusState return type changed from bool to error.
 
@@ -214,4 +216,4 @@ Please see the Release Notes for [v0.41.x](https://github.com/cosmos/cosmos-sdk/
 
 The IBC module was removed in the commit hash [da064e13d56add466548135739c5860a9f7ed842](https://github.com/cosmos/cosmos-sdk/commit/da064e13d56add466548135739c5860a9f7ed842) on the SDK. The release for SDK v0.43.0 will be the first release without the IBC module.
 
-Backports should be made to the [release/v0.42.x](https://github.com/cosmos/cosmos-sdk/tree/release/v0.42.x) branch on the SDK.
+Backports should be made to the [release/v0.42.x](https://github.com/cosmos/cosmos-sdk/tree/release/v0.42.x) branch on the SDK.

docs/migrations/v200-to-v300.md

@@ -0,0 +1,30 @@
+# Migrating from v2.0.0 to v3.0.0
+
+This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
+Any changes that must be done by a user of ibc-go should be documented here.
+
+There are four sections based on the four potential user groups of this document:
+- Chains
+- IBC Apps
+- Relayers
+- IBC Light Clients
+
+**Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated to bump the version number on major releases.
+
+## Chains
+
+- No relevant changes were made in this release.
+
+## IBC Apps
+
+- No relevant changes were made in this release.
+
+## Relayers
+
+- No relevant changes were made in this release.
+
+## 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.

modules/core/02-client/keeper/client.go

@@ -218,12 +218,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() {

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!")
 }
 

modules/core/exported/client.go

@@ -61,7 +61,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

modules/light-clients/06-solomachine/types/misbehaviour_handle.go

@@ -19,11 +19,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{},
 		)
@@ -34,16 +34,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

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")
+					}
 				}
 			})
 		}

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

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)
 			}
 		})
 	}

modules/light-clients/07-tendermint/types/store.go

@@ -42,6 +42,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)

modules/light-clients/09-localhost/types/client_state.go

@@ -99,8 +99,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

modules/light-clients/09-localhost/types/client_state_test.go

@@ -171,9 +171,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() {