From b53d06c03bcd17fb37049597531ee96c65e0c94c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Feb 2022 17:01:52 +0100 Subject: [PATCH] refactor: include transaction response in ics27 channel acknowledgement (#811) ## Description ref: #701 --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [x] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [x] Re-reviewed `Files changed` in the Github PR explorer - [x] Review `Codecov Report` in the comment section below once CI passes (cherry picked from commit 6c48f7e57a53504790b4759f05670fd46ce37b00) --- .../interchain-accounts/auth-modules.md | 117 ++++++++++++++++++ .../27-interchain-accounts/host/ibc_module.go | 11 +- .../host/ibc_module_test.go | 87 ++++++++++++- .../host/keeper/relay.go | 66 ++++++---- .../host/keeper/relay_test.go | 4 +- 5 files changed, 256 insertions(+), 29 deletions(-) diff --git a/docs/app-modules/interchain-accounts/auth-modules.md b/docs/app-modules/interchain-accounts/auth-modules.md index 4d8b126dd2f..b87265d4e28 100644 --- a/docs/app-modules/interchain-accounts/auth-modules.md +++ b/docs/app-modules/interchain-accounts/auth-modules.md @@ -210,6 +210,123 @@ seq, err = keeper.icaControllerKeeper.SendTx(ctx, chanCap, portID, packetData, t The data within an `InterchainAccountPacketData` must be serialized using a format supported by the host chain. If the host chain is using the ibc-go host chain submodule, `SerializeCosmosTx` should be used. If the `InterchainAccountPacketData.Data` is serialized using a format not support by the host chain, the packet will not be successfully received. +## `OnAcknowledgementPacket` + +Controller chains will be able to access the acknowledgement written into the host chain state once a relayer relays the acknowledgement. +The acknowledgement bytes will be passed to the auth module via the `OnAcknowledgementPacket` callback. +Auth modules are expected to know how to decode the acknowledgement. + +If the controller chain is connected to a host chain using the host module on ibc-go, it may interpret the acknowledgement bytes as follows: + +Begin by unmarshaling the acknowledgement into sdk.TxMsgData: +```go +txMsgData := &sdk.TxMsgData{} +if err := proto.Unmarshal(ack.Acknowledgement(), txMsgData); err != nil { + return err +} +``` + +If the txMsgData.Data field is non nil, the host chain is using SDK version <= v0.45. +The auth module should interpret the txMsgData.Data as follows: + +```go +switch len(txMsgData.Data) { +case 0: + for _, msgData := range txMsgData.Data { + if err := handler(msgData); err != nil { + return err + } + } +... +} +``` + +A handler will be needed to interpret what actions to perform based on the message type sent. +A router could be used, or more simply a switch statement. + +```go +func handler(msgData sdk.MsgData) error { +switch msgData.TypeURL { +case banktypes.MsgSend: + msgResponse := &banktypes.MsgSendResponse{} + if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { + return err + } + + handleBankSendMsg(msgResponse) + +case stakingtypes.MsgDelegate: + msgResponse := &stakingtypes.MsgDelegateResponse{} + if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { + return err + } + + handleStakingDelegateMsg(msgResponse) + +case transfertypes.MsgTransfer: + msgResponse := &transfertypes.MsgTransferResponse{} + if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { + return err + } + + handleIBCTransferMsg(msgResponse) + +default: + return +} +``` + +If the txMsgData.Data is empty, the host chain is using SDK version > v0.45. +The auth module should interpret the txMsgData.Responses as follows: + +```go +... +// switch statement from above continued +default: + for _, any := range txMsgData.MsgResponses { + if err := handleAny(any); err != nil { + return err + } + } +} +``` + +A handler will be needed to interpret what actions to perform based on the type url of the Any. +A router could be used, or more simply a switch statement. +It may be possible to deduplicate logic between `handler` and `handleAny`. + +```go +func handleAny(any *codectypes.Any) error { +switch any.TypeURL { +case banktypes.MsgSend: + msgResponse, err := unpackBankMsgSendResponse(any) + if err != nil { + return err + } + + handleBankSendMsg(msgResponse) + +case stakingtypes.MsgDelegate: + msgResponse, err := unpackStakingDelegateResponse(any) + if err != nil { + return err + } + + handleStakingDelegateMsg(msgResponse) + + case transfertypes.MsgTransfer: + msgResponse, err := unpackIBCTransferMsgResponse(any) + if err != nil { + return err + } + + handleIBCTransferMsg(msgResponse) + +default: + return +} +``` + ### Integration into `app.go` file To integrate the authentication module into your chain, please follow the steps outlined above in [app.go integration](./integration.md#example-integration). diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index cec21d258c3..be302e09151 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -108,17 +108,16 @@ func (im IBCModule) OnRecvPacket( return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled.Error()) } - ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - - if err := im.keeper.OnRecvPacket(ctx, packet); err != nil { - ack = types.NewErrorAcknowledgement(err) - + txResponse, err := im.keeper.OnRecvPacket(ctx, packet) + if err != nil { // Emit an event including the error msg keeper.EmitWriteErrorAcknowledgementEvent(ctx, packet, err) + + return types.NewErrorAcknowledgement(err) } // NOTE: acknowledgement will be written synchronously during IBC handler execution. - return ack + return channeltypes.NewResultAcknowledgement(txResponse) } // OnAcknowledgementPacket implements the IBCModule interface diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index ddf90c6d46c..d28633d08c6 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -7,8 +7,12 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/gogo/protobuf/proto" "github.com/stretchr/testify/suite" + abcitypes "github.com/tendermint/tendermint/abci/types" "github.com/tendermint/tendermint/crypto" + tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" + tmstate "github.com/tendermint/tendermint/state" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" @@ -458,6 +462,22 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { } packetData = icaPacketData.GetBytes() + // build expected ack + msgResponseBz, err := proto.Marshal(&banktypes.MsgSendResponse{}) + suite.Require().NoError(err) + + msgData := &sdk.MsgData{ + MsgType: sdk.MsgTypeURL(msg), + Data: msgResponseBz, + } + + expectedTxResponse, err := proto.Marshal(&sdk.TxMsgData{ + Data: []*sdk.MsgData{msgData}, + }) + suite.Require().NoError(err) + + expectedAck := channeltypes.NewResultAcknowledgement(expectedTxResponse) + params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) @@ -476,7 +496,12 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { suite.Require().True(ok) ack := cbs.OnRecvPacket(suite.chainB.GetContext(), packet, nil) - suite.Require().Equal(tc.expAckSuccess, ack.Success()) + if tc.expAckSuccess { + suite.Require().True(ack.Success()) + suite.Require().Equal(expectedAck, ack) + } else { + suite.Require().False(ack.Success()) + } }) } @@ -685,3 +710,63 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose() hasBalance = suite.chainB.GetSimApp().BankKeeper.HasBalance(suite.chainB.GetContext(), icaAddr, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)}) suite.Require().True(hasBalance) } + +// The safety of including SDK MsgResponses in the acknowledgement rests +// on the inclusion of the abcitypes.ResponseDeliverTx.Data in the +// abcitypes.ResposneDeliverTx hash. If the abcitypes.ResponseDeliverTx.Data +// gets removed from consensus they must no longer be used in the packet +// acknowledgement. +// +// This test acts as an indicator that the abcitypes.ResponseDeliverTx.Data +// may no longer be deterministic. +func (suite *InterchainAccountsTestSuite) TestABCICodeDeterminism() { + msgResponseBz, err := proto.Marshal(&channeltypes.MsgChannelOpenInitResponse{}) + suite.Require().NoError(err) + + msgData := &sdk.MsgData{ + MsgType: sdk.MsgTypeURL(&channeltypes.MsgChannelOpenInit{}), + Data: msgResponseBz, + } + + txResponse, err := proto.Marshal(&sdk.TxMsgData{ + Data: []*sdk.MsgData{msgData}, + }) + suite.Require().NoError(err) + + deliverTx := abcitypes.ResponseDeliverTx{ + Data: txResponse, + } + responses := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTx, + }, + } + + differentMsgResponseBz, err := proto.Marshal(&channeltypes.MsgRecvPacketResponse{}) + suite.Require().NoError(err) + + differentMsgData := &sdk.MsgData{ + MsgType: sdk.MsgTypeURL(&channeltypes.MsgRecvPacket{}), + Data: differentMsgResponseBz, + } + + differentTxResponse, err := proto.Marshal(&sdk.TxMsgData{ + Data: []*sdk.MsgData{differentMsgData}, + }) + suite.Require().NoError(err) + + differentDeliverTx := abcitypes.ResponseDeliverTx{ + Data: differentTxResponse, + } + + differentResponses := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &differentDeliverTx, + }, + } + + hash := tmstate.ABCIResponsesResultsHash(&responses) + differentHash := tmstate.ABCIResponsesResultsHash(&differentResponses) + + suite.Require().NotEqual(hash, differentHash) +} diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay.go b/modules/apps/27-interchain-accounts/host/keeper/relay.go index 803045794ad..8610a50f5e5 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay.go @@ -3,66 +3,89 @@ package keeper import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/gogo/protobuf/proto" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ) -// OnRecvPacket handles a given interchain accounts packet on a destination host chain -func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error { +// OnRecvPacket handles a given interchain accounts packet on a destination host chain. +// If the transaction is successfully executed, the transaction response bytes will be returned. +func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) ([]byte, error) { var data icatypes.InterchainAccountPacketData if err := icatypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { // UnmarshalJSON errors are indeterminate and therefore are not wrapped and included in failed acks - return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain account packet data") + return nil, sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain account packet data") } switch data.Type { case icatypes.EXECUTE_TX: msgs, err := icatypes.DeserializeCosmosTx(k.cdc, data.Data) if err != nil { - return err + return nil, err } - if err = k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs); err != nil { - return err + txResponse, err := k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs) + if err != nil { + return nil, err } - return nil + return txResponse, nil default: - return icatypes.ErrUnknownDataType + return nil, icatypes.ErrUnknownDataType } } -func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) error { +// executeTx attempts to execute the provided transaction. It begins by authenticating the transaction signer. +// If authentication succeeds, it does basic validation of the messages before attempting to deliver each message +// into state. The state changes will only be committed if all messages in the transaction succeed. Thus the +// execution of the transaction is atomic, all state changes are reverted if a single message fails. +func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) ([]byte, error) { channel, found := k.channelKeeper.GetChannel(ctx, destPort, destChannel) if !found { - return channeltypes.ErrChannelNotFound + return nil, channeltypes.ErrChannelNotFound } if err := k.authenticateTx(ctx, msgs, channel.ConnectionHops[0], sourcePort); err != nil { - return err + return nil, err + } + + txMsgData := &sdk.TxMsgData{ + Data: make([]*sdk.MsgData, len(msgs)), } // CacheContext returns a new context with the multi-store branched into a cached storage object // writeCache is called only if all msgs succeed, performing state transitions atomically cacheCtx, writeCache := ctx.CacheContext() - for _, msg := range msgs { + for i, msg := range msgs { if err := msg.ValidateBasic(); err != nil { - return err + return nil, err + } + + msgResponse, err := k.executeMsg(cacheCtx, msg) + if err != nil { + return nil, err } - if err := k.executeMsg(cacheCtx, msg); err != nil { - return err + txMsgData.Data[i] = &sdk.MsgData{ + MsgType: sdk.MsgTypeURL(msg), + Data: msgResponse, } + } // NOTE: The context returned by CacheContext() creates a new EventManager, so events must be correctly propagated back to the current context ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) writeCache() - return nil + txResponse, err := proto.Marshal(txMsgData) + if err != nil { + return nil, sdkerrors.Wrap(err, "failed to marshal tx data") + } + + return txResponse, nil } // authenticateTx ensures the provided msgs contain the correct interchain account signer address retrieved @@ -89,20 +112,21 @@ func (k Keeper) authenticateTx(ctx sdk.Context, msgs []sdk.Msg, connectionID, po return nil } -// Attempts to get the message handler from the router and if found will then execute the message -func (k Keeper) executeMsg(ctx sdk.Context, msg sdk.Msg) error { +// Attempts to get the message handler from the router and if found will then execute the message. +// If the message execution is successful, the proto marshaled message response will be returned. +func (k Keeper) executeMsg(ctx sdk.Context, msg sdk.Msg) ([]byte, error) { handler := k.msgRouter.Handler(msg) if handler == nil { - return icatypes.ErrInvalidRoute + return nil, icatypes.ErrInvalidRoute } res, err := handler(ctx, msg) if err != nil { - return err + return nil, err } // NOTE: The sdk msg handler creates a new EventManager, so events must be correctly propagated back to the current context ctx.EventManager().EmitEvents(res.GetEvents()) - return nil + return res.Data, nil } diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index f128804b415..fda31c34ef0 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -424,12 +424,14 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { 0, ) - err = suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) + txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) if tc.expPass { suite.Require().NoError(err) + suite.Require().NotNil(txResponse) } else { suite.Require().Error(err) + suite.Require().Nil(txResponse) } }) }