From 3be0b6338233992e583721e763af19f3ac0b0544 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 15 Dec 2022 17:13:59 +0000 Subject: [PATCH] refactor: require light clients to set the initial client state and consensus state via the client state `Initialize` method (#2936) * set the initial client and consensus state via the client state Initialize method. update godocs * updating godocs * updating migration doc * updating light client guide docs for Initialize method * updating migration doc * Apply suggestions from code review Co-authored-by: Charly * Update docs/ibc/light-clients/client-state.md Co-authored-by: Carlos Rodriguez * adding tests to lightclients * updating 02-client tests Co-authored-by: Charly Co-authored-by: Carlos Rodriguez --- docs/ibc/light-clients/client-state.md | 5 ++-- docs/migrations/v6-to-v7.md | 4 +++- modules/core/02-client/keeper/client.go | 15 ++++-------- modules/core/02-client/keeper/client_test.go | 3 +++ modules/core/exported/client.go | 5 ++-- .../06-solomachine/client_state.go | 8 +++++-- .../06-solomachine/client_state_test.go | 9 ++++++-- .../07-tendermint/client_state.go | 14 +++++++---- .../07-tendermint/client_state_test.go | 23 ++++++++++++++----- 9 files changed, 55 insertions(+), 31 deletions(-) diff --git a/docs/ibc/light-clients/client-state.md b/docs/ibc/light-clients/client-state.md index fa32e90fe8a..024da207fbb 100644 --- a/docs/ibc/light-clients/client-state.md +++ b/docs/ibc/light-clients/client-state.md @@ -48,9 +48,10 @@ This value is used to facilitate timeouts by checking the packet timeout timesta ## `Initialize` method -Clients must validate the initial consensus state, and may store any client-specific metadata necessary. +Clients must validate the initial consensus state, and set the initial client state and consensus state in the provided client store. +Clients may also store any necessary client-specific metadata. -`Initialize` gets called when a [client is created](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/client.go#L32). +`Initialize` is called when a [client is created](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/client.go#L32). ## `VerifyMembership` method diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index 64cd0d54f28..6649b216f6e 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -106,7 +106,9 @@ The `VerifyUpgradeAndUpdateState` function has been modified. The client state a Light clients **must** handle all management of client and consensus states including the setting of updated client state and consensus state in the client store. -The `CheckHeaderAndUpdateState` function has been split into 4 new functions: +The `Initialize` method is now expected to set the initial client state, consensus state and any client-specific metadata in the provided store upon client creation. + +The `CheckHeaderAndUpdateState` method has been split into 4 new methods: - `VerifyClientMessage` verifies a `ClientMessage`. A `ClientMessage` could be a `Header`, `Misbehaviour`, or batch update. Calls to `CheckForMisbehaviour`, `UpdateState`, and `UpdateStateOnMisbehaviour` will assume that the content of the `ClientMessage` has been verified and can be trusted. An error should be returned if the `ClientMessage` fails to verify. diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 1e7bbe9c26a..a593c682fd9 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -10,8 +10,9 @@ import ( "github.com/cosmos/ibc-go/v6/modules/core/exported" ) -// CreateClient creates a new client state and populates it with a given consensus -// state as defined in https://github.com/cosmos/ibc/tree/master/spec/core/ics-002-client-semantics#create +// CreateClient generates a new client identifier and isolated prefix store for the provided client state. +// The client state is responsible for setting any client-specific data in the store via the Initialize method. +// This includes the client state, initial consensus state and any associated metadata. func (k Keeper) CreateClient( ctx sdk.Context, clientState exported.ClientState, consensusState exported.ConsensusState, ) (string, error) { @@ -24,18 +25,12 @@ func (k Keeper) CreateClient( } clientID := k.GenerateClientIdentifier(ctx, clientState.ClientType()) + clientStore := k.ClientStore(ctx, clientID) - k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String()) - - // verifies initial consensus state against client state and initializes client store with any client-specific metadata - // e.g. set ProcessedTime in Tendermint clients - if err := clientState.Initialize(ctx, k.cdc, k.ClientStore(ctx, clientID), consensusState); err != nil { + if err := clientState.Initialize(ctx, k.cdc, clientStore, consensusState); err != nil { return "", err } - k.SetClientState(ctx, clientID, clientState) - k.SetClientConsensusState(ctx, clientID, clientState.GetLatestHeight(), consensusState) - k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String()) defer telemetry.IncrCounterWithLabels( diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 12acddb4ff1..f20ba1dcdc1 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -10,6 +10,7 @@ import ( "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" commitmenttypes "github.com/cosmos/ibc-go/v6/modules/core/23-commitment/types" + host "github.com/cosmos/ibc-go/v6/modules/core/24-host" "github.com/cosmos/ibc-go/v6/modules/core/exported" solomachine "github.com/cosmos/ibc-go/v6/modules/light-clients/06-solomachine" ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" @@ -32,9 +33,11 @@ func (suite *KeeperTestSuite) TestCreateClient() { if tc.expPass { suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) suite.Require().NotNil(clientID, "valid test case %d failed: %s", i, tc.msg) + suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) } else { suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) suite.Require().Equal("", clientID, "invalid test case %d passed: %s", i, tc.msg) + suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) } } } diff --git a/modules/core/exported/client.go b/modules/core/exported/client.go index 81dea3373c5..3b2305beb71 100644 --- a/modules/core/exported/client.go +++ b/modules/core/exported/client.go @@ -59,9 +59,8 @@ type ClientState interface { height Height, ) (uint64, error) - // Initialization function - // Clients must validate the initial consensus state, and may store any client-specific metadata - // necessary for correct light client operation + // Initialize is called upon client creation, it allows the client to perform validation on the initial consensus state and set the + // client state, consensus state and any client-specific metadata necessary for correct light client operation in the provided client store. Initialize(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, consensusState ConsensusState) error // VerifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the specified height. diff --git a/modules/light-clients/06-solomachine/client_state.go b/modules/light-clients/06-solomachine/client_state.go index ed2bf259418..5148a35aa62 100644 --- a/modules/light-clients/06-solomachine/client_state.go +++ b/modules/light-clients/06-solomachine/client_state.go @@ -77,12 +77,16 @@ func (cs ClientState) ZeroCustomFields() exported.ClientState { panic("ZeroCustomFields is not implemented as the solo machine implementation does not support upgrades.") } -// Initialize will check that initial consensus state is equal to the latest consensus state of the initial client. -func (cs ClientState) Initialize(_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, consState exported.ConsensusState) error { +// Initialize checks that the initial consensus state is equal to the latest consensus state of the initial client and +// sets the client state in the provided client store. +func (cs ClientState) Initialize(_ sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, consState exported.ConsensusState) error { if !reflect.DeepEqual(cs.ConsensusState, consState) { return sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "consensus state in initial client does not equal initial consensus state. expected: %s, got: %s", cs.ConsensusState, consState) } + + setClientState(clientStore, cdc, &cs) + return nil } diff --git a/modules/light-clients/06-solomachine/client_state_test.go b/modules/light-clients/06-solomachine/client_state_test.go index e494a484ec8..dbb958ae7ba 100644 --- a/modules/light-clients/06-solomachine/client_state_test.go +++ b/modules/light-clients/06-solomachine/client_state_test.go @@ -8,6 +8,7 @@ import ( clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" commitmenttypes "github.com/cosmos/ibc-go/v6/modules/core/23-commitment/types" + host "github.com/cosmos/ibc-go/v6/modules/core/24-host" "github.com/cosmos/ibc-go/v6/modules/core/exported" solomachine "github.com/cosmos/ibc-go/v6/modules/light-clients/06-solomachine" ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" @@ -125,16 +126,20 @@ func (suite *SoloMachineTestSuite) TestInitialize() { } for _, tc := range testCases { + suite.SetupTest() + + store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), "solomachine") err := sm.ClientState().Initialize( suite.chainA.GetContext(), suite.chainA.Codec, - suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), "solomachine"), - tc.consState, + store, tc.consState, ) if tc.expPass { suite.Require().NoError(err, "valid testcase: %s failed", tc.name) + suite.Require().True(store.Has(host.ClientStateKey())) } else { suite.Require().Error(err, "invalid testcase: %s passed", tc.name) + suite.Require().False(store.Has(host.ClientStateKey())) } } } diff --git a/modules/light-clients/07-tendermint/client_state.go b/modules/light-clients/07-tendermint/client_state.go index 9f6da36bf00..cc6fbe9c482 100644 --- a/modules/light-clients/07-tendermint/client_state.go +++ b/modules/light-clients/07-tendermint/client_state.go @@ -188,15 +188,19 @@ func (cs ClientState) ZeroCustomFields() exported.ClientState { } } -// Initialize will check that initial consensus state is a Tendermint consensus state -// and will store ProcessedTime for initial consensus state as ctx.BlockTime() -func (cs ClientState) Initialize(ctx sdk.Context, _ codec.BinaryCodec, clientStore sdk.KVStore, consState exported.ConsensusState) error { - if _, ok := consState.(*ConsensusState); !ok { +// Initialize checks that the initial consensus state is an 07-tendermint consensus state and +// sets the client state, consensus state and associated metadata in the provided client store. +func (cs ClientState) Initialize(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, consState exported.ConsensusState) error { + consensusState, ok := consState.(*ConsensusState) + if !ok { return sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "invalid initial consensus state. expected type: %T, got: %T", &ConsensusState{}, consState) } - // set metadata for initial consensus state. + + setClientState(clientStore, cdc, &cs) + setConsensusState(clientStore, cdc, consensusState, cs.GetLatestHeight()) setConsensusMetadata(ctx, clientStore, cs.GetLatestHeight()) + return nil } diff --git a/modules/light-clients/07-tendermint/client_state_test.go b/modules/light-clients/07-tendermint/client_state_test.go index 64ffff0fd50..be7cccf2ce4 100644 --- a/modules/light-clients/07-tendermint/client_state_test.go +++ b/modules/light-clients/07-tendermint/client_state_test.go @@ -192,19 +192,30 @@ func (suite *TendermintTestSuite) TestInitialize() { }, } - path := ibctesting.NewPath(suite.chainA, suite.chainB) - err := path.EndpointA.CreateClient() - suite.Require().NoError(err) + for _, tc := range testCases { + suite.SetupTest() + path := ibctesting.NewPath(suite.chainA, suite.chainB) - clientState := suite.chainA.GetClientState(path.EndpointA.ClientID) - store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID) + tmConfig, ok := path.EndpointB.ClientConfig.(*ibctesting.TendermintConfig) + suite.Require().True(ok) - for _, tc := range testCases { + clientState := ibctm.NewClientState( + path.EndpointB.Chain.ChainID, + tmConfig.TrustLevel, tmConfig.TrustingPeriod, tmConfig.UnbondingPeriod, tmConfig.MaxClockDrift, + suite.chainB.LastHeader.GetTrustedHeight(), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, + ) + + store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID) err := clientState.Initialize(suite.chainA.GetContext(), suite.chainA.Codec, store, tc.consensusState) + if tc.expPass { suite.Require().NoError(err, "valid case returned an error") + suite.Require().True(store.Has(host.ClientStateKey())) + suite.Require().True(store.Has(host.ConsensusStateKey(suite.chainB.LastHeader.GetTrustedHeight()))) } else { suite.Require().Error(err, "invalid case didn't return an error") + suite.Require().False(store.Has(host.ClientStateKey())) + suite.Require().False(store.Has(host.ConsensusStateKey(suite.chainB.LastHeader.GetTrustedHeight()))) } } }