refactor!: remove ZeroCustomFields from ClientState interface (cosmos…

…#5830) * refactor: remove ZeroCustomFields from ClientState interface * docs: add migration doc entry * fix: build * Update modules/light-clients/07-tendermint/client_state.go Co-authored-by: Damian Nolan <[email protected]> * Update modules/core/02-client/keeper/keeper.go Co-authored-by: Carlos Rodriguez <[email protected]> --------- Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
notional-labs · Feb 14, 2024 · 824b823 · 824b823
1 parent b68ba7d
commit 824b823
Show file tree

Hide file tree

Showing 18 changed files with 28 additions and 74 deletions.
diff --git a/docs/docs/03-light-clients/01-developer-guide/02-client-state.md b/docs/docs/03-light-clients/01-developer-guide/02-client-state.md
@@ -37,13 +37,6 @@ All possible `Status` types can be found [here](https://github.com/cosmos/ibc-go
 
 This field is returned in the response of the gRPC [`ibc.core.client.v1.Query/ClientStatus`](https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/02-client/types/query.pb.go#L665) endpoint.
 
-## `ZeroCustomFields` method
-
-`ZeroCustomFields` should return a copy of the light client with all client customizable fields with their zero value. It should not mutate the fields of the light client.
-This method is used when [scheduling upgrades](https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/02-client/keeper/proposal.go#L82). Upgrades are used to upgrade chain specific fields.
-In the tendermint case, this may be the chain ID or the unbonding period.
-For more information about client upgrades see the [Handling upgrades](05-upgrades.md) section.
-
 ## `GetTimestampAtHeight` method
 
 `GetTimestampAtHeight` must return the timestamp for the consensus state associated with the provided height.

diff --git a/docs/docs/03-light-clients/01-developer-guide/05-upgrades.md b/docs/docs/03-light-clients/01-developer-guide/05-upgrades.md
@@ -42,24 +42,17 @@ Clients should have **prior knowledge of the merkle path** that the upgraded cli
 
 ## Chain specific vs client specific client parameters
 
-Developers should maintain the distinction between client parameters that are uniform across every valid light client of a chain (chain-chosen parameters), and client parameters that are customizable by each individual client (client-chosen parameters); since this distinction is necessary to implement the `ZeroCustomFields` method in the [`ClientState` interface](02-client-state.md):
+Developers should maintain the distinction between client parameters that are uniform across every valid light client of a chain (chain-chosen parameters), and client parameters that are customizable by each individual client (client-chosen parameters).
 
-```go
-// Utility function that zeroes out any client customizable fields in client state
-// Ledger enforced fields are maintained while all custom fields are zero values
-// Used to verify upgrades
-func (cs ClientState) ZeroCustomFields() ClientState
-```
-
-Developers must ensure that the new client adopts all of the new client parameters that must be uniform across every valid light client of a chain (chain-chosen parameters), while maintaining the client parameters that are customizable by each individual client (client-chosen parameters) from the previous version of the client. `ZeroCustomFields` is a useful utility function to ensure only chain specific fields are updated during upgrades.
+When upgrading a client, developers must ensure that the new client adopts all of the new client parameters that must be uniform across every valid light client of a chain (chain-chosen parameters), while maintaining the client parameters that are customizable by each individual client (client-chosen parameters) from the previous version of the client.
 
 ## Security
 
 Upgrades must adhere to the IBC Security Model. IBC does not rely on the assumption of honest relayers for correctness. Thus users should not have to rely on relayers to maintain client correctness and security (though honest relayers must exist to maintain relayer liveness). While relayers may choose any set of client parameters while creating a new `ClientState`, this still holds under the security model since users can always choose a relayer-created client that suits their security and correctness needs or create a client with their desired parameters if no such client exists.
 
 However, when upgrading an existing client, one must keep in mind that there are already many users who depend on this client's particular parameters. **We cannot give the upgrading relayer free choice over these parameters once they have already been chosen. This would violate the security model** since users who rely on the client would have to rely on the upgrading relayer to maintain the same level of security.
 
-Thus, developers must make sure that their upgrade mechanism allows clients to upgrade the chain-specified parameters whenever a chain upgrade changes these parameters (examples in the Tendermint client include `UnbondingPeriod`, `TrustingPeriod`, `ChainID`, `UpgradePath`, etc), while ensuring that the relayer submitting the `MsgUpgradeClient` cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include `TrustLevel`, `MaxClockDrift`, etc). The previous paragraph discusses how `ZeroCustomFields` helps achieve this.
+Thus, developers must make sure that their upgrade mechanism allows clients to upgrade the chain-specified parameters whenever a chain upgrade changes these parameters (examples in the Tendermint client include `UnbondingPeriod`, `TrustingPeriod`, `ChainID`, `UpgradePath`, etc), while ensuring that the relayer submitting the `MsgUpgradeClient` cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include `TrustLevel`, `MaxClockDrift`, etc). 
 
 ### Document potential client parameter conflicts during upgrades
 

diff --git a/docs/docs/05-migrations/13-v8-to-v9.md b/docs/docs/05-migrations/13-v8-to-v9.md
@@ -50,4 +50,6 @@ Please use the new functions `path.Setup`, `path.SetupClients`, `path.SetupConne
 
 ## IBC Light Clients
 
-- No relevant changes were made in this release.
+### API removals
+
+The `ZeroCustomFields` interface function has been removed from the `ClientState` interface. Core IBC only used this function to set tendermint client states when scheduling an IBC software upgrade. The interface function has been replaced by a type assertion.  
diff --git a/e2e/tests/core/02-client/client_test.go b/e2e/tests/core/02-client/client_test.go
@@ -147,7 +147,7 @@ func (s *ClientTestSuite) TestScheduleIBCUpgrade_Succeeds() {
 		s.Require().NoError(err)
 		s.Require().NotEqual(originalChainID, newChainID)
 
-		upgradedClientState := clientState.ZeroCustomFields().(*ibctm.ClientState)
+		upgradedClientState := clientState.(*ibctm.ClientState).ZeroCustomFields()
 		upgradedClientState.ChainId = newChainID
 
 		legacyUpgradeProposal, err := clienttypes.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, upgradetypes.Plan{

diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
@@ -310,7 +310,7 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 func (suite *KeeperTestSuite) TestUpgradeClient() {
 	var (
 		path                                             *ibctesting.Path
-		upgradedClient                                   exported.ClientState
+		upgradedClient                                   *ibctm.ClientState
 		upgradedConsState                                exported.ConsensusState
 		lastHeight                                       exported.Height
 		upgradedClientProof, upgradedConsensusStateProof []byte
@@ -422,9 +422,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 				suite.Require().NoError(err)
 
 				// change upgradedClient client-specified parameters
-				tmClient := upgradedClient.(*ibctm.ClientState)
-				tmClient.ChainId = "wrongchainID"
-				upgradedClient = tmClient
+				upgradedClient.ChainId = "wrongchainID"
 
 				suite.coordinator.CommitBlock(suite.chainB)
 				err = path.EndpointA.UpdateClient()
@@ -451,9 +449,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 				suite.Require().NoError(err)
 
 				// change upgradedClient height to be lower than current client state height
-				tmClient := upgradedClient.(*ibctm.ClientState)
-				tmClient.LatestHeight = clienttypes.NewHeight(0, 1)
-				upgradedClient = tmClient
+				upgradedClient.LatestHeight = clienttypes.NewHeight(0, 1)
 
 				suite.coordinator.CommitBlock(suite.chainB)
 				err = path.EndpointA.UpdateClient()

diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go
@@ -439,7 +439,12 @@ func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
 // ScheduleIBCSoftwareUpgrade schedules an upgrade for the IBC client.
 func (k Keeper) ScheduleIBCSoftwareUpgrade(ctx sdk.Context, plan upgradetypes.Plan, upgradedClientState exported.ClientState) error {
 	// zero out any custom fields before setting
-	cs := upgradedClientState.ZeroCustomFields()
+	cs, ok := upgradedClientState.(*ibctm.ClientState)
+	if !ok {
+		return errorsmod.Wrapf(types.ErrInvalidClientType, "expected: %T, got: %T", &ibctm.ClientState{}, upgradedClientState)
+	}
+
+	cs = cs.ZeroCustomFields()
 	bz, err := types.MarshalClientState(k.cdc, cs)
 	if err != nil {
 		return errorsmod.Wrap(err, "could not marshal UpgradedClientState")

diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go
@@ -531,7 +531,7 @@ func (suite *KeeperTestSuite) TestIBCSoftwareUpgrade() {
 
 			path := ibctesting.NewPath(suite.chainA, suite.chainB)
 			path.SetupClients()
-			upgradedClientState = suite.chainA.GetClientState(path.EndpointA.ClientID).ZeroCustomFields().(*ibctm.ClientState)
+			upgradedClientState = path.EndpointA.GetClientState().(*ibctm.ClientState).ZeroCustomFields()
 
 			// use height 1000 to distinguish from old plan
 			plan = upgradetypes.Plan{

diff --git a/modules/core/02-client/migrations/v7/solomachine.go b/modules/core/02-client/migrations/v7/solomachine.go
@@ -72,11 +72,6 @@ func (ClientState) Validate() error {
 	panic(errors.New("legacy solo machine is deprecated"))
 }
 
-// ZeroCustomFields panics!
-func (ClientState) ZeroCustomFields() exported.ClientState {
-	panic(errors.New("legacy solo machine is deprecated"))
-}
-
 // Initialize panics!
 func (ClientState) Initialize(_ sdk.Context, _ codec.BinaryCodec, _ storetypes.KVStore, _ exported.ConsensusState) error {
 	panic(errors.New("legacy solo machine is deprecated"))

diff --git a/modules/core/02-client/types/legacy_proposal.go b/modules/core/02-client/types/legacy_proposal.go
@@ -4,7 +4,6 @@ package types
 
 import (
 	"fmt"
-	"reflect"
 
 	errorsmod "cosmossdk.io/errors"
 	upgradetypes "cosmossdk.io/x/upgrade/types"
@@ -116,15 +115,11 @@ func (up *UpgradeProposal) ValidateBasic() error {
 		return errorsmod.Wrap(ErrInvalidUpgradeProposal, "upgraded client state cannot be nil")
 	}
 
-	clientState, err := UnpackClientState(up.UpgradedClientState)
+	_, err := UnpackClientState(up.UpgradedClientState)
 	if err != nil {
 		return errorsmod.Wrap(err, "failed to unpack upgraded client state")
 	}
 
-	if !reflect.DeepEqual(clientState, clientState.ZeroCustomFields()) {
-		return errorsmod.Wrap(ErrInvalidUpgradeProposal, "upgraded client state is not zeroed out")
-	}
-
 	return nil
 }
 

diff --git a/modules/core/exported/client.go b/modules/core/exported/client.go
@@ -58,11 +58,6 @@ type ClientState interface {
 	// ExportMetadata must export metadata stored within the clientStore for genesis export
 	ExportMetadata(clientStore storetypes.KVStore) []GenesisMetadata
 
-	// ZeroCustomFields zeroes out any client customizable fields in client state
-	// Ledger enforced fields are maintained while all custom fields are zero values
-	// Used to verify upgrades
-	ZeroCustomFields() ClientState
-
 	// GetTimestampAtHeight must return the timestamp for the consensus state associated with the provided height.
 	GetTimestampAtHeight(
 		ctx sdk.Context,

diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go
@@ -781,7 +781,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 		path              *ibctesting.Path
 		newChainID        string
 		newClientHeight   clienttypes.Height
-		upgradedClient    exported.ClientState
+		upgradedClient    *ibctm.ClientState
 		upgradedConsState exported.ConsensusState
 		lastHeight        exported.Height
 		msg               *clienttypes.MsgUpgradeClient
@@ -889,7 +889,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			suite.Require().NoError(err, "upgrade handler failed on valid case: %s", tc.name)
 			newClient, ok := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
 			suite.Require().True(ok)
-			newChainSpecifiedClient := newClient.ZeroCustomFields()
+			newChainSpecifiedClient := newClient.(*ibctm.ClientState).ZeroCustomFields()
 			suite.Require().Equal(upgradedClient, newChainSpecifiedClient)
 
 			expectedEvents := sdk.Events{
@@ -2577,8 +2577,8 @@ func (suite *KeeperTestSuite) TestIBCSoftwareUpgrade() {
 				Height: 1000,
 			}
 			// update trusting period
-			clientState := path.EndpointB.GetClientState()
-			clientState.(*ibctm.ClientState).TrustingPeriod += 100
+			clientState := path.EndpointB.GetClientState().(*ibctm.ClientState)
+			clientState.TrustingPeriod += 100
 
 			var err error
 			msg, err = clienttypes.NewMsgIBCSoftwareUpgrade(

diff --git a/modules/light-clients/06-solomachine/client_state.go b/modules/light-clients/06-solomachine/client_state.go
@@ -1,7 +1,6 @@
 package solomachine
 
 import (
-	"errors"
 	"reflect"
 
 	errorsmod "cosmossdk.io/errors"
@@ -75,11 +74,6 @@ func (cs ClientState) Validate() error {
 	return cs.ConsensusState.ValidateBasic()
 }
 
-// ZeroCustomFields is not implemented for solo machine
-func (ClientState) ZeroCustomFields() exported.ClientState {
-	panic(errors.New("ZeroCustomFields is not implemented as the solo machine implementation does not support upgrades"))
-}
-
 // 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 storetypes.KVStore, consState exported.ConsensusState) error {

diff --git a/modules/light-clients/07-tendermint/client_state.go b/modules/light-clients/07-tendermint/client_state.go
@@ -174,8 +174,10 @@ func (cs ClientState) Validate() error {
 }
 
 // ZeroCustomFields returns a ClientState that is a copy of the current ClientState
-// with all client customizable fields zeroed out
-func (cs ClientState) ZeroCustomFields() exported.ClientState {
+// with all client customizable fields zeroed out. All chain specific fields must
+// remain unchanged. This client state will be used to verify chain upgrades when a
+// chain breaks a light client verification parameter such as chainID.
+func (cs ClientState) ZeroCustomFields() *ClientState {
 	// copy over all chain-specified fields
 	// and leave custom fields empty
 	return &ClientState{

diff --git a/modules/light-clients/07-tendermint/upgrade.go b/modules/light-clients/07-tendermint/upgrade.go
@@ -70,7 +70,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 	}
 
 	// Verify client proof
-	bz, err := cdc.MarshalInterface(upgradedClient.ZeroCustomFields())
+	bz, err := cdc.MarshalInterface(tmUpgradeClient.ZeroCustomFields())
 	if err != nil {
 		return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err)
 	}

diff --git a/modules/light-clients/07-tendermint/upgrade_test.go b/modules/light-clients/07-tendermint/upgrade_test.go
@@ -13,7 +13,7 @@ import (
 func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 	var (
 		newChainID                                       string
-		upgradedClient                                   exported.ClientState
+		upgradedClient                                   *ibctm.ClientState
 		upgradedConsState                                exported.ConsensusState
 		lastHeight                                       clienttypes.Height
 		path                                             *ibctesting.Path

diff --git a/modules/light-clients/08-wasm/types/client_state.go b/modules/light-clients/08-wasm/types/client_state.go
@@ -73,12 +73,6 @@ func (cs ClientState) Status(ctx sdk.Context, clientStore storetypes.KVStore, _
 	return exported.Status(result.Status)
 }
 
-// ZeroCustomFields returns a ClientState that is a copy of the current ClientState
-// with all client customizable fields zeroed out
-func (cs ClientState) ZeroCustomFields() exported.ClientState {
-	return &cs
-}
-
 // GetTimestampAtHeight returns the timestamp in nanoseconds of the consensus state at the given height.
 func (cs ClientState) GetTimestampAtHeight(
 	ctx sdk.Context,

diff --git a/modules/light-clients/09-localhost/client_state.go b/modules/light-clients/09-localhost/client_state.go
@@ -49,11 +49,6 @@ func (cs ClientState) Validate() error {
 	return nil
 }
 
-// ZeroCustomFields returns the same client state since there are no custom fields in the 09-localhost client state.
-func (cs ClientState) ZeroCustomFields() exported.ClientState {
-	return &cs
-}
-
 // Initialize ensures that initial consensus state for localhost is nil.
 func (ClientState) Initialize(ctx sdk.Context, cdc codec.BinaryCodec, clientStore storetypes.KVStore, consState exported.ConsensusState) error {
 	if consState != nil {

diff --git a/modules/light-clients/09-localhost/client_state_test.go b/modules/light-clients/09-localhost/client_state_test.go
@@ -31,11 +31,6 @@ func (suite *LocalhostTestSuite) TestGetLatestHeight() {
 	suite.Require().Equal(expectedHeight, clientState.GetLatestHeight())
 }
 
-func (suite *LocalhostTestSuite) TestZeroCustomFields() {
-	clientState := localhost.NewClientState(clienttypes.NewHeight(1, 10))
-	suite.Require().Equal(clientState, clientState.ZeroCustomFields())
-}
-
 func (suite *LocalhostTestSuite) TestGetTimestampAtHeight() {
 	ctx := suite.chain.GetContext()
 	clientState := localhost.NewClientState(clienttypes.NewHeight(1, 10))