From c9cb02ea98a6d2b3387d996e46f4ca072c6b868c Mon Sep 17 00:00:00 2001 From: Aditya Date: Tue, 6 Oct 2020 05:49:15 -0400 Subject: [PATCH] Upgrade-Client Followup #2 (#7460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 <25233464+colin-axner@users.noreply.github.com> --- x/ibc/core/02-client/handler_test.go | 29 ----- x/ibc/core/02-client/keeper/client_test.go | 29 +---- .../07-tendermint/types/upgrade.go | 27 +++++ .../07-tendermint/types/upgrade_test.go | 102 +++++------------- 4 files changed, 53 insertions(+), 134 deletions(-) diff --git a/x/ibc/core/02-client/handler_test.go b/x/ibc/core/02-client/handler_test.go index d03ed03408..93b96e33ce 100644 --- 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 index 9cec0e8bcd..edc66c1e44 100644 --- 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 index 80fa360646..3f2f31e7b1 100644 --- 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 index 52f876e1d0..e69d7b111f 100644 --- 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,7 +44,7 @@ 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) @@ -51,7 +52,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { 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) + // 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: "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) - - 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,29 +93,21 @@ 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()) @@ -172,7 +115,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { 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{} @@ -180,7 +123,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { 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") @@ -188,7 +131,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { 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()