Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove marshalling of anys to bytes for verify upgrade #5967

Merged
merged 8 commits into from
Mar 12, 2024
Merged

fix: remove marshalling of anys to bytes for verify upgrade #5967

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0fe7df4
fix: remove marshalling of anys to bytes for verify upgrade
damiannolan Mar 11, 2024
3072bb4
test: refactor 02-client tests for verify upgrade
damiannolan Mar 11, 2024
8db1631
test: fix tests in 07-tenderint client module
damiannolan Mar 12, 2024
c0e4665
chore: rm backwards compatability notice and marshalling of anys in c…
damiannolan Mar 12, 2024
1298b89
test: cleanup happy path assertions in 07-tendermint verify upgrade test
damiannolan Mar 12, 2024
3e28783
nit: update inline code commentin test
damiannolan Mar 12, 2024
e550c20
doc: update godoc of UpgradeClient handler in core ibc
damiannolan Mar 12, 2024
59d5690
lint: make lint-fix
damiannolan Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 111 additions & 101 deletions modules/core/02-client/keeper/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

upgradetypes "cosmossdk.io/x/upgrade/types"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"

clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
Expand Down Expand Up @@ -323,7 +324,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
upgradedConsState exported.ConsensusState
upgradeHeight exported.Height
upgradedClientProof, upgradedConsensusStateProof []byte
upgradedClientBz, upgradedConsStateBz []byte
// upgradedClientBz, upgradedConsStateBz []byte
upgradedClientAny, upgradedConsStateAny *codectypes.Any
)

testCases := []struct {
Expand All @@ -338,9 +340,9 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), suite.chainB.Codec.MustMarshal(upgradedClientAny))
suite.Require().NoError(err)
err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), suite.chainB.Codec.MustMarshal(upgradedConsStateAny))
Comment on lines +342 to +344
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marshalling the any here is exactly what clienttypes.MarshalClientState() and clienttypes.MarshalConsensusState() were doing previously.

https://github.com/cosmos/cosmos-sdk/blob/main/codec/proto_codec.go#L230-L239

suite.Require().NoError(err)

// commit upgrade store changes and update clients
Expand All @@ -358,101 +360,101 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
},
expPass: true,
},
{
name: "client state not found",
setup: func() {
// last Height is at next block
upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
suite.Require().NoError(err)
err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
suite.Require().NoError(err)

// commit upgrade store changes and update clients

suite.coordinator.CommitBlock(suite.chainB)
err = path.EndpointA.UpdateClient()
suite.Require().NoError(err)

cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
suite.Require().True(found)
tmCs, ok := cs.(*ibctm.ClientState)
suite.Require().True(ok)

upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())

path.EndpointA.ClientID = "wrongclientid"
},
expPass: false,
},
{
name: "client state is not active",
setup: func() {
// client is frozen

// last Height is at next block
upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
suite.Require().NoError(err)
err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
suite.Require().NoError(err)

// commit upgrade store changes and update clients
suite.coordinator.CommitBlock(suite.chainB)
err = path.EndpointA.UpdateClient()
suite.Require().NoError(err)

cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
suite.Require().True(found)
tmCs, ok := cs.(*ibctm.ClientState)
suite.Require().True(ok)

upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())

// set frozen client in store
tmClient, ok := cs.(*ibctm.ClientState)
suite.Require().True(ok)
tmClient.FrozenHeight = clienttypes.NewHeight(1, 1)
suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, tmClient)
},
expPass: false,
},
{
name: "light client module VerifyUpgradeAndUpdateState fails",
setup: func() {
// last Height is at next block
upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
suite.Require().NoError(err)
err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
suite.Require().NoError(err)

// change upgradedClient client-specified parameters
upgradedClient.ChainId = "wrongchainID"
upgradedClientBz = clienttypes.MustMarshalClientState(suite.chainA.Codec, upgradedClient)

suite.coordinator.CommitBlock(suite.chainB)
err = path.EndpointA.UpdateClient()
suite.Require().NoError(err)

cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
suite.Require().True(found)
tmCs, ok := cs.(*ibctm.ClientState)
suite.Require().True(ok)

upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
},
expPass: false,
},
// {
// name: "client state not found",
// setup: func() {
// // last Height is at next block
// upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// // zero custom fields and store in upgrade store
// err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
// suite.Require().NoError(err)
// err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
// suite.Require().NoError(err)

// // commit upgrade store changes and update clients

// suite.coordinator.CommitBlock(suite.chainB)
// err = path.EndpointA.UpdateClient()
// suite.Require().NoError(err)

// cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
// suite.Require().True(found)
// tmCs, ok := cs.(*ibctm.ClientState)
// suite.Require().True(ok)

// upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
// upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())

// path.EndpointA.ClientID = "wrongclientid"
// },
// expPass: false,
// },
// {
// name: "client state is not active",
// setup: func() {
// // client is frozen

// // last Height is at next block
// upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// // zero custom fields and store in upgrade store
// err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
// suite.Require().NoError(err)
// err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
// suite.Require().NoError(err)

// // commit upgrade store changes and update clients
// suite.coordinator.CommitBlock(suite.chainB)
// err = path.EndpointA.UpdateClient()
// suite.Require().NoError(err)

// cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
// suite.Require().True(found)
// tmCs, ok := cs.(*ibctm.ClientState)
// suite.Require().True(ok)

// upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
// upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())

// // set frozen client in store
// tmClient, ok := cs.(*ibctm.ClientState)
// suite.Require().True(ok)
// tmClient.FrozenHeight = clienttypes.NewHeight(1, 1)
// suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, tmClient)
// },
// expPass: false,
// },
// {
// name: "light client module VerifyUpgradeAndUpdateState fails",
// setup: func() {
// // last Height is at next block
// upgradeHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))

// // zero custom fields and store in upgrade store
// err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedClientBz)
// suite.Require().NoError(err)
// err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(upgradeHeight.GetRevisionHeight()), upgradedConsStateBz)
// suite.Require().NoError(err)

// // change upgradedClient client-specified parameters
// upgradedClient.ChainId = "wrongchainID"
// upgradedClientBz = clienttypes.MustMarshalClientState(suite.chainA.Codec, upgradedClient)

// suite.coordinator.CommitBlock(suite.chainB)
// err = path.EndpointA.UpdateClient()
// suite.Require().NoError(err)

// cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
// suite.Require().True(found)
// tmCs, ok := cs.(*ibctm.ClientState)
// suite.Require().True(ok)

// upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
// upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(upgradeHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight())
// },
// expPass: false,
// },
}

for _, tc := range testCases {
Expand All @@ -469,15 +471,23 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {

upgradedClient = ibctm.NewClientState(newChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, clienttypes.NewHeight(revisionNumber+1, clientState.LatestHeight.GetRevisionHeight()+1), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
upgradedClient = upgradedClient.ZeroCustomFields()
upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)

// upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)
// suite.Require().NoError(err)

upgradedClientAny, err = codectypes.NewAnyWithValue(upgradedClient)
suite.Require().NoError(err)

upgradedConsState = &ibctm.ConsensusState{NextValidatorsHash: []byte("nextValsHash")}
upgradedConsStateBz = clienttypes.MustMarshalConsensusState(suite.chainA.Codec, upgradedConsState)

// upgradedConsStateBz = clienttypes.MustMarshalConsensusState(suite.chainA.Codec, upgradedConsState)

upgradedConsStateAny, err = codectypes.NewAnyWithValue(upgradedConsState)
suite.Require().NoError(err)

tc.setup()

err = suite.chainA.App.GetIBCKeeper().ClientKeeper.UpgradeClient(suite.chainA.GetContext(), path.EndpointA.ClientID, upgradedClientBz, upgradedConsStateBz, upgradedClientProof, upgradedConsensusStateProof)
err = suite.chainA.App.GetIBCKeeper().ClientKeeper.UpgradeClient(suite.chainA.GetContext(), path.EndpointA.ClientID, upgradedClientAny.Value, upgradedConsStateAny.Value, upgradedClientProof, upgradedConsensusStateProof)

if tc.expPass {
suite.Require().NoError(err, "verify upgrade failed on valid case: %s", tc.name)
Expand Down
24 changes: 12 additions & 12 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,21 +72,21 @@ func (k Keeper) UpgradeClient(goCtx context.Context, msg *clienttypes.MsgUpgrade

// Backwards Compatibility: the light client module is expecting
// to unmarshal an interface so we marshal the client state Any
upgradedClientState, err := k.cdc.Marshal(msg.ClientState)
if err != nil {
return nil, err
}
// Backwards Compatibility: the light client module is expecting
// to unmarshal an interface so we marshal the consensus state Any
upgradedConsensusState, err := k.cdc.Marshal(msg.ConsensusState)
if err != nil {
return nil, err
}
// upgradedClientState, err := k.cdc.Marshal(msg.ClientState)
// if err != nil {
// return nil, err
// }
// // Backwards Compatibility: the light client module is expecting
// // to unmarshal an interface so we marshal the consensus state Any
// upgradedConsensusState, err := k.cdc.Marshal(msg.ConsensusState)
// if err != nil {
// return nil, err
// }

if err := k.ClientKeeper.UpgradeClient(
ctx, msg.ClientId,
upgradedClientState,
upgradedConsensusState,
msg.ClientState.Value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe write a short comment to explain this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the godoc of UpgradeClient :)

msg.ConsensusState.Value,
msg.ProofUpgradeClient,
msg.ProofUpgradeConsensusState,
); err != nil {
Expand Down
32 changes: 17 additions & 15 deletions modules/light-clients/07-tendermint/light_client_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,25 +279,27 @@ func (lcm LightClientModule) VerifyUpgradeAndUpdateState(
) error {
var (
cdc = lcm.keeper.Codec()
newClientState exported.ClientState
newConsensusState exported.ConsensusState
newClientState ClientState
newConsensusState ConsensusState
)

if err := cdc.UnmarshalInterface(newClient, &newClientState); err != nil {
if err := cdc.Unmarshal(newClient, &newClientState); err != nil {
return err
}
newTmClientState, ok := newClientState.(*ClientState)
if !ok {
return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "expected client state type %T, got %T", (*ClientState)(nil), newClientState)
}

if err := cdc.UnmarshalInterface(newConsState, &newConsensusState); err != nil {
// newTmClientState, ok := newClientState.(*ClientState)
// if !ok {
// return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "expected client state type %T, got %T", (*ClientState)(nil), newClientState)
// }

if err := cdc.Unmarshal(newConsState, &newConsensusState); err != nil {
return err
}
newTmConsensusState, ok := newConsensusState.(*ConsensusState)
if !ok {
return errorsmod.Wrapf(clienttypes.ErrInvalidConsensus, "expected consensus state type %T, got %T", (*ConsensusState)(nil), newConsensusState)
}

// newTmConsensusState, ok := newConsensusState.(*ConsensusState)
// if !ok {
// return errorsmod.Wrapf(clienttypes.ErrInvalidConsensus, "expected consensus state type %T, got %T", (*ConsensusState)(nil), newConsensusState)
// }

clientStore := lcm.storeProvider.ClientStore(ctx, clientID)
clientState, found := getClientState(clientStore, cdc)
Expand All @@ -307,9 +309,9 @@ func (lcm LightClientModule) VerifyUpgradeAndUpdateState(

// last height of current counterparty chain must be client's latest height
lastHeight := clientState.LatestHeight
if !newTmClientState.LatestHeight.GT(lastHeight) {
return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s", newTmClientState.LatestHeight, lastHeight)
if !newClientState.LatestHeight.GT(lastHeight) {
return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s", newClientState.LatestHeight, lastHeight)
}

return clientState.VerifyUpgradeAndUpdateState(ctx, cdc, clientStore, newTmClientState, newTmConsensusState, upgradeClientProof, upgradeConsensusStateProof)
return clientState.VerifyUpgradeAndUpdateState(ctx, cdc, clientStore, &newClientState, &newConsensusState, upgradeClientProof, upgradeConsensusStateProof)
}
1 change: 1 addition & 0 deletions modules/light-clients/07-tendermint/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
return errorsmod.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T",
&ClientState{}, upgradedClient)
}

tmUpgradeConsState, ok := upgradedConsState.(*ConsensusState)
if !ok {
return errorsmod.Wrapf(clienttypes.ErrInvalidConsensus, "upgraded consensus state must be Tendermint consensus state. expected %T, got: %T",
Expand Down
Loading