refactor: modify VerifyUpgradeAndUpdateState to set upgraded client a…

…nd consensus state (#598)

* 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

* add changelog entry

* add migration docs

* use upgraded client to emit height in events

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.

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.

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
 }

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!")
 }
 

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

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

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)

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,52 +46,52 @@ 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
 	// This verifies that upgrade is intended for the provided revision, since committed client must exist
 	// 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

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

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