IBC Upgrades function interface return client/consensus (#7895)

* return client and consensus state on upgrades

* fix build

x/ibc/core/02-client/keeper/client.go

@@ -118,21 +118,22 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
-	err := clientState.VerifyUpgrade(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade)
+	updatedClientState, updatedConsensusState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID)
 	}
 
-	k.SetClientState(ctx, clientID, upgradedClient)
+	k.SetClientState(ctx, clientID, updatedClientState)
+	k.SetClientConsensusState(ctx, clientID, updatedClientState.GetLatestHeight(), updatedConsensusState)
 
-	k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", upgradedClient.GetLatestHeight().String())
+	k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", updatedClientState.GetLatestHeight().String())
 
 	defer func() {
 		telemetry.IncrCounterWithLabels(
 			[]string{"ibc", "client", "upgrade"},
 			1,
 			[]metrics.Label{
-				telemetry.NewLabel("client-type", clientState.ClientType()),
+				telemetry.NewLabel("client-type", updatedClientState.ClientType()),
 				telemetry.NewLabel("client-id", clientID),
 			},
 		)
@@ -143,8 +144,8 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 		sdk.NewEvent(
 			types.EventTypeUpgradeClient,
 			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
-			sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, upgradedClient.GetLatestHeight().String()),
+			sdk.NewAttribute(types.AttributeKeyClientType, updatedClientState.ClientType()),
+			sdk.NewAttribute(types.AttributeKeyConsensusHeight, updatedClientState.GetLatestHeight().String()),
 		),
 	)
 

x/ibc/core/exported/client.go

@@ -39,14 +39,14 @@ type ClientState interface {
 	CheckProposedHeaderAndUpdateState(sdk.Context, codec.BinaryMarshaler, sdk.KVStore, Header) (ClientState, ConsensusState, error)
 
 	// Upgrade functions
-	VerifyUpgrade(
+	VerifyUpgradeAndUpdateState(
 		ctx sdk.Context,
 		cdc codec.BinaryMarshaler,
 		store sdk.KVStore,
 		newClient ClientState,
 		upgradeHeight Height,
 		proofUpgrade []byte,
-	) error
+	) (ClientState, ConsensusState, 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

x/ibc/light-clients/06-solomachine/types/client_state.go

@@ -74,12 +74,12 @@ func (cs ClientState) ZeroCustomFields() exported.ClientState {
 	)
 }
 
-// VerifyUpgrade returns an error since solomachine client does not support upgrades
-func (cs ClientState) VerifyUpgrade(
+// VerifyUpgradeAndUpdateState returns an error since solomachine client does not support upgrades
+func (cs ClientState) VerifyUpgradeAndUpdateState(
 	_ sdk.Context, _ codec.BinaryMarshaler, _ sdk.KVStore,
 	_ exported.ClientState, _ exported.Height, _ []byte,
-) error {
-	return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client")
+) (exported.ClientState, exported.ConsensusState, error) {
+	return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client")
 }
 
 // VerifyClientState verifies a proof of the client state of the running chain

x/ibc/light-clients/07-tendermint/types/upgrade.go

@@ -3,7 +3,6 @@ package types
 import (
 	"fmt"
 	"net/url"
-	"reflect"
 	"strings"
 
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -14,7 +13,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
 )
 
-// VerifyUpgrade checks if the upgraded client has been committed by the current client
+// VerifyUpgradeAndUpdateState checks if the upgraded client has been committed by the current client
 // It will zero out all client-specific fields (e.g. TrustingPeriod and verify all data
 // in client state that must be the same across all valid Tendermint clients for the new chain.
 // VerifyUpgrade will return an error if:
@@ -23,85 +22,83 @@ import (
 // - the latest height of the new client does not match the height in committed client
 // - any Tendermint chain specified parameter in upgraded client such as ChainID, UnbondingPeriod,
 //   and ProofSpecs do not match parameters set by committed client
-func (cs ClientState) VerifyUpgrade(
+func (cs ClientState) VerifyUpgradeAndUpdateState(
 	ctx sdk.Context, cdc codec.BinaryMarshaler, clientStore sdk.KVStore,
 	upgradedClient exported.ClientState, upgradeHeight exported.Height, proofUpgrade []byte,
-) error {
+) (exported.ClientState, exported.ConsensusState, error) {
 	if cs.UpgradePath == "" {
-		return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set")
+		return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set")
 	}
 	upgradePath, err := constructUpgradeMerklePath(cs.UpgradePath, upgradeHeight)
 	if err != nil {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, unescaping key with URL format failed: %v", err)
+		return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, unescaping key with URL format failed: %v", err)
 	}
 
 	// UpgradeHeight must be in same version as client state height
 	if cs.GetLatestHeight().GetVersionNumber() != upgradeHeight.GetVersionNumber() {
-		return sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version at which upgrade occurs must be same as current client version. expected version %d, got %d",
+		return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version at which upgrade occurs must be same as current client version. expected version %d, got %d",
 			cs.GetLatestHeight().GetVersionNumber(), upgradeHeight.GetVersionNumber())
 	}
 
-	tmClient, ok := upgradedClient.(*ClientState)
-	if !ok {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T",
-			&ClientState{}, upgradedClient)
-	}
-
 	if !upgradedClient.GetLatestHeight().GT(cs.GetLatestHeight()) {
-		return sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgraded client height %s must be greater than current client height %s",
+		return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgraded client height %s must be greater than current client height %s",
 			upgradedClient.GetLatestHeight(), cs.GetLatestHeight())
 	}
 
 	if len(proofUpgrade) == 0 {
-		return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "proof of upgrade is empty")
+		return nil, nil, sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "proof of upgrade is empty")
 	}
 
 	var merkleProof commitmenttypes.MerkleProof
 	if err := cdc.UnmarshalBinaryBare(proofUpgrade, &merkleProof); err != nil {
-		return sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal merkle proof: %v", err)
+		return nil, nil, sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal merkle proof: %v", err)
 	}
 
 	// counterparty chain must commit the upgraded client with all client-customizable fields zeroed out
 	// at the upgrade path specified by current client
 	committedClient := upgradedClient.ZeroCustomFields()
 	bz, err := codec.MarshalAny(cdc, committedClient)
 	if err != nil {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err)
+		return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %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 version, since committed client must exist
 	// at this consensus state
 	consState, err := GetConsensusState(clientStore, cdc, upgradeHeight)
 	if err != nil {
-		return sdkerrors.Wrap(err, "could not retrieve consensus state for upgradeHeight")
+		return nil, nil, sdkerrors.Wrap(err, "could not retrieve consensus state for upgradeHeight")
 	}
 
 	if cs.IsExpired(consState.Timestamp, ctx.BlockTime()) {
-		return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "cannot upgrade an expired client")
+		return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidClient, "cannot upgrade an expired client")
 	}
 
 	tmCommittedClient, ok := committedClient.(*ClientState)
 	if !ok {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T",
+		return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T",
 			&ClientState{}, upgradedClient)
 	}
 
-	// Relayer must keep all client-chosen parameters the same as the previous client.
-	// Compare relayer-provided client state against expected client state.
+	// Relayer chosen client parameters are ignored.
 	// All chain-chosen parameters come from committed client, all client-chosen parameters
-	// come from current client
-	expectedClient := NewClientState(
+	// come from current client.
+	updatedClientState := NewClientState(
 		tmCommittedClient.ChainId, cs.TrustLevel, cs.TrustingPeriod, tmCommittedClient.UnbondingPeriod,
 		cs.MaxClockDrift, tmCommittedClient.LatestHeight, tmCommittedClient.ConsensusParams, tmCommittedClient.ProofSpecs, tmCommittedClient.UpgradePath,
 		cs.AllowUpdateAfterExpiry, cs.AllowUpdateAfterMisbehaviour,
 	)
-	if !reflect.DeepEqual(expectedClient, tmClient) {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "upgraded client does not maintain previous chosen parameters. expected: %v got: %v",
-			expectedClient, tmClient)
+
+	if err := updatedClientState.Validate(); err != nil {
+		return nil, nil, sdkerrors.Wrap(err, "updated client state failed basic validation")
+	}
+
+	if err := merkleProof.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradePath, bz); err != nil {
+		return nil, nil, err
 	}
 
-	return merkleProof.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradePath, bz)
+	// TODO: Return valid consensus state https://github.com/cosmos/cosmos-sdk/issues/7708
+	return updatedClientState, &ConsensusState{}, nil
 }
 
 // construct MerklePath from upgradePath

x/ibc/light-clients/07-tendermint/types/upgrade_test.go

@@ -317,6 +317,31 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 			},
 			expPass: false,
 		},
+		{
+			name: "unsuccessful upgrade: updated unbonding period is equal to trusting period",
+			setup: func() {
+
+				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, trustingPeriod, maxClockDrift, newClientHeight, ibctesting.DefaultConsensusParams, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+
+				// upgrade Height is at next block
+				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
+
+				// zero custom fields and store in upgrade store
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetVersionHeight()), upgradedClient)
+
+				// commit upgrade store changes and update clients
+
+				suite.coordinator.CommitBlock(suite.chainB)
+				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, exported.Tendermint)
+				suite.Require().NoError(err)
+
+				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
+				suite.Require().True(found)
+
+				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
+			},
+			expPass: false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -332,7 +357,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 		cs := suite.chainA.GetClientState(clientA)
 		clientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA)
 
-		err := cs.VerifyUpgrade(
+		clientState, consensusState, err := cs.VerifyUpgradeAndUpdateState(
 			suite.chainA.GetContext(),
 			suite.cdc,
 			clientStore,
@@ -343,8 +368,14 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 
 		if tc.expPass {
 			suite.Require().NoError(err, "verify upgrade failed on valid case: %s", tc.name)
+			suite.Require().NotNil(clientState, "verify upgrade failed on valid case: %s", tc.name)
+			suite.Require().NotNil(consensusState, "verify upgrade failed on valid case: %s", tc.name)
 		} 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)
+
 		}
 	}
 }

x/ibc/light-clients/09-localhost/types/client_state.go

@@ -102,12 +102,12 @@ func (cs ClientState) CheckProposedHeaderAndUpdateState(
 	return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "cannot update localhost client with a proposal")
 }
 
-// VerifyUpgrade returns an error since localhost cannot be upgraded
-func (cs ClientState) VerifyUpgrade(
+// VerifyUpgradeAndUpdateState returns an error since localhost cannot be upgraded
+func (cs ClientState) VerifyUpgradeAndUpdateState(
 	_ sdk.Context, _ codec.BinaryMarshaler, _ sdk.KVStore,
 	_ exported.ClientState, _ exported.Height, _ []byte,
-) error {
-	return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade localhost client")
+) (exported.ClientState, exported.ConsensusState, error) {
+	return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade localhost client")
 }
 
 // VerifyClientState verifies that the localhost client state is stored locally