Skip to content

Commit

Permalink
UpgradeClient Followup cosmos#1 (cosmos#7457)
Browse files Browse the repository at this point in the history 
* require old chain halts before upgrade

* Update proto/ibc/core/client/v1/client.proto

Co-authored-by: Federico Kunze <[email protected]>

* start address reviews

* Apply suggestions from code review

Co-authored-by: colin axnér <[email protected]>

* address reviews

* rework upgrade to ensure there is never more than one upgrade client in store

* fix tests

* fix conditional

* make proto-gen

* remove if statement skipping tests in upgrade keeper test

* address reviews

* correctly escape and unescape merkle keys

* add small conditional check

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Colin Axner <[email protected]>
  • Loading branch information
@AdityaSripal @fedekunze @colin-axner
4 people authored Oct 8, 2020
1 parent a87d6ea commit 31ab35a
Show file tree
Hide file tree
Showing 34 changed files with 1,328 additions and 958 deletions.
6 changes: 4 additions & 2 deletions proto/ibc/core/client/v1/client.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ message MsgUpgradeClient {
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\""];
// proof that old chain committed to new client
bytes proof_upgrade = 3 [(gogoproto.moretags) = "yaml:\"proof_upgrade\""];
bytes proof_upgrade = 4 [(gogoproto.moretags) = "yaml:\"proof_upgrade\""];
// signer address
string signer = 4;
string signer = 5;
}

// MsgSubmitMisbehaviour defines an sdk.Msg type that submits Evidence for
Expand Down
2 changes: 1 addition & 1 deletion proto/ibc/lightclients/tendermint/v1/tendermint.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ message ClientState {
repeated ics23.ProofSpec proof_specs = 8 [(gogoproto.moretags) = "yaml:\"proof_specs\""];

// Path at which next upgraded client will be committed
ibc.core.commitment.v1.MerklePath upgrade_path = 9 [(gogoproto.moretags) = "yaml:\"upgrade_path\""];
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
Expand Down
3 changes: 2 additions & 1 deletion x/distribution/types/genesis.pb.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion x/ibc/core/02-client/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func HandleMsgUpgradeClient(ctx sdk.Context, k keeper.Keeper, msg *types.MsgUpgr
return nil, err
}

if err = k.UpgradeClient(ctx, msg.ClientId, upgradedClient, msg.ProofUpgrade); err != nil {
if err = k.UpgradeClient(ctx, msg.ClientId, upgradedClient, msg.UpgradeHeight, msg.ProofUpgrade); err != nil {
return nil, err
}

Expand Down
46 changes: 32 additions & 14 deletions x/ibc/core/02-client/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,11 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
var (
clientA string
upgradedClient exported.ClientState
upgradeHeight exported.Height
msg *clienttypes.MsgUpgradeClient
)

upgradeHeight := clienttypes.NewHeight(1, 1)
newClientHeight := clienttypes.NewHeight(1, 1)

cases := []struct {
name string
Expand All @@ -98,9 +99,13 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
name: "successful upgrade",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &ibctesting.UpgradePath, false, false)
upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.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(), upgradedClient)
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)

// commit upgrade store changes and update clients

Expand All @@ -111,9 +116,9 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
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())
proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: true,
Expand All @@ -125,23 +130,32 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
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())
// upgrade Height is at next block
upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))

proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())

consState := ibctmtypes.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("app_hash")), []byte("next_vals_hash"))
consAny, err := clienttypes.PackConsensusState(consState)
suite.Require().NoError(err)

msg = &types.MsgUpgradeClient{ClientId: clientA, ClientState: consAny, ProofUpgrade: proofUpgrade, Signer: suite.chainA.SenderAccount.GetAddress().String()}
height, _ := upgradeHeight.(types.Height)

msg = &types.MsgUpgradeClient{ClientId: clientA, ClientState: consAny, UpgradeHeight: &height, ProofUpgrade: proofUpgrade, Signer: suite.chainA.SenderAccount.GetAddress().String()}
},
expPass: false,
},
{
name: "invalid clientstate",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &ibctesting.UpgradePath, false, false)
upgradedClient = ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.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(), upgradedClient)
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)

// commit upgrade store changes and update clients

Expand All @@ -152,9 +166,9 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
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())
proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: false,
Expand All @@ -163,17 +177,21 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
name: "VerifyUpgrade fails",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &ibctesting.UpgradePath, false, false)
upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.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(), upgradedClient)
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)

// 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)

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, nil, suite.chainA.SenderAccount.GetAddress())
msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, nil, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: false,
Expand Down
4 changes: 2 additions & 2 deletions x/ibc/core/02-client/keeper/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ 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, proofUpgrade []byte) error {
func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient exported.ClientState, upgradeHeight exported.Height, proofUpgrade []byte) error {
clientState, found := k.GetClientState(ctx, clientID)
if !found {
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
Expand All @@ -93,7 +93,7 @@ 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, proofUpgrade)
err := clientState.VerifyUpgrade(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade)
if err != nil {
return sdkerrors.Wrapf(err, "cannot upgrade client with ID: %s", clientID)
}
Expand Down
Loading

0 comments on commit 31ab35a

Please sign in to comment.