refactor: removing CheckMisbehaviourAndUpdateState from ClientState i…

…nterface (cosmos#1212)

* refactor: removing CheckForMisbehaviourAndUpdateState from ClientState interface and associated client implemtations

* chore: changelog

CHANGELOG.md

@@ -44,8 +44,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 * (modules/core/02-client) [\#1188](https://github.com/cosmos/ibc-go/pull/1188/files) Routing `MsgSubmitMisbehaviour` to `UpdateClient` keeper function. Deprecating `SubmitMisbehaviour` endpoint.
-
-
 * (modules/core/02-client) [\#1208](https://github.com/cosmos/ibc-go/pull/1208) Replace `CheckHeaderAndUpdateState` usage in 02-client with calls to `VerifyClientMessage`, `CheckForMisbehaviour`, `UpdateStateOnMisbehaviour` and `UpdateState`. 
 * (modules/light-clients/09-localhost) [\#1187](https://github.com/cosmos/ibc-go/pull/1187/) Removing localhost light client implementation as it is not functional. 
 * [\#1186](https://github.com/cosmos/ibc-go/pull/1186/files) Removing `GetRoot` function from ConsensusState interface in `02-client`. `GetRoot` is unused by core IBC. 
@@ -54,6 +52,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/02-client) [\#1170](https://github.com/cosmos/ibc-go/pull/1170) Updating `ClientUpdateProposal` to set client state in lightclient implementations `CheckSubstituteAndUpdateState` methods.
 * (modules/core/02-client) [\#1197](https://github.com/cosmos/ibc-go/pull/1197) Adding `CheckForMisbehaviour` to `ClientState` interface. 
 * (modules/core/02-client) [\#1195](https://github.com/cosmos/ibc-go/pull/1210) Removing `CheckHeaderAndUpdateState` from `ClientState` interface & associated light client implementations. 
+* (modules/core/02-client) [\#1189](https://github.com/cosmos/ibc-go/pull/1212) Removing `CheckMisbehaviourAndUpdateState` from `ClientState` interface & associated light client implementations.
 * (modules/core/exported) [\#1206](https://github.com/cosmos/ibc-go/pull/1206) Adding new method `UpdateState` to `ClientState` interface.
 
 ### Features

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

@@ -161,45 +161,3 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 
 	return nil
 }
-
-// CheckMisbehaviourAndUpdateState checks for client misbehaviour and freezes the
-// client if so.
-func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, clientID string, misbehaviour exported.ClientMessage) error {
-	clientState, found := k.GetClientState(ctx, clientID)
-	if !found {
-		return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", clientID)
-	}
-
-	clientStore := k.ClientStore(ctx, clientID)
-
-	if status := clientState.Status(ctx, clientStore, k.cdc); status != exported.Active {
-		return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot process misbehaviour for client (%s) with status %s", clientID, status)
-	}
-
-	if err := misbehaviour.ValidateBasic(); err != nil {
-		return err
-	}
-
-	clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, misbehaviour)
-	if err != nil {
-		return err
-	}
-
-	k.SetClientState(ctx, clientID, clientState)
-	k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID)
-
-	defer func() {
-		telemetry.IncrCounterWithLabels(
-			[]string{"ibc", "client", "misbehaviour"},
-			1,
-			[]metrics.Label{
-				telemetry.NewLabel(types.LabelClientType, misbehaviour.ClientType()),
-				telemetry.NewLabel(types.LabelClientID, clientID),
-			},
-		)
-	}()
-
-	EmitSubmitMisbehaviourEvent(ctx, clientID, clientState)
-
-	return nil
-}

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

@@ -6,7 +6,6 @@ import (
 	"time"
 
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
-	tmtypes "github.com/tendermint/tendermint/types"
 
 	"github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
 	clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
@@ -15,7 +14,6 @@ import (
 	solomachinetypes "github.com/cosmos/ibc-go/v3/modules/light-clients/06-solomachine/types"
 	ibctmtypes "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
-	ibctestingmock "github.com/cosmos/ibc-go/v3/testing/mock"
 )
 
 func (suite *KeeperTestSuite) TestCreateClient() {
@@ -408,253 +406,6 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 
 }
 
-func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
-	var (
-		clientID string
-		err      error
-	)
-
-	altPrivVal := ibctestingmock.NewPV()
-	altPubKey, err := altPrivVal.GetPubKey()
-	suite.Require().NoError(err)
-	altVal := tmtypes.NewValidator(altPubKey, 4)
-
-	// Set valSet here with suite.valSet so it doesn't get reset on each testcase
-	valSet := suite.valSet
-	valsHash := valSet.Hash()
-
-	// Create bothValSet with both suite validator and altVal
-	bothValSet := tmtypes.NewValidatorSet(append(suite.valSet.Validators, altVal))
-	bothValsHash := bothValSet.Hash()
-	// Create alternative validator set with only altVal
-	altValSet := tmtypes.NewValidatorSet([]*tmtypes.Validator{altVal})
-
-	// Create signer array and ensure it is in same order as bothValSet
-	_, suiteVal := suite.valSet.GetByIndex(0)
-	bothSigners := make(map[string]tmtypes.PrivValidator, 2)
-	bothSigners[suiteVal.Address.String()] = suite.privVal
-	bothSigners[altVal.Address.String()] = altPrivVal
-
-	altSigners := make(map[string]tmtypes.PrivValidator, 1)
-	altSigners[altVal.Address.String()] = altPrivVal
-
-	// Create valid Misbehaviour by making a duplicate header that signs over different block time
-	altTime := suite.ctx.BlockTime().Add(time.Minute)
-
-	heightPlus3 := types.NewHeight(0, height+3)
-	heightPlus5 := types.NewHeight(0, height+5)
-
-	testCases := []struct {
-		name         string
-		misbehaviour *ibctmtypes.Misbehaviour
-		malleate     func() error
-		expPass      bool
-	}{
-		{
-			"trusting period misbehavior should pass",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothValSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = bothValsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				return err
-			},
-			true,
-		},
-		{
-			"time misbehavior should pass",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+5), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothValSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = bothValsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				return err
-			},
-			true,
-		},
-		{
-			"misbehavior at later height should pass",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, valSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = valsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				// store intermediate consensus state to check that trustedHeight does not need to be highest consensus state before header height
-				intermediateConsState := &ibctmtypes.ConsensusState{
-					Timestamp:          suite.now.Add(time.Minute),
-					NextValidatorsHash: suite.chainB.Vals.Hash(),
-				}
-				suite.keeper.SetClientConsensusState(suite.ctx, clientID, heightPlus3, intermediateConsState)
-
-				clientState.LatestHeight = heightPlus3
-				suite.keeper.SetClientState(suite.ctx, clientID, clientState)
-
-				return err
-			},
-			true,
-		},
-		{
-			"misbehavior at later height with different trusted heights should pass",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), heightPlus3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothValSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = valsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				// store trusted consensus state for Header2
-				intermediateConsState := &ibctmtypes.ConsensusState{
-					Timestamp:          suite.now.Add(time.Minute),
-					NextValidatorsHash: bothValsHash,
-				}
-				suite.keeper.SetClientConsensusState(suite.ctx, clientID, heightPlus3, intermediateConsState)
-
-				clientState.LatestHeight = heightPlus3
-				suite.keeper.SetClientState(suite.ctx, clientID, clientState)
-
-				return err
-			},
-			true,
-		},
-		{
-			"misbehavior ValidateBasic fails: misbehaviour height is at same height as trusted height",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothValSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = bothValsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				return err
-			},
-			false,
-		},
-		{
-			"trusted ConsensusState1 not found",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), heightPlus3, altTime, bothValSet, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, valSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = valsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-				// intermediate consensus state at height + 3 is not created
-				return err
-			},
-			false,
-		},
-		{
-			"trusted ConsensusState2 not found",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, valSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus5.RevisionHeight+1), heightPlus3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothValSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = valsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-				// intermediate consensus state at height + 3 is not created
-				return err
-			},
-			false,
-		},
-		{
-			"client state not found",
-			&ibctmtypes.Misbehaviour{},
-			func() error { return nil },
-			false,
-		},
-		{
-			"client already is not active - client is frozen",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothValSet, bothSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				suite.consensusState.NextValidatorsHash = bothValsHash
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				clientState.FrozenHeight = types.NewHeight(0, 1)
-				suite.keeper.SetClientState(suite.ctx, clientID, clientState)
-
-				return err
-			},
-			false,
-		},
-		{
-			"misbehaviour check failed",
-			&ibctmtypes.Misbehaviour{
-				Header1:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, altTime, bothValSet, bothValSet, bothValSet, bothSigners),
-				Header2:  suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight+1), testClientHeight, suite.ctx.BlockTime(), altValSet, altValSet, bothValSet, altSigners),
-				ClientId: clientID,
-			},
-			func() error {
-				clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-				if err != nil {
-					return err
-				}
-				clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
-
-				return err
-			},
-			false,
-		},
-	}
-
-	for i, tc := range testCases {
-		tc := tc
-		i := i
-
-		suite.Run(tc.name, func() {
-			suite.SetupTest()       // reset
-			clientID = testClientID // must be explicitly changed
-
-			err := tc.malleate()
-			suite.Require().NoError(err)
-
-			tc.misbehaviour.ClientId = clientID
-
-			err = suite.keeper.CheckMisbehaviourAndUpdateState(suite.ctx, clientID, tc.misbehaviour)
-
-			if tc.expPass {
-				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
-
-				clientState, found := suite.keeper.GetClientState(suite.ctx, clientID)
-				suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
-				suite.Require().True(!clientState.(*ibctmtypes.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)
-			}
-		})
-	}
-}
-
 func (suite *KeeperTestSuite) TestUpdateClientEventEmission() {
 	path := ibctesting.NewPath(suite.chainA, suite.chainB)
 	suite.coordinator.SetupClients(path)

modules/core/exported/client.go

@@ -69,7 +69,6 @@ type ClientState interface {
 	UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) error
 
 	// Update and Misbehaviour functions
-	CheckMisbehaviourAndUpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) (ClientState, 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

@@ -2,51 +2,8 @@ package types
 
 import (
 	"github.com/cosmos/cosmos-sdk/codec"
-	sdk "github.com/cosmos/cosmos-sdk/types"
-	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
-
-	clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
-	"github.com/cosmos/ibc-go/v3/modules/core/exported"
 )
 
-// CheckMisbehaviourAndUpdateState determines whether or not the currently registered
-// public key signed over two different messages with the same sequence. If this is true
-// the client state is updated to a frozen status.
-// NOTE: Misbehaviour is not tracked for previous public keys, a solo machine may update to
-// a new public key before the misbehaviour is processed. Therefore, misbehaviour is data
-// order processing dependent.
-func (cs ClientState) CheckMisbehaviourAndUpdateState(
-	ctx sdk.Context,
-	cdc codec.BinaryCodec,
-	clientStore sdk.KVStore,
-	misbehaviour exported.ClientMessage,
-) (exported.ClientState, error) {
-
-	soloMisbehaviour, ok := misbehaviour.(*Misbehaviour)
-	if !ok {
-		return nil, sdkerrors.Wrapf(
-			clienttypes.ErrInvalidClientType,
-			"misbehaviour type %T, expected %T", misbehaviour, &Misbehaviour{},
-		)
-	}
-
-	// NOTE: a check that the misbehaviour message data are not equal is done by
-	// misbehaviour.ValidateBasic which is called by the 02-client keeper.
-
-	// verify first signature
-	if err := cs.verifySignatureAndData(cdc, soloMisbehaviour, soloMisbehaviour.SignatureOne); err != nil {
-		return nil, sdkerrors.Wrap(err, "failed to verify signature one")
-	}
-
-	// verify second signature
-	if err := cs.verifySignatureAndData(cdc, soloMisbehaviour, soloMisbehaviour.SignatureTwo); err != nil {
-		return nil, sdkerrors.Wrap(err, "failed to verify signature two")
-	}
-
-	cs.IsFrozen = true
-	return &cs, nil
-}
-
 // verifySignatureAndData verifies that the currently registered public key has signed
 // over the provided data and that the data is valid. The data is valid if it can be
 // unmarshaled into the specified data type.