refactor: require light clients to set the initial client state and c…

…onsensus state via the client state `Initialize` method (cosmos#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 <charly@interchain.io>

* Update docs/ibc/light-clients/client-state.md

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* adding tests to lightclients

* updating 02-client tests

Co-authored-by: Charly <charly@interchain.io>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

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
 

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.
 

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(

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

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.

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
 }
 

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

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
 }
 

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