diff --git a/CHANGELOG.md b/CHANGELOG.md index 929183fb2fe..02777a689bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,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. * (testing) [\#939](https://github.com/cosmos/ibc-go/pull/939) Support custom power reduction for testing. * (modules/core/05-port) [\#1086](https://github.com/cosmos/ibc-go/pull/1086) Added `counterpartyChannelID` argument to IBCModule.OnChanOpenAck * (06-solomachine) [\#1100](https://github.com/cosmos/ibc-go/pull/1100) Remove `GetClientID` function from 06-solomachine `Misbehaviour` type. 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. diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 78086ddb4d6..bfadcda026e 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -158,30 +158,26 @@ 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, - 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.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, "height", upgradedClient.GetLatestHeight().String()) 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), }, ) }() - // emitting events in the keeper emits for client upgrades - EmitUpgradeClientEvent(ctx, clientID, updatedClientState) + EmitUpgradeClientEvent(ctx, clientID, upgradedClient) return nil } diff --git a/modules/core/02-client/legacy/v100/solomachine.go b/modules/core/02-client/legacy/v100/solomachine.go index e16ba5b60ac..c9814439902 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 bb473468041..39095aff28f 100644 --- a/modules/core/exported/client.go +++ b/modules/core/exported/client.go @@ -68,6 +68,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, @@ -76,7 +77,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 d92f69b98e4..ef3088b314f 100644 --- a/modules/light-clients/06-solomachine/types/client_state.go +++ b/modules/light-clients/06-solomachine/types/client_state.go @@ -88,8 +88,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 785ed77ba97..33c2386bb78 100644 --- a/modules/light-clients/07-tendermint/types/store.go +++ b/modules/light-clients/07-tendermint/types/store.go @@ -43,6 +43,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 5e23c8d9036..471769f0610 100644 --- a/modules/light-clients/07-tendermint/types/upgrade.go +++ b/modules/light-clients/07-tendermint/types/upgrade.go @@ -28,16 +28,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) } @@ -46,22 +46,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 @@ -69,29 +69,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 @@ -105,7 +105,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 @@ -119,10 +119,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 175ce7fc358..112d3366cda 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 e818f9c8d7e..728e4ec5f12 100644 --- a/modules/light-clients/09-localhost/types/client_state.go +++ b/modules/light-clients/09-localhost/types/client_state.go @@ -138,8 +138,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