From 2122cc9613d67f8a5e01ca69c9d53c6bf9ec914d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 6 Dec 2021 15:39:27 +0100 Subject: [PATCH 1/4] modify VerifyUpgradeAndUpdateState interface function to remove returned client and consensus state removes the client and consensus state from the return values in VerifyUpgradeAndUpdateState client state interface function Updates light client implementations to set client and consensus state in client store Fixes and updates tests --- modules/core/02-client/keeper/client.go | 12 +++---- .../core/02-client/legacy/v100/solomachine.go | 2 +- modules/core/exported/client.go | 3 +- .../06-solomachine/types/client_state.go | 4 +-- .../07-tendermint/types/store.go | 7 +++++ .../07-tendermint/types/upgrade.go | 31 ++++++++++--------- .../07-tendermint/types/upgrade_test.go | 11 ++++--- .../09-localhost/types/client_state.go | 4 +-- 8 files changed, 40 insertions(+), 34 deletions(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index dbd626e5063..6000bc50263 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -165,23 +165,20 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot upgrade client (%s) with status %s", clientID, status) } - updatedClientState, updatedConsState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore, + err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore, upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState) if err != nil { return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID) } - k.SetClientState(ctx, clientID, updatedClientState) - k.SetClientConsensusState(ctx, clientID, updatedClientState.GetLatestHeight(), updatedConsState) - - k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", updatedClientState.GetLatestHeight().String()) + k.Logger(ctx).Info("client state upgraded", "client-id", clientID) defer func() { telemetry.IncrCounterWithLabels( []string{"ibc", "client", "upgrade"}, 1, []metrics.Label{ - telemetry.NewLabel(types.LabelClientType, updatedClientState.ClientType()), + telemetry.NewLabel(types.LabelClientType, upgradedClient.ClientType()), telemetry.NewLabel(types.LabelClientID, clientID), }, ) @@ -192,8 +189,7 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e sdk.NewEvent( types.EventTypeUpgradeClient, sdk.NewAttribute(types.AttributeKeyClientID, clientID), - sdk.NewAttribute(types.AttributeKeyClientType, updatedClientState.ClientType()), - sdk.NewAttribute(types.AttributeKeyConsensusHeight, updatedClientState.GetLatestHeight().String()), + sdk.NewAttribute(types.AttributeKeyClientType, upgradedClient.ClientType()), ), ) diff --git a/modules/core/02-client/legacy/v100/solomachine.go b/modules/core/02-client/legacy/v100/solomachine.go index b6c1142cab4..0a599cf0d95 100644 --- a/modules/core/02-client/legacy/v100/solomachine.go +++ b/modules/core/02-client/legacy/v100/solomachine.go @@ -114,7 +114,7 @@ func (cs ClientState) CheckSubstituteAndUpdateState( func (cs ClientState) VerifyUpgradeAndUpdateState( _ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientState, _ exported.ConsensusState, _, _ []byte, -) (exported.ClientState, exported.ConsensusState, error) { +) error { panic("legacy solo machine is deprecated!") } diff --git a/modules/core/exported/client.go b/modules/core/exported/client.go index de4cbe48c8a..e4f973d8809 100644 --- a/modules/core/exported/client.go +++ b/modules/core/exported/client.go @@ -70,6 +70,7 @@ type ClientState interface { // height of the current revision is somehow encoded in the proof verification process. // This is to ensure that no premature upgrades occur, since upgrade plans committed to by the counterparty // may be cancelled or modified before the last planned height. + // If the upgrade is verified, the upgraded client and consensus states must be set in the client store. VerifyUpgradeAndUpdateState( ctx sdk.Context, cdc codec.BinaryCodec, @@ -78,7 +79,7 @@ type ClientState interface { newConsState ConsensusState, proofUpgradeClient, proofUpgradeConsState []byte, - ) (ClientState, ConsensusState, error) + ) error // 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 diff --git a/modules/light-clients/06-solomachine/types/client_state.go b/modules/light-clients/06-solomachine/types/client_state.go index 6835f09ce50..a31e1f9b3b9 100644 --- a/modules/light-clients/06-solomachine/types/client_state.go +++ b/modules/light-clients/06-solomachine/types/client_state.go @@ -94,8 +94,8 @@ func (cs ClientState) ExportMetadata(_ sdk.KVStore) []exported.GenesisMetadata { func (cs ClientState) VerifyUpgradeAndUpdateState( _ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientState, _ exported.ConsensusState, _, _ []byte, -) (exported.ClientState, exported.ConsensusState, error) { - return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client") +) error { + return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client") } // VerifyClientState verifies a proof of the client state of the running chain diff --git a/modules/light-clients/07-tendermint/types/store.go b/modules/light-clients/07-tendermint/types/store.go index 6ee21abfc28..73655123a84 100644 --- a/modules/light-clients/07-tendermint/types/store.go +++ b/modules/light-clients/07-tendermint/types/store.go @@ -42,6 +42,13 @@ var ( KeyIteration = []byte("/iterationKey") ) +// setClientState stores the client state +func setClientState(clientStore sdk.KVStore, cdc codec.BinaryCodec, clientState *ClientState) { + key := host.ClientStateKey() + val := clienttypes.MustMarshalClientState(cdc, clientState) + clientStore.Set(key, val) +} + // SetConsensusState stores the consensus state at the given height. func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensusState *ConsensusState, height exported.Height) { key := host.ConsensusStateKey(height) diff --git a/modules/light-clients/07-tendermint/types/upgrade.go b/modules/light-clients/07-tendermint/types/upgrade.go index 73cc1360538..216b43851e4 100644 --- a/modules/light-clients/07-tendermint/types/upgrade.go +++ b/modules/light-clients/07-tendermint/types/upgrade.go @@ -27,16 +27,16 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, upgradedClient exported.ClientState, upgradedConsState exported.ConsensusState, proofUpgradeClient, proofUpgradeConsState []byte, -) (exported.ClientState, exported.ConsensusState, error) { +) error { if len(cs.UpgradePath) == 0 { - return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set") + return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set") } // last height of current counterparty chain must be client's latest height lastHeight := cs.GetLatestHeight() if !upgradedClient.GetLatestHeight().GT(lastHeight) { - return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s", + return sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s", upgradedClient.GetLatestHeight(), lastHeight) } @@ -45,22 +45,22 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( // counterparty must also commit to the upgraded consensus state at a sub-path under the upgrade path specified tmUpgradeClient, ok := upgradedClient.(*ClientState) if !ok { - return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T", + return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T", &ClientState{}, upgradedClient) } tmUpgradeConsState, ok := upgradedConsState.(*ConsensusState) if !ok { - return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "upgraded consensus state must be Tendermint consensus state. expected %T, got: %T", + return sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "upgraded consensus state must be Tendermint consensus state. expected %T, got: %T", &ConsensusState{}, upgradedConsState) } // unmarshal proofs var merkleProofClient, merkleProofConsState commitmenttypes.MerkleProof if err := cdc.Unmarshal(proofUpgradeClient, &merkleProofClient); err != nil { - return nil, nil, sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal client merkle proof: %v", err) + return sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal client merkle proof: %v", err) } if err := cdc.Unmarshal(proofUpgradeConsState, &merkleProofConsState); err != nil { - return nil, nil, sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal consensus state merkle proof: %v", err) + return sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal consensus state merkle proof: %v", err) } // Must prove against latest consensus state to ensure we are verifying against latest upgrade plan @@ -68,29 +68,29 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( // at this consensus state consState, err := GetConsensusState(clientStore, cdc, lastHeight) if err != nil { - return nil, nil, sdkerrors.Wrap(err, "could not retrieve consensus state for lastHeight") + return sdkerrors.Wrap(err, "could not retrieve consensus state for lastHeight") } // Verify client proof bz, err := cdc.MarshalInterface(upgradedClient) if err != nil { - return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err) + return sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err) } // construct clientState Merkle path upgradeClientPath := constructUpgradeClientMerklePath(cs.UpgradePath, lastHeight) if err := merkleProofClient.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeClientPath, bz); err != nil { - return nil, nil, sdkerrors.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.Pretty()) + return sdkerrors.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.Pretty()) } // Verify consensus state proof bz, err = cdc.MarshalInterface(upgradedConsState) if err != nil { - return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "could not marshal consensus state: %v", err) + return sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "could not marshal consensus state: %v", err) } // construct consensus state Merkle path upgradeConsStatePath := constructUpgradeConsStateMerklePath(cs.UpgradePath, lastHeight) if err := merkleProofConsState.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeConsStatePath, bz); err != nil { - return nil, nil, sdkerrors.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.Pretty()) + return sdkerrors.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.Pretty()) } // Construct new client state and consensus state @@ -104,7 +104,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( ) if err := newClientState.Validate(); err != nil { - return nil, nil, sdkerrors.Wrap(err, "updated client state failed basic validation") + return sdkerrors.Wrap(err, "updated client state failed basic validation") } // The new consensus state is merely used as a trusted kernel against which headers on the new @@ -118,10 +118,11 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( tmUpgradeConsState.Timestamp, commitmenttypes.NewMerkleRoot([]byte(SentinelRoot)), tmUpgradeConsState.NextValidatorsHash, ) - // set metadata for this consensus state + setClientState(clientStore, cdc, newClientState) + SetConsensusState(clientStore, cdc, newConsState, newClientState.LatestHeight) setConsensusMetadata(ctx, clientStore, tmUpgradeClient.LatestHeight) - return newClientState, newConsState, nil + return nil } // construct MerklePath for the committed client from upgradePath diff --git a/modules/light-clients/07-tendermint/types/upgrade_test.go b/modules/light-clients/07-tendermint/types/upgrade_test.go index 9a89e8fafaf..fb0010f422d 100644 --- a/modules/light-clients/07-tendermint/types/upgrade_test.go +++ b/modules/light-clients/07-tendermint/types/upgrade_test.go @@ -457,7 +457,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { // Call ZeroCustomFields on upgraded clients to clear any client-chosen parameters in test-case upgradedClient upgradedClient = upgradedClient.ZeroCustomFields() - clientState, consensusState, err := cs.VerifyUpgradeAndUpdateState( + err := cs.VerifyUpgradeAndUpdateState( suite.chainA.GetContext(), suite.cdc, clientStore, @@ -469,14 +469,15 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { if tc.expPass { suite.Require().NoError(err, "verify upgrade failed on valid case: %s", tc.name) + + clientState := suite.chainA.GetClientState(path.EndpointA.ClientID) suite.Require().NotNil(clientState, "verify upgrade failed on valid case: %s", tc.name) + + consensusState, found := suite.chainA.GetConsensusState(path.EndpointA.ClientID, clientState.GetLatestHeight()) suite.Require().NotNil(consensusState, "verify upgrade failed on valid case: %s", tc.name) + suite.Require().True(found) } else { suite.Require().Error(err, "verify upgrade passed on invalid case: %s", tc.name) - suite.Require().Nil(clientState, "verify upgrade passed on invalid case: %s", tc.name) - - suite.Require().Nil(consensusState, "verify upgrade passed on invalid case: %s", tc.name) - } } } diff --git a/modules/light-clients/09-localhost/types/client_state.go b/modules/light-clients/09-localhost/types/client_state.go index 499f5f9389c..d4f9d9059e3 100644 --- a/modules/light-clients/09-localhost/types/client_state.go +++ b/modules/light-clients/09-localhost/types/client_state.go @@ -116,8 +116,8 @@ func (cs ClientState) CheckSubstituteAndUpdateState( func (cs ClientState) VerifyUpgradeAndUpdateState( _ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientState, _ exported.ConsensusState, _, _ []byte, -) (exported.ClientState, exported.ConsensusState, error) { - return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade localhost client") +) error { + return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade localhost client") } // VerifyClientState verifies that the localhost client state is stored locally From d1111263ab6450e19bff723ebd910f40c41145cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Mar 2022 13:06:36 +0100 Subject: [PATCH 2/4] add changelog entry --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c42454c3ed..39533319576 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (core) [\#709](https://github.com/cosmos/ibc-go/pull/709) Replace github.com/pkg/errors with stdlib errors ### API Breaking - + +* (02-client) [\#598](https://github.com/cosmos/ibc-go/pull/598) The client state and consensus state return value has been removed from `VerifyUpgradeAndUpdateState`. Light client implementations must update the client state and consensus state after verifying a valid client upgrade. * (channel( [\#848](https://github.com/cosmos/ibc-go/pull/848) Added `ChannelId` to MsgChannelOpenInitResponse * (testing( [\#813](https://github.com/cosmos/ibc-go/pull/813) The `ack` argument to the testing function `RelayPacket` has been removed as it is no longer needed. * (testing) [\#774](https://github.com/cosmos/ibc-go/pull/774) Added `ChainID` arg to `SetupWithGenesisValSet` on the testing app. `Coordinator` generated ChainIDs now starts at index 1 From 83c5f6981e890114d7bcbb001a31281b25f98222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Mar 2022 13:07:44 +0100 Subject: [PATCH 3/4] add migration docs --- docs/migrations/v3-to-v4.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index 90e9af256d2..f933a7380f7 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -23,4 +23,8 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`. +## IBC Light Clients +The `VerifyUpgradeAndUpdateState` function has been modified. The client state and consensus state return value has been removed. + +Light clients **must** set the updated client state and consensus state in the client store after verifying a valid client upgrade. From 12f50f5b12bd70f107bfcfeeaadb3d1b0c5a92b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 9 Mar 2022 11:51:18 +0100 Subject: [PATCH 4/4] use upgraded client to emit height in events --- modules/core/02-client/keeper/client.go | 10 +++++----- modules/core/02-client/keeper/events.go | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 9bbd41ca429..26a6216b76b 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -158,13 +158,13 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot upgrade client (%s) with status %s", clientID, status) } - err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore, - upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState) - if err != nil { + if err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore, + upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState, + ); err != nil { return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID) } - k.Logger(ctx).Info("client state upgraded", "client-id", clientID) + k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", upgradedClient.GetLatestHeight().String()) defer func() { telemetry.IncrCounterWithLabels( @@ -177,7 +177,7 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e ) }() - EmitUpgradeClientEvent(ctx, clientID, upgradedClient.ClientType()) + EmitUpgradeClientEvent(ctx, clientID, upgradedClient) return nil } diff --git a/modules/core/02-client/keeper/events.go b/modules/core/02-client/keeper/events.go index e71faf5f821..ff8ae1c3acd 100644 --- a/modules/core/02-client/keeper/events.go +++ b/modules/core/02-client/keeper/events.go @@ -41,12 +41,13 @@ func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientState exporte } // EmitUpdateClientEvent emits an upgrade client event -func EmitUpgradeClientEvent(ctx sdk.Context, clientID string, clientType string) { +func EmitUpgradeClientEvent(ctx sdk.Context, clientID string, clientState exported.ClientState) { ctx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( types.EventTypeUpgradeClient, sdk.NewAttribute(types.AttributeKeyClientID, clientID), - sdk.NewAttribute(types.AttributeKeyClientType, clientType), + sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()), + sdk.NewAttribute(types.AttributeKeyConsensusHeight, clientState.GetLatestHeight().String()), ), sdk.NewEvent( sdk.EventTypeMessage,