refactor: modify VerifyUpgradeAndUpdateState to set upgraded client and consensus state

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

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

@@ -110,8 +110,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