From 7200c7f13e20dda059bc3bbc2d084832bb7829d0 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 19 Feb 2024 16:15:42 +0100 Subject: [PATCH 1/3] refactor: remove hardcoding of tendermint self client validation (#5836) * chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist * chore: update service definition URL in 08-wasm stargate accepted queries * chore: add doc comment to querier test, address nit to move defaultAcceptList * feat(draft): add custom client validator func * feat: add SelfClientValidator type alias func and refactor tests to confirm it works * refactor: updated ibc client keeper to use interface type for self client validation of consensus parameters * lint: make lint-fix * chore: merge main and fix linter * test: cleaned up GetSelfConsensusState tests * test: added test cases for custom validator logic * nit: rename receiver arg * fix: put back ibctm import from merge conflicts --------- Co-authored-by: chatton --- .../core/02-client/keeper/client_validator.go | 126 +++++++++++++ modules/core/02-client/keeper/keeper.go | 129 +++---------- modules/core/02-client/keeper/keeper_test.go | 177 ++++++++++++++---- modules/core/02-client/types/client.go | 7 + modules/core/02-client/types/errors.go | 1 + testing/mock/client_validator.go | 31 +++ 6 files changed, 328 insertions(+), 143 deletions(-) create mode 100644 modules/core/02-client/keeper/client_validator.go create mode 100644 testing/mock/client_validator.go diff --git a/modules/core/02-client/keeper/client_validator.go b/modules/core/02-client/keeper/client_validator.go new file mode 100644 index 00000000000..7010c5958e4 --- /dev/null +++ b/modules/core/02-client/keeper/client_validator.go @@ -0,0 +1,126 @@ +package keeper + +import ( + "reflect" + + errorsmod "cosmossdk.io/errors" + upgradetypes "cosmossdk.io/x/upgrade/types" + + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cometbft/cometbft/light" + + "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" + ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" + "github.com/cosmos/ibc-go/v8/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" +) + +var _ types.SelfClientValidator = (*TendermintClientValidator)(nil) + +// TendermintClientValidator implements the SelfClientValidator interface. +type TendermintClientValidator struct { + stakingKeeper types.StakingKeeper +} + +// NewTendermintClientValidator creates and returns a new SelfClientValidator for tendermint consensus. +func NewTendermintClientValidator(stakingKeeper types.StakingKeeper) *TendermintClientValidator { + return &TendermintClientValidator{ + stakingKeeper: stakingKeeper, + } +} + +// GetSelfConsensusState implements types.SelfClientValidatorI. +func (tcv *TendermintClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + selfHeight, ok := height.(types.Height) + if !ok { + return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected %T, got %T", types.Height{}, height) + } + + // check that height revision matches chainID revision + revision := types.ParseChainID(ctx.ChainID()) + if revision != height.GetRevisionNumber() { + return nil, errorsmod.Wrapf(types.ErrInvalidHeight, "chainID revision number does not match height revision number: expected %d, got %d", revision, height.GetRevisionNumber()) + } + + histInfo, err := tcv.stakingKeeper.GetHistoricalInfo(ctx, int64(selfHeight.RevisionHeight)) + if err != nil { + return nil, errorsmod.Wrapf(err, "height %d", selfHeight.RevisionHeight) + } + + consensusState := &ibctm.ConsensusState{ + Timestamp: histInfo.Header.Time, + Root: commitmenttypes.NewMerkleRoot(histInfo.Header.GetAppHash()), + NextValidatorsHash: histInfo.Header.NextValidatorsHash, + } + + return consensusState, nil +} + +// ValidateSelfClient implements types.SelfClientValidatorI. +func (tcv *TendermintClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + tmClient, ok := clientState.(*ibctm.ClientState) + if !ok { + return errorsmod.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T", &ibctm.ClientState{}, tmClient) + } + + if !tmClient.FrozenHeight.IsZero() { + return types.ErrClientFrozen + } + + if ctx.ChainID() != tmClient.ChainId { + return errorsmod.Wrapf(types.ErrInvalidClient, "invalid chain-id. expected: %s, got: %s", + ctx.ChainID(), tmClient.ChainId) + } + + revision := types.ParseChainID(ctx.ChainID()) + + // client must be in the same revision as executing chain + if tmClient.LatestHeight.RevisionNumber != revision { + return errorsmod.Wrapf(types.ErrInvalidClient, "client is not in the same revision as the chain. expected revision: %d, got: %d", + tmClient.LatestHeight.RevisionNumber, revision) + } + + selfHeight := types.NewHeight(revision, uint64(ctx.BlockHeight())) + if tmClient.LatestHeight.GTE(selfHeight) { + return errorsmod.Wrapf(types.ErrInvalidClient, "client has LatestHeight %d greater than or equal to chain height %d", + tmClient.LatestHeight, selfHeight) + } + + expectedProofSpecs := commitmenttypes.GetSDKSpecs() + if !reflect.DeepEqual(expectedProofSpecs, tmClient.ProofSpecs) { + return errorsmod.Wrapf(types.ErrInvalidClient, "client has invalid proof specs. expected: %v got: %v", + expectedProofSpecs, tmClient.ProofSpecs) + } + + if err := light.ValidateTrustLevel(tmClient.TrustLevel.ToTendermint()); err != nil { + return errorsmod.Wrapf(types.ErrInvalidClient, "trust-level invalid: %v", err) + } + + expectedUbdPeriod, err := tcv.stakingKeeper.UnbondingTime(ctx) + if err != nil { + return errorsmod.Wrapf(err, "failed to retrieve unbonding period") + } + + if expectedUbdPeriod != tmClient.UnbondingPeriod { + return errorsmod.Wrapf(types.ErrInvalidClient, "invalid unbonding period. expected: %s, got: %s", + expectedUbdPeriod, tmClient.UnbondingPeriod) + } + + if tmClient.UnbondingPeriod < tmClient.TrustingPeriod { + return errorsmod.Wrapf(types.ErrInvalidClient, "unbonding period must be greater than trusting period. unbonding period (%d) < trusting period (%d)", + tmClient.UnbondingPeriod, tmClient.TrustingPeriod) + } + + if len(tmClient.UpgradePath) != 0 { + // For now, SDK IBC implementation assumes that upgrade path (if defined) is defined by SDK upgrade module + expectedUpgradePath := []string{upgradetypes.StoreKey, upgradetypes.KeyUpgradedIBCState} + if !reflect.DeepEqual(expectedUpgradePath, tmClient.UpgradePath) { + return errorsmod.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %v, got %v", + expectedUpgradePath, tmClient.UpgradePath) + } + } + + return nil +} diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 2ef23ee5e58..21ab11ffad9 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -3,7 +3,6 @@ package keeper import ( "errors" "fmt" - "reflect" "strings" errorsmod "cosmossdk.io/errors" @@ -15,12 +14,8 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cometbft/cometbft/light" - "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" - commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" - ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" "github.com/cosmos/ibc-go/v8/modules/core/exported" ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" localhost "github.com/cosmos/ibc-go/v8/modules/light-clients/09-localhost" @@ -29,21 +24,23 @@ import ( // Keeper represents a type that grants read and write permissions to any client // state information type Keeper struct { - storeKey storetypes.StoreKey - cdc codec.BinaryCodec - legacySubspace types.ParamSubspace - stakingKeeper types.StakingKeeper - upgradeKeeper types.UpgradeKeeper + storeKey storetypes.StoreKey + cdc codec.BinaryCodec + legacySubspace types.ParamSubspace + selfClientValidator types.SelfClientValidator + stakingKeeper types.StakingKeeper + upgradeKeeper types.UpgradeKeeper } // NewKeeper creates a new NewKeeper instance func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, legacySubspace types.ParamSubspace, sk types.StakingKeeper, uk types.UpgradeKeeper) Keeper { return Keeper{ - storeKey: key, - cdc: cdc, - legacySubspace: legacySubspace, - stakingKeeper: sk, - upgradeKeeper: uk, + storeKey: key, + cdc: cdc, + legacySubspace: legacySubspace, + selfClientValidator: NewTendermintClientValidator(sk), + stakingKeeper: sk, + upgradeKeeper: uk, } } @@ -63,6 +60,15 @@ func (k Keeper) UpdateLocalhostClient(ctx sdk.Context, clientState exported.Clie return clientState.UpdateState(ctx, k.cdc, k.ClientStore(ctx, exported.LocalhostClientID), nil) } +// SetSelfClientValidator sets a custom self client validation function. +func (k *Keeper) SetSelfClientValidator(selfClientValidator types.SelfClientValidator) { + if selfClientValidator == nil { + panic(fmt.Errorf("cannot set a nil self client validator")) + } + + k.selfClientValidator = selfClientValidator +} + // GenerateClientIdentifier returns the next client identifier. func (k Keeper) GenerateClientIdentifier(ctx sdk.Context, clientType string) string { nextClientSeq := k.GetNextClientSequence(ctx) @@ -282,96 +288,15 @@ func (k Keeper) GetLatestClientConsensusState(ctx sdk.Context, clientID string) // and returns the expected consensus state at that height. // For now, can only retrieve self consensus states for the current revision func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { - selfHeight, ok := height.(types.Height) - if !ok { - return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected %T, got %T", types.Height{}, height) - } - // check that height revision matches chainID revision - revision := types.ParseChainID(ctx.ChainID()) - if revision != height.GetRevisionNumber() { - return nil, errorsmod.Wrapf(types.ErrInvalidHeight, "chainID revision number does not match height revision number: expected %d, got %d", revision, height.GetRevisionNumber()) - } - histInfo, err := k.stakingKeeper.GetHistoricalInfo(ctx, int64(selfHeight.RevisionHeight)) - if err != nil { - return nil, errorsmod.Wrapf(err, "height %d", selfHeight.RevisionHeight) - } - - consensusState := &ibctm.ConsensusState{ - Timestamp: histInfo.Header.Time, - Root: commitmenttypes.NewMerkleRoot(histInfo.Header.GetAppHash()), - NextValidatorsHash: histInfo.Header.NextValidatorsHash, - } - - return consensusState, nil + return k.selfClientValidator.GetSelfConsensusState(ctx, height) } -// ValidateSelfClient validates the client parameters for a client of the running chain -// This function is only used to validate the client state the counterparty stores for this chain -// Client must be in same revision as the executing chain +// ValidateSelfClient validates the client parameters for a client of the running chain. +// This function is only used to validate the client state the counterparty stores for this chain. +// NOTE: If the client type is not of type Tendermint then delegate to a custom client validator function. +// This allows support for non-Tendermint clients, for example 08-wasm clients. func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { - tmClient, ok := clientState.(*ibctm.ClientState) - if !ok { - return errorsmod.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T", - &ibctm.ClientState{}, tmClient) - } - - if !tmClient.FrozenHeight.IsZero() { - return types.ErrClientFrozen - } - - if ctx.ChainID() != tmClient.ChainId { - return errorsmod.Wrapf(types.ErrInvalidClient, "invalid chain-id. expected: %s, got: %s", - ctx.ChainID(), tmClient.ChainId) - } - - revision := types.ParseChainID(ctx.ChainID()) - - // client must be in the same revision as executing chain - if tmClient.LatestHeight.RevisionNumber != revision { - return errorsmod.Wrapf(types.ErrInvalidClient, "client is not in the same revision as the chain. expected revision: %d, got: %d", - tmClient.LatestHeight.RevisionNumber, revision) - } - - selfHeight := types.NewHeight(revision, uint64(ctx.BlockHeight())) - if tmClient.LatestHeight.GTE(selfHeight) { - return errorsmod.Wrapf(types.ErrInvalidClient, "client has LatestHeight %d greater than or equal to chain height %d", - tmClient.LatestHeight, selfHeight) - } - - expectedProofSpecs := commitmenttypes.GetSDKSpecs() - if !reflect.DeepEqual(expectedProofSpecs, tmClient.ProofSpecs) { - return errorsmod.Wrapf(types.ErrInvalidClient, "client has invalid proof specs. expected: %v got: %v", - expectedProofSpecs, tmClient.ProofSpecs) - } - - if err := light.ValidateTrustLevel(tmClient.TrustLevel.ToTendermint()); err != nil { - return errorsmod.Wrapf(types.ErrInvalidClient, "trust-level invalid: %v", err) - } - - expectedUbdPeriod, err := k.stakingKeeper.UnbondingTime(ctx) - if err != nil { - return errorsmod.Wrapf(err, "failed to retrieve unbonding period") - } - - if expectedUbdPeriod != tmClient.UnbondingPeriod { - return errorsmod.Wrapf(types.ErrInvalidClient, "invalid unbonding period. expected: %s, got: %s", - expectedUbdPeriod, tmClient.UnbondingPeriod) - } - - if tmClient.UnbondingPeriod < tmClient.TrustingPeriod { - return errorsmod.Wrapf(types.ErrInvalidClient, "unbonding period must be greater than trusting period. unbonding period (%d) < trusting period (%d)", - tmClient.UnbondingPeriod, tmClient.TrustingPeriod) - } - - if len(tmClient.UpgradePath) != 0 { - // For now, SDK IBC implementation assumes that upgrade path (if defined) is defined by SDK upgrade module - expectedUpgradePath := []string{upgradetypes.StoreKey, upgradetypes.KeyUpgradedIBCState} - if !reflect.DeepEqual(expectedUpgradePath, tmClient.UpgradePath) { - return errorsmod.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %v, got %v", - expectedUpgradePath, tmClient.UpgradePath) - } - } - return nil + return k.selfClientValidator.ValidateSelfClient(ctx, clientState) } // GetUpgradePlan executes the upgrade keeper GetUpgradePlan function. diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index c879cdd0920..6302ffb362b 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -29,6 +29,7 @@ import ( ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" localhost "github.com/cosmos/ibc-go/v8/modules/light-clients/09-localhost" ibctesting "github.com/cosmos/ibc-go/v8/testing" + "github.com/cosmos/ibc-go/v8/testing/mock" "github.com/cosmos/ibc-go/v8/testing/simapp" ) @@ -149,78 +150,130 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() { testClientHeight := types.GetSelfHeight(suite.chainA.GetContext()) testClientHeight.RevisionHeight-- + var clientState exported.ClientState + testCases := []struct { - name string - clientState exported.ClientState - expPass bool + name string + malleate func() + expError error }{ { "success", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - true, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + nil, }, { "success with nil UpgradePath", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), nil), - true, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), nil) + }, + nil, + }, + { + "success with custom self validator: solomachine", + func() { + clientState = solomachine.NewClientState(1, &solomachine.ConsensusState{}) + + smSelfClientValidator := &mock.ClientValidator{ + ValidateSelfClientFn: func(ctx sdk.Context, clientState exported.ClientState) error { + smClientState, ok := clientState.(*solomachine.ClientState) + suite.Require().True(ok) + suite.Require().Equal(uint64(1), smClientState.Sequence) + + return nil + }, + } + + // add some mock validation logic + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetSelfClientValidator(smSelfClientValidator) + }, + nil, }, { "frozen client", - &ibctm.ClientState{ChainId: suite.chainA.ChainID, TrustLevel: ibctm.DefaultTrustLevel, TrustingPeriod: trustingPeriod, UnbondingPeriod: ubdPeriod, MaxClockDrift: maxClockDrift, FrozenHeight: testClientHeight, LatestHeight: testClientHeight, ProofSpecs: commitmenttypes.GetSDKSpecs(), UpgradePath: ibctesting.UpgradePath}, - false, + func() { + clientState = &ibctm.ClientState{ChainId: suite.chainA.ChainID, TrustLevel: ibctm.DefaultTrustLevel, TrustingPeriod: trustingPeriod, UnbondingPeriod: ubdPeriod, MaxClockDrift: maxClockDrift, FrozenHeight: testClientHeight, LatestHeight: testClientHeight, ProofSpecs: commitmenttypes.GetSDKSpecs(), UpgradePath: ibctesting.UpgradePath} + }, + types.ErrClientFrozen, }, { "incorrect chainID", - ibctm.NewClientState("gaiatestnet", ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - false, + func() { + clientState = ibctm.NewClientState("gaiatestnet", ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid client height", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, types.GetSelfHeight(suite.chainA.GetContext()).Increment().(types.Height), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, types.GetSelfHeight(suite.chainA.GetContext()).Increment().(types.Height), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid client type", - solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}), - false, + func() { + clientState = solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}) + }, + types.ErrInvalidClient, }, { "invalid client revision", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeightRevision1, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeightRevision1, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid proof specs", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, nil, ibctesting.UpgradePath), - false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, nil, ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid trust level", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid unbonding period", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod+10, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod+10, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid trusting period", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, ubdPeriod+10, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, ubdPeriod+10, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + types.ErrInvalidClient, }, { "invalid upgrade path", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), []string{"bad", "upgrade", "path"}), - false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), []string{"bad", "upgrade", "path"}) + }, + types.ErrInvalidClient, }, } for _, tc := range testCases { tc := tc - suite.Run(tc.name, func() { - err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ValidateSelfClient(suite.chainA.GetContext(), tc.clientState) - if tc.expPass { + suite.SetupTest() + + tc.malleate() + + err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ValidateSelfClient(suite.chainA.GetContext(), clientState) + + expPass := tc.expError == nil + if expPass { suite.Require().NoError(err, "expected valid client for case: %s", tc.name) } else { suite.Require().Error(err, "expected invalid client for case: %s", tc.name) @@ -308,27 +361,69 @@ func (suite KeeperTestSuite) TestGetAllGenesisMetadata() { //nolint:govet // thi }) } -func (suite KeeperTestSuite) TestGetConsensusState() { //nolint:govet // this is a test, we are okay with copying locks - suite.ctx = suite.ctx.WithBlockHeight(10) +func (suite *KeeperTestSuite) TestGetConsensusState() { + var height types.Height + cases := []struct { - name string - height types.Height - expPass bool + name string + malleate func() + expError error }{ - {"zero height", types.ZeroHeight(), false}, - {"height > latest height", types.NewHeight(0, uint64(suite.ctx.BlockHeight())+1), false}, - {"latest height - 1", types.NewHeight(0, uint64(suite.ctx.BlockHeight())-1), true}, - {"latest height", types.GetSelfHeight(suite.ctx), true}, + {"zero height", func() { + height = types.ZeroHeight() + }, stakingtypes.ErrNoHistoricalInfo}, + {"height > latest height", func() { + height = types.NewHeight(0, uint64(suite.ctx.BlockHeight())+1) + }, stakingtypes.ErrNoHistoricalInfo}, + { + name: "custom client validator: failure", + malleate: func() { + clientValidator := &mock.ClientValidator{ + GetSelfConsensusStateFn: func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + return nil, mock.MockApplicationCallbackError + }, + } + suite.keeper.SetSelfClientValidator(clientValidator) + }, + expError: mock.MockApplicationCallbackError, + }, + { + name: "custom client validator: success", + malleate: func() { + clientValidator := &mock.ClientValidator{ + GetSelfConsensusStateFn: func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + return &solomachine.ConsensusState{}, nil + }, + } + suite.keeper.SetSelfClientValidator(clientValidator) + }, + expError: nil, + }, + {"latest height - 1", func() { + height = types.NewHeight(0, uint64(suite.ctx.BlockHeight())-1) + }, nil}, + {"latest height", func() { + height = types.GetSelfHeight(suite.ctx) + }, nil}, } for i, tc := range cases { + suite.SetupTest() + suite.ctx = suite.ctx.WithBlockHeight(10) tc := tc - cs, err := suite.keeper.GetSelfConsensusState(suite.ctx, tc.height) - if tc.expPass { + + height = types.ZeroHeight() + + tc.malleate() + + cs, err := suite.keeper.GetSelfConsensusState(suite.ctx, height) + + expPass := tc.expError == nil + if expPass { suite.Require().NoError(err, "Case %d should have passed: %s", i, tc.name) suite.Require().NotNil(cs, "Case %d should have passed: %s", i, tc.name) } else { - suite.Require().Error(err, "Case %d should have failed: %s", i, tc.name) + suite.Require().ErrorIs(err, tc.expError, "Case %d should have failed: %s", i, tc.name) suite.Require().Nil(cs, "Case %d should have failed: %s", i, tc.name) } } diff --git a/modules/core/02-client/types/client.go b/modules/core/02-client/types/client.go index 31da1a54e70..2b2447aced8 100644 --- a/modules/core/02-client/types/client.go +++ b/modules/core/02-client/types/client.go @@ -11,6 +11,7 @@ import ( errorsmod "cosmossdk.io/errors" codectypes "github.com/cosmos/cosmos-sdk/codec/types" + sdk "github.com/cosmos/cosmos-sdk/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" "github.com/cosmos/ibc-go/v8/modules/core/exported" @@ -21,6 +22,12 @@ var ( _ codectypes.UnpackInterfacesMessage = (*ConsensusStateWithHeight)(nil) ) +// SelfClientValidator defines an interface used to validate an IBC ClientState against a host chain's underlying consensus parameters. +type SelfClientValidator interface { + GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) + ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error +} + // NewIdentifiedClientState creates a new IdentifiedClientState instance func NewIdentifiedClientState(clientID string, clientState exported.ClientState) IdentifiedClientState { msg, ok := clientState.(proto.Message) diff --git a/modules/core/02-client/types/errors.go b/modules/core/02-client/types/errors.go index 3a726378cbc..3dc556901f8 100644 --- a/modules/core/02-client/types/errors.go +++ b/modules/core/02-client/types/errors.go @@ -36,4 +36,5 @@ var ( ErrClientNotActive = errorsmod.Register(SubModuleName, 29, "client state is not active") ErrFailedMembershipVerification = errorsmod.Register(SubModuleName, 30, "membership verification failed") ErrFailedNonMembershipVerification = errorsmod.Register(SubModuleName, 31, "non-membership verification failed") + ErrClientTypeNotSupported = errorsmod.Register(SubModuleName, 32, "client type not supported") ) diff --git a/testing/mock/client_validator.go b/testing/mock/client_validator.go new file mode 100644 index 00000000000..ad7be616b20 --- /dev/null +++ b/testing/mock/client_validator.go @@ -0,0 +1,31 @@ +package mock + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + "github.com/cosmos/ibc-go/v8/modules/core/exported" +) + +var _ clienttypes.SelfClientValidator = (*ClientValidator)(nil) + +type ClientValidator struct { + GetSelfConsensusStateFn func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) + ValidateSelfClientFn func(ctx sdk.Context, clientState exported.ClientState) error +} + +func (cv *ClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + if cv.GetSelfConsensusStateFn == nil { + return nil, nil + } + + return cv.GetSelfConsensusStateFn(ctx, height) +} + +func (cv *ClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + if cv.ValidateSelfClientFn == nil { + return nil + } + + return cv.ValidateSelfClientFn(ctx, clientState) +} From a7ce91d7a4f8ad419592068ca3d219448ea92921 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 19 Feb 2024 15:21:43 +0000 Subject: [PATCH 2/3] Custom self client validation for 08 wasm tendermint clients (#5858) Co-Authored-By: @charleenfei --- Makefile | 4 ++ .../08-wasm/testing/simapp/app.go | 5 ++ .../08-wasm/types/client_validator.go | 62 +++++++++++++++++++ scripts/build-wasm-simapp-docker.sh | 16 +++++ 4 files changed, 87 insertions(+) create mode 100644 modules/light-clients/08-wasm/types/client_validator.go create mode 100755 scripts/build-wasm-simapp-docker.sh diff --git a/Makefile b/Makefile index 96bc12dda24..81df6ad9a3d 100644 --- a/Makefile +++ b/Makefile @@ -121,6 +121,10 @@ $(BUILD_TARGETS): go.sum $(BUILDDIR)/ $(BUILDDIR)/: mkdir -p $(BUILDDIR)/ +#? build-docker-wasm: Build wasm simapp with specified tag. +build-docker-wasm: + ./scripts/build-wasm-simapp-docker.sh $(tag) + .PHONY: build build-linux #? distclean: Run `make clean` diff --git a/modules/light-clients/08-wasm/testing/simapp/app.go b/modules/light-clients/08-wasm/testing/simapp/app.go index 830ca619957..6fb165049e3 100644 --- a/modules/light-clients/08-wasm/testing/simapp/app.go +++ b/modules/light-clients/08-wasm/testing/simapp/app.go @@ -128,6 +128,7 @@ import ( ibctransfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" ibc "github.com/cosmos/ibc-go/v8/modules/core" ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client" + clientkeeper "github.com/cosmos/ibc-go/v8/modules/core/02-client/keeper" ibcclienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" ibcconnectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types" porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" @@ -416,6 +417,10 @@ func NewSimApp( app.IBCKeeper = ibckeeper.NewKeeper( appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) + + tmClientValidator := clientkeeper.NewTendermintClientValidator(app.StakingKeeper) + app.IBCKeeper.ClientKeeper.SetSelfClientValidator(wasmtypes.NewWasmTMClientValidator(appCodec, tmClientValidator)) + // Register the proposal types // Deprecated: Avoid adding new handlers, instead use the new proposal flow // by granting the governance module the right to execute the message. diff --git a/modules/light-clients/08-wasm/types/client_validator.go b/modules/light-clients/08-wasm/types/client_validator.go new file mode 100644 index 00000000000..1718a29a757 --- /dev/null +++ b/modules/light-clients/08-wasm/types/client_validator.go @@ -0,0 +1,62 @@ +package types + +import ( + errorsmod "cosmossdk.io/errors" + + "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" + + clientkeeper "github.com/cosmos/ibc-go/v8/modules/core/02-client/keeper" + clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + "github.com/cosmos/ibc-go/v8/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" +) + +type WasmTMClientValidator struct { + cdc codec.BinaryCodec + tm *clientkeeper.TendermintClientValidator +} + +var _ clienttypes.SelfClientValidator = (*WasmTMClientValidator)(nil) + +// NewWasmTMClientValidator creates and returns a new SelfClientValidator for wasm tendermint consensus. +func NewWasmTMClientValidator(cdc codec.BinaryCodec, tm *clientkeeper.TendermintClientValidator) *WasmTMClientValidator { + return &WasmTMClientValidator{ + cdc: cdc, + tm: tm, + } +} + +func (w WasmTMClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + consensusState, err := w.tm.GetSelfConsensusState(ctx, height) + if err != nil { + return nil, err + } + + // encode consensusState to wasm.ConsensusState.Data + bz, err := w.cdc.Marshal(consensusState) + if err != nil { + return nil, err + } + + wasmConsensusState := &ConsensusState{ + Data: bz, + } + + return wasmConsensusState, nil +} + +func (w WasmTMClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + wasmClientState, ok := clientState.(*ClientState) + if !ok { + return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "client must be a wasm client, expected: %T, got: %T", ClientState{}, wasmClientState) + } + + // unmarshal the wasmClientState bytes into tendermint client and call self validation + var tmClientState *ibctm.ClientState + if err := w.cdc.Unmarshal(wasmClientState.Data, tmClientState); err != nil { + return err + } + + return w.tm.ValidateSelfClient(ctx, tmClientState) +} diff --git a/scripts/build-wasm-simapp-docker.sh b/scripts/build-wasm-simapp-docker.sh new file mode 100755 index 00000000000..739097ae038 --- /dev/null +++ b/scripts/build-wasm-simapp-docker.sh @@ -0,0 +1,16 @@ +#!/bin/bash + +set -eou pipefail + +# build_wasm_image extracts the correct libwasm version and checksum +# based on the go.mod and builds a docker image with the provided tag. +function build_wasm_image(){ + local version="$(scripts/get-libwasm-version.py --get-version)" + local checksum="$(scripts/get-libwasm-version.py --get-checksum)" + docker build . -t "${1}" -f modules/light-clients/08-wasm/Dockerfile --build-arg LIBWASM_VERSION=${version} --build-arg LIBWASM_CHECKSUM=${checksum} +} + +# default to latest if no tag is specified. +TAG="${1:-ibc-go-wasm-simd:latest}" + +build_wasm_image "${TAG}" \ No newline at end of file From 83e79c2e3993645c7a6d473748e562044d2e1df9 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 21 Feb 2024 16:06:28 +0100 Subject: [PATCH 3/3] fix: remove self client validation of 08-wasm client in wasm simapp --- modules/light-clients/08-wasm/testing/simapp/app.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/modules/light-clients/08-wasm/testing/simapp/app.go b/modules/light-clients/08-wasm/testing/simapp/app.go index 6fb165049e3..6580149748b 100644 --- a/modules/light-clients/08-wasm/testing/simapp/app.go +++ b/modules/light-clients/08-wasm/testing/simapp/app.go @@ -128,7 +128,6 @@ import ( ibctransfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" ibc "github.com/cosmos/ibc-go/v8/modules/core" ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client" - clientkeeper "github.com/cosmos/ibc-go/v8/modules/core/02-client/keeper" ibcclienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" ibcconnectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types" porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" @@ -418,9 +417,6 @@ func NewSimApp( appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) - tmClientValidator := clientkeeper.NewTendermintClientValidator(app.StakingKeeper) - app.IBCKeeper.ClientKeeper.SetSelfClientValidator(wasmtypes.NewWasmTMClientValidator(appCodec, tmClientValidator)) - // Register the proposal types // Deprecated: Avoid adding new handlers, instead use the new proposal flow // by granting the governance module the right to execute the message.