Add ConsensusState to IBC Upgrades (#7919)

* upgrade progress

* fix build minus cli

* write state at block before upgrade height

* refix build

* Apply suggestions from code review

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* fix upgrade and start with tests

* fix tendermint tests

* add tests and remove unnecessary relayer options on upgradedClient

* fix all tests except weird msg panic

* add invalid final client test and codec stuff

* fix everything but msg issue

* remove problematic test for now

* safer self validation

* upgrade fixes

* proto-linting

* remove unnecessary last height

* add timestamp to committed upgrade consensus state

* address final nits

* address nit

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

proto/cosmos/upgrade/v1beta1/query.proto

@@ -1,6 +1,7 @@
 syntax = "proto3";
 package cosmos.upgrade.v1beta1;
 
+import "google/protobuf/any.proto";
 import "google/api/annotations.proto";
 import "cosmos/upgrade/v1beta1/upgrade.proto";
 
@@ -17,6 +18,14 @@ service Query {
   rpc AppliedPlan(QueryAppliedPlanRequest) returns (QueryAppliedPlanResponse) {
     option (google.api.http).get = "/cosmos/upgrade/v1beta1/applied_plan/{name}";
   }
+
+  // UpgradedConsensusState queries the consensus state that will serve
+  // as a trusted kernel for the next version of this chain. It will only be
+  // stored at the last height of this chain.
+  // UpgradedConsensusState RPC not supported with legacy querier
+  rpc UpgradedConsensusState(QueryUpgradedConsensusStateRequest) returns (QueryUpgradedConsensusStateResponse) {
+    option (google.api.http).get = "/cosmos/upgrade/v1beta1/upgraded_consensus_state/{last_height}";
+  }
 }
 
 // QueryCurrentPlanRequest is the request type for the Query/CurrentPlan RPC
@@ -43,3 +52,17 @@ message QueryAppliedPlanResponse {
   // height is the block height at which the plan was applied.
   int64 height = 1;
 }
+
+// QueryUpgradedConsensusStateRequest is the request type for the Query/UpgradedConsensusState
+// RPC method.
+message QueryUpgradedConsensusStateRequest {
+  // last height of the current chain must be sent in request
+  // as this is the height under which next consensus state is stored
+  int64 last_height = 1;
+}
+
+// QueryUpgradedConsensusStateResponse is the response type for the Query/UpgradedConsensusState
+// RPC method.
+message QueryUpgradedConsensusStateResponse {
+  google.protobuf.Any upgraded_consensus_state = 1;
+}

proto/ibc/core/client/v1/tx.proto

@@ -60,16 +60,21 @@ message MsgUpdateClientResponse {}
 
 // MsgUpgradeClient defines an sdk.Msg to upgrade an IBC client to a new client state
 message MsgUpgradeClient {
+  option (gogoproto.equal)           = false;
+  option (gogoproto.goproto_getters) = false;
+
   // client unique identifier
   string client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""];
   // upgraded client state
   google.protobuf.Any client_state = 2 [(gogoproto.moretags) = "yaml:\"client_state\""];
-  // height at which old chain halts and upgrades (i.e last block executed)
-  Height upgrade_height = 3 [(gogoproto.moretags) = "yaml:\"upgrade_height\""];
+  // upgraded consensus state, only contains enough information to serve as a basis of trust in update logic
+  google.protobuf.Any consensus_state = 3 [(gogoproto.moretags) = "yaml:\"consensus_state\""];
   // proof that old chain committed to new client
-  bytes proof_upgrade = 4 [(gogoproto.moretags) = "yaml:\"proof_upgrade\""];
+  bytes proof_upgrade_client = 4 [(gogoproto.moretags) = "yaml:\"proof_upgrade_client\""];
+  // proof that old chain committed to new consensus state
+  bytes proof_upgrade_consensus_state = 5 [(gogoproto.moretags) = "yaml:\"proof_upgrade_consensus_state\""];
   // signer address
-  string signer = 5;
+  string signer = 6;
 }
 
 // MsgUpgradeClientResponse defines the Msg/UpgradeClient response type.

proto/ibc/lightclients/tendermint/v1/tendermint.proto

@@ -42,8 +42,12 @@ message ClientState {
   // Proof specifications used in verifying counterparty state
   repeated ics23.ProofSpec proof_specs = 8 [(gogoproto.moretags) = "yaml:\"proof_specs\""];
 
-  // Path at which next upgraded client will be committed
-  string upgrade_path = 9 [(gogoproto.moretags) = "yaml:\"upgrade_path\""];
+  // Path at which next upgraded client will be committed.
+  // Each element corresponds to the key for a single CommitmentProof in the chained proof.
+  // NOTE: ClientState must stored under `{upgradePath}/{upgradeHeight}/clientState`
+  // ConsensusState must be stored under `{upgradepath}/{upgradeHeight}/consensusState`
+  // For SDK chains using the default upgrade module, upgrade_path should be []string{"upgrade", "upgradedIBCState"}`
+  repeated string upgrade_path = 9 [(gogoproto.moretags) = "yaml:\"upgrade_path\""];
 
   // This flag, when set to true, will allow governance to recover a client
   // which has expired

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

@@ -107,7 +107,8 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 
 // UpgradeClient upgrades the client to a new client state if this new client was committed to
 // by the old client at the specified upgrade height
-func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient exported.ClientState, upgradeHeight exported.Height, proofUpgrade []byte) error {
+func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient exported.ClientState, upgradedConsState exported.ConsensusState,
+	proofUpgradeClient, proofUpgradeConsState []byte) error {
 	clientState, found := k.GetClientState(ctx, clientID)
 	if !found {
 		return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
@@ -118,13 +119,14 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
-	updatedClientState, updatedConsensusState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade)
+	updatedClientState, updatedConsState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID),
+		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(), updatedConsensusState)
+	k.SetClientConsensusState(ctx, clientID, updatedClientState.GetLatestHeight(), updatedConsState)
 
 	k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", updatedClientState.GetLatestHeight().String())
 

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

@@ -235,10 +235,11 @@ func (suite *KeeperTestSuite) TestUpdateClientLocalhost() {
 
 func (suite *KeeperTestSuite) TestUpgradeClient() {
 	var (
-		upgradedClient exported.ClientState
-		upgradeHeight  exported.Height
-		clientA        string
-		proofUpgrade   []byte
+		upgradedClient                              exported.ClientState
+		upgradedConsState                           exported.ConsensusState
+		lastHeight                                  exported.Height
+		clientA                                     string
+		proofUpgradedClient, proofUpgradedConsState []byte
 	)
 
 	testCases := []struct {
@@ -251,12 +252,16 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			setup: func() {
 
 				upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+				upgradedConsState = &ibctmtypes.ConsensusState{
+					NextValidatorsHash: []byte("nextValsHash"),
+				}
 
-				// upgrade Height is at next block
-				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
+				// last Height is at next block
+				lastHeight = 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)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedClient)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedConsState)
 
 				// commit upgrade store changes and update clients
 
@@ -267,7 +272,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 				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())
+				proofUpgradedClient, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
+				proofUpgradedConsState, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
 			},
 			expPass: true,
 		},
@@ -276,12 +282,16 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			setup: func() {
 
 				upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+				upgradedConsState = &ibctmtypes.ConsensusState{
+					NextValidatorsHash: []byte("nextValsHash"),
+				}
 
-				// upgrade Height is at next block
-				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
+				// last Height is at next block
+				lastHeight = 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)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedClient)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedConsState)
 
 				// commit upgrade store changes and update clients
 
@@ -292,7 +302,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 				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())
+				proofUpgradedClient, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
+				proofUpgradedConsState, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
 
 				clientA = "wrongclientid"
 			},
@@ -303,12 +314,16 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			setup: func() {
 
 				upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+				upgradedConsState = &ibctmtypes.ConsensusState{
+					NextValidatorsHash: []byte("nextValsHash"),
+				}
 
-				// upgrade Height is at next block
-				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
+				// last Height is at next block
+				lastHeight = 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)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedClient)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedConsState)
 
 				// commit upgrade store changes and update clients
 
@@ -319,7 +334,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 				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())
+				proofUpgradedClient, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
+				proofUpgradedConsState, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
 
 				// set frozen client in store
 				tmClient, ok := cs.(*ibctmtypes.ClientState)
@@ -334,12 +350,16 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			setup: func() {
 
 				upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+				upgradedConsState = &ibctmtypes.ConsensusState{
+					NextValidatorsHash: []byte("nextValsHash"),
+				}
 
-				// upgrade Height is at next block
-				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
+				// last Height is at next block
+				lastHeight = 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)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedClient)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetVersionHeight()), upgradedConsState)
 
 				// change upgradedClient client-specified parameters
 				upgradedClient = ibctmtypes.NewClientState("wrongchainID", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, true, true)
@@ -351,7 +371,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 				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())
+				proofUpgradedClient, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
+				proofUpgradedConsState, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
 			},
 			expPass: false,
 		},
@@ -363,7 +384,10 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 
 		tc.setup()
 
-		err := suite.chainA.App.IBCKeeper.ClientKeeper.UpgradeClient(suite.chainA.GetContext(), clientA, upgradedClient, upgradeHeight, proofUpgrade)
+		// Call ZeroCustomFields on upgraded clients to clear any client-chosen parameters in test-case upgradedClient
+		upgradedClient = upgradedClient.ZeroCustomFields()
+
+		err := suite.chainA.App.IBCKeeper.ClientKeeper.UpgradeClient(suite.chainA.GetContext(), clientA, upgradedClient, upgradedConsState, proofUpgradedClient, proofUpgradedConsState)
 
 		if tc.expPass {
 			suite.Require().NoError(err, "verify upgrade failed on valid case: %s", tc.name)

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

@@ -2,7 +2,6 @@ package keeper
 
 import (
 	"fmt"
-	"net/url"
 	"reflect"
 	"strings"
 
@@ -246,13 +245,11 @@ func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientS
 			tmClient.UnbondingPeriod, tmClient.TrustingPeriod)
 	}
 
-	if tmClient.UpgradePath != "" {
+	if len(tmClient.UpgradePath) != 0 {
 		// For now, SDK IBC implementation assumes that upgrade path (if defined) is defined by SDK upgrade module
-		// Must escape any merkle key before adding it to upgrade path
-		upgradeKey := url.PathEscape(upgradetypes.KeyUpgradedClient)
-		expectedUpgradePath := fmt.Sprintf("%s/%s", upgradetypes.StoreKey, upgradeKey)
-		if tmClient.UpgradePath != expectedUpgradePath {
-			return sdkerrors.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %s, got %s",
+		expectedUpgradePath := []string{upgradetypes.StoreKey, upgradetypes.KeyUpgradedIBCState}
+		if !reflect.DeepEqual(expectedUpgradePath, tmClient.UpgradePath) {
+			return sdkerrors.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %v, got %v",
 				expectedUpgradePath, tmClient.UpgradePath)
 		}
 	}

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

@@ -166,7 +166,7 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() {
 		},
 		{
 			"success with nil UpgradePath",
-			ibctmtypes.NewClientState(suite.chainA.ChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), "", false, false),
+			ibctmtypes.NewClientState(suite.chainA.ChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), nil, false, false),
 			true,
 		},
 		{
@@ -216,7 +216,7 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() {
 		},
 		{
 			"invalid upgrade path",
-			ibctmtypes.NewClientState(suite.chainA.ChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), "bad/upgrade/path", false, false),
+			ibctmtypes.NewClientState(suite.chainA.ChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), []string{"bad", "upgrade", "path"}, false, false),
 			false,
 		},
 	}

x/ibc/core/02-client/types/codec.go

@@ -37,6 +37,7 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
 		(*sdk.Msg)(nil),
 		&MsgCreateClient{},
 		&MsgUpdateClient{},
+		&MsgUpgradeClient{},
 		&MsgSubmitMisbehaviour{},
 	)