certusone
/
cosmos-sdk
mirror of https://github.com/certusone/cosmos-sdk.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 <25233464+colin-axner@users.noreply.github.com>
This commit is contained in:
Aditya 2020-10-06 05:49:15 -04:00 committed by GitHub
parent 4a1b2fba43
commit c9cb02ea98
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 53 additions and 134 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
x/ibc/core/02-client/handler_test.go
View File

@ -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() {

29
x/ibc/core/02-client/keeper/client_test.go
View File

@ -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() {

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

@ -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)
}

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

@ -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()