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 all commits
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
31 changes: 18 additions & 13 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 @@ -322,8 +323,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
upgradedClient *ibctm.ClientState
upgradedConsState exported.ConsensusState
upgradeHeight exported.Height
upgradedClientAny, upgradedConsStateAny *codectypes.Any
upgradedClientProof, upgradedConsensusStateProof []byte
upgradedClientBz, upgradedConsStateBz []byte
)

testCases := []struct {
Expand All @@ -338,9 +339,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 @@ -365,9 +366,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))
suite.Require().NoError(err)

// commit upgrade store changes and update clients
Expand Down Expand Up @@ -397,9 +398,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))
suite.Require().NoError(err)

// commit upgrade store changes and update clients
Expand Down Expand Up @@ -430,14 +431,15 @@ 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))
suite.Require().NoError(err)

// change upgradedClient client-specified parameters
upgradedClient.ChainId = "wrongchainID"
upgradedClientBz = clienttypes.MustMarshalClientState(suite.chainA.Codec, upgradedClient)
upgradedClientAny, err = codectypes.NewAnyWithValue(upgradedClient)
suite.Require().NoError(err)

suite.coordinator.CommitBlock(suite.chainB)
err = path.EndpointA.UpdateClient()
Expand Down Expand Up @@ -469,15 +471,18 @@ 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)

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

upgradedConsState = &ibctm.ConsensusState{NextValidatorsHash: []byte("nextValsHash")}
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
22 changes: 7 additions & 15 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,18 @@ func (k Keeper) UpdateClient(goCtx context.Context, msg *clienttypes.MsgUpdateCl
}

// UpgradeClient defines a rpc handler method for MsgUpgradeClient.
// NOTE: The raw bytes of the conretes types encoded into protobuf.Any is passed to the client keeper.
// The 02-client handler will route to the appropriate light client module based on client identifier and it is the responsibility
// of the light client module to unmarshal and interpret the proto encoded bytes.
// Backwards compatibility with older versions of ibc-go is maintained through the light client module reconstructing and encoding
// the expected concrete type to the protobuf.Any for proof verification.
func (k Keeper) UpgradeClient(goCtx context.Context, msg *clienttypes.MsgUpgradeClient) (*clienttypes.MsgUpgradeClientResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

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

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
30 changes: 10 additions & 20 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 @@ -277,26 +277,16 @@ func (lcm LightClientModule) VerifyUpgradeAndUpdateState(
upgradeClientProof,
upgradeConsensusStateProof []byte,
) error {
var (
cdc = lcm.keeper.Codec()
newClientState exported.ClientState
newConsensusState exported.ConsensusState
)
cdc := lcm.keeper.Codec()

if err := cdc.UnmarshalInterface(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)
var newClientState ClientState
if err := cdc.Unmarshal(newClient, &newClientState); err != nil {
return errorsmod.Wrap(clienttypes.ErrInvalidClient, err.Error())
}

if err := cdc.UnmarshalInterface(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)
var newConsensusState ConsensusState
if err := cdc.Unmarshal(newConsState, &newConsensusState); err != nil {
return errorsmod.Wrap(clienttypes.ErrInvalidConsensus, err.Error())
}

clientStore := lcm.storeProvider.ClientStore(ctx, clientID)
Expand All @@ -307,9 +297,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)
}
Loading
Loading