From 824b823948fc0235dbb4e2327a51153a47111ec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 14 Feb 2024 16:00:45 +0100 Subject: [PATCH] refactor!: remove ZeroCustomFields from ClientState interface (#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 * Update modules/core/02-client/keeper/keeper.go Co-authored-by: Carlos Rodriguez --------- Co-authored-by: Damian Nolan Co-authored-by: Carlos Rodriguez --- .../01-developer-guide/02-client-state.md | 7 ------- .../01-developer-guide/05-upgrades.md | 13 +++---------- docs/docs/05-migrations/13-v8-to-v9.md | 4 +++- e2e/tests/core/02-client/client_test.go | 2 +- modules/core/02-client/keeper/client_test.go | 10 +++------- modules/core/02-client/keeper/keeper.go | 7 ++++++- modules/core/02-client/keeper/keeper_test.go | 2 +- modules/core/02-client/migrations/v7/solomachine.go | 5 ----- modules/core/02-client/types/legacy_proposal.go | 7 +------ modules/core/exported/client.go | 5 ----- modules/core/keeper/msg_server_test.go | 8 ++++---- .../light-clients/06-solomachine/client_state.go | 6 ------ modules/light-clients/07-tendermint/client_state.go | 6 ++++-- modules/light-clients/07-tendermint/upgrade.go | 2 +- modules/light-clients/07-tendermint/upgrade_test.go | 2 +- modules/light-clients/08-wasm/types/client_state.go | 6 ------ modules/light-clients/09-localhost/client_state.go | 5 ----- .../light-clients/09-localhost/client_state_test.go | 5 ----- 18 files changed, 28 insertions(+), 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 index cceb32637fd..cad1982fc97 100644 --- 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 index 2e0b0baf578..10eb3ed5ce4 100644 --- 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,16 +42,9 @@ 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 @@ -59,7 +52,7 @@ Upgrades must adhere to the IBC Security Model. IBC does not rely on the assumpt 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 index 8f5b8754f77..ee4657da395 100644 --- 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 index c247d3399b3..784ced70e4f 100644 --- 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 index 4b924664563..4cf12e21ad8 100644 --- 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 index 491006c8a63..009910adff5 100644 --- 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 index 8f1680bf5c3..c8d217edc6f 100644 --- 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 index 4f0e219097a..bdee6ff6494 100644 --- 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 index 86629994d25..3ce2e0a0aed 100644 --- 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 index fec1564c4c3..de109efc3eb 100644 --- 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 index 39ede4366e7..1c865aaf274 100644 --- 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 index 1794bc442d3..051cd81507c 100644 --- 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 index c5c70cdf1a8..826b7554a0b 100644 --- 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 index beccdf0a346..300023040da 100644 --- 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 index 4cdca645e2a..290fcae730a 100644 --- 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 index 1764df16d5a..cced48113d1 100644 --- 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 index 89c9b4a8c28..d30cad5c34d 100644 --- 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 index 7764289768d..c9618da5412 100644 --- 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))