Upgrade-Client Followup #2 (#7460)

* enforce client-chosen parameters are persisted across upgrades in tendermint clients * Update x/ibc/light-clients/07-tendermint/types/upgrade.go Co-authored-by: colin axnér <[email protected]>
cosmos · Oct 6, 2020 · c9cb02e · c9cb02e
1 parent 4a1b2fb
commit c9cb02e
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 134 deletions.
diff --git a/x/ibc/core/02-client/handler_test.go b/x/ibc/core/02-client/handler_test.go
@@ -11,7 +11,6 @@ import (
 	clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
 	commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/23-commitment/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
-	solomachinetypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/types"
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
@@ -119,34 +118,6 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
 			},
 			expPass: true,
 		},
-		{
-			name: "successful upgrade to different client type",
-			setup: func() {
-
-				// previous chain committed to the change
-				upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.chainA.App.AppCodec(), clientA, "diversifier", 1).ClientState()
-				soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
-				// change sequence to be higher height than latest current client height
-				soloClient.Sequence = 100000000000
-				// zero custom fields and store in upgrade store
-				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), soloClient)
-
-				// commit upgrade store changes and update clients
-
-				suite.coordinator.CommitBlock(suite.chainB)
-				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.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(), cs.GetLatestHeight().GetEpochHeight())
-
-				msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
-				suite.Require().NoError(err)
-			},
-			expPass: true,
-		},
 		{
 			name: "invalid upgrade: msg.ClientState does not contain valid clientstate",
 			setup: func() {

diff --git a/x/ibc/core/02-client/keeper/client_test.go b/x/ibc/core/02-client/keeper/client_test.go
@@ -11,7 +11,6 @@ import (
 	clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
 	commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/23-commitment/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
-	solomachinetypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/types"
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
 	localhosttypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/09-localhost/types"
 	ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
@@ -248,7 +247,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 		expPass bool
 	}{
 		{
-			name: "successful upgrade to a new tendermint client",
+			name: "successful upgrade",
 			setup: func() {
 
 				upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &ibctesting.UpgradePath, false, false)
@@ -268,32 +267,6 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			},
 			expPass: true,
 		},
-		{
-			name: "successful upgrade to a solomachine client",
-			setup: func() {
-				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
-				suite.Require().True(found)
-
-				// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
-				// previous chain committed to the change
-				upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
-				soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
-				// change sequence to be higher height than latest current client height
-				soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
-				// zero custom fields and store in upgrade store
-				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
-
-				suite.coordinator.CommitBlock(suite.chainB)
-				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.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(), cs.GetLatestHeight().GetEpochHeight())
-			},
-			expPass: true,
-		},
 		{
 			name: "client state not found",
 			setup: func() {

diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade.go b/x/ibc/light-clients/07-tendermint/types/upgrade.go
@@ -1,6 +1,8 @@
 package types
 
 import (
+	"reflect"
+
 	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -25,6 +27,11 @@ func (cs ClientState) VerifyUpgrade(
 	if cs.UpgradePath == nil {
 		return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set")
 	}
+	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, "upgrade client height %s must be greater than current client height %s",
@@ -54,5 +61,25 @@ func (cs ClientState) VerifyUpgrade(
 		return sdkerrors.Wrap(err, "could not retrieve latest consensus state")
 	}
 
+	tmCommittedClient, ok := committedClient.(*ClientState)
+	if !ok {
+		return 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.
+	// All chain-chosen parameters come from committed client, all client-chosen parameters
+	// come from current client
+	expectedClient := NewClientState(
+		tmCommittedClient.ChainId, cs.TrustLevel, cs.TrustingPeriod, tmCommittedClient.UnbondingPeriod,
+		cs.MaxClockDrift, tmCommittedClient.LatestHeight, 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)
+	}
+
 	return merkleProof.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), *cs.UpgradePath, bz)
 }
diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade_test.go b/x/ibc/light-clients/07-tendermint/types/upgrade_test.go
@@ -1,9 +1,10 @@
 package types_test
 
 import (
+	"fmt"
+
 	commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/23-commitment/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
-	solomachinetypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
@@ -22,7 +23,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 		expPass bool
 	}{
 		{
-			name: "successful upgrade to a new tendermint client",
+			name: "successful upgrade",
 			setup: func() {
 
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
@@ -43,15 +44,15 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 			expPass: true,
 		},
 		{
-			name: "successful upgrade to a new tendermint client with different client chosen parameters",
+			name: "successful upgrade with different client chosen parameters",
 			setup: func() {
 
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
 				// zero custom fields and store in upgrade store
 				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
 
 				// change upgradedClient client-specified parameters
-				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, true)
+				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, false)
 
 				suite.coordinator.CommitBlock(suite.chainB)
 				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
@@ -60,67 +61,17 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
 				suite.Require().True(found)
 
-				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
-			},
-			expPass: true,
-		},
-		{
-			name: "successful upgrade to a solomachine client",
-			setup: func() {
-				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
-				suite.Require().True(found)
-
-				// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
-				// previous chain committed to the change
-				upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
-				soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
-				// change sequence to be higher height than latest current client height
-				soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
-				// zero custom fields and store in upgrade store
-				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
-
-				suite.coordinator.CommitBlock(suite.chainB)
-				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.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(), cs.GetLatestHeight().GetEpochHeight())
-			},
-			expPass: true,
-		},
-		{
-			name: "successful upgrade to a solomachine client with different client-chosen parameters",
-			setup: func() {
-				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
-				suite.Require().True(found)
-
-				// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
-				// previous chain committed to the change
-				upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
-				soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
-				// change sequence to be higher height than latest current client height
-				soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
-				// zero custom fields and store in upgrade store
-				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), soloClient)
-
-				// change client-specified parameter
-				soloClient.AllowUpdateAfterProposal = true
-
-				suite.coordinator.CommitBlock(suite.chainB)
-				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
-				suite.Require().NoError(err)
-
-				cs, found = suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
-				suite.Require().True(found)
+				// Previous client-chosen parameters must be the same as upgraded client chosen parameters
+				tmClient, _ := cs.(*types.ClientState)
+				oldClient := types.NewClientState(tmClient.ChainId, types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, tmClient.LatestHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, false)
+				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, oldClient)
 
 				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
 			},
 			expPass: true,
 		},
 		{
-			name: "unsuccessful upgrade to a new tendermint client: chain-specified paramaters do not match committed client",
+			name: "unsuccessful upgrade: chain-specified paramaters do not match committed client",
 			setup: func() {
 
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
@@ -142,53 +93,45 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 			expPass: false,
 		},
 		{
-
-			name: "unsuccessful upgrade to a new solomachine client: chain-specified paramaters do not match committed client",
+			name: "unsuccessful upgrade: client-specified parameters do not match previous client",
 			setup: func() {
-				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
-				suite.Require().True(found)
 
-				// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
-				// previous chain committed to the change
-				upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
-				soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
-				// change sequence to be higher height than latest current client height
-				soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
+				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
 				// zero custom fields and store in upgrade store
 				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
 
-				// change chain-specified parameters from committed values
-				soloClient.Sequence = 10000
+				// change upgradedClient client-specified parameters
+				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, false)
 
 				suite.coordinator.CommitBlock(suite.chainB)
 				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
 				suite.Require().NoError(err)
 
-				cs, found = suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
+				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
 				suite.Require().True(found)
 
 				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
 			},
 			expPass: false,
 		},
 		{
-			name: "unsuccessful upgrade to a new tendermint client: proof is empty",
+			name: "unsuccessful upgrade: proof is empty",
 			setup: func() {
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
 				proofUpgrade = []byte{}
 			},
 			expPass: false,
 		},
 		{
-			name: "unsuccessful upgrade to a new tendermint client: proof unmarshal failed",
+			name: "unsuccessful upgrade: proof unmarshal failed",
 			setup: func() {
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
 				proofUpgrade = []byte("proof")
 			},
 			expPass: false,
 		},
 		{
-			name: "unsuccessful upgrade to a new tendermint client: proof verification failed",
+			name: "unsuccessful upgrade: proof verification failed",
 			setup: func() {
 				// create but do not store upgraded client
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
@@ -197,11 +140,12 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 				suite.Require().True(found)
 
 				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
+				fmt.Printf("%#v\n", proofUpgrade)
 			},
 			expPass: false,
 		},
 		{
-			name: "unsuccessful upgrade to a new tendermint client: upgrade path is nil",
+			name: "unsuccessful upgrade: upgrade path is nil",
 			setup: func() {
 
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
@@ -227,7 +171,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 			expPass: false,
 		},
 		{
-			name: "unsuccessful upgrade to a new tendermint client: upgraded height is not greater than current height",
+			name: "unsuccessful upgrade: upgraded height is not greater than current height",
 			setup: func() {
 
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
@@ -251,6 +195,10 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 
 	for _, tc := range testCases {
 		tc := tc
+
+		// reset suite
+		suite.SetupTest()
+
 		clientA, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, ibctesting.Tendermint)
 
 		tc.setup()