From b40dbc646c7191aaee12d3a70243f9b6f3f80919 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Tue, 28 Jun 2022 14:57:57 +0100 Subject: [PATCH 1/2] Consolidate usage of NewErrorAcknowledgement (#1565) --- CHANGELOG.md | 3 + docs/migrations/v3-to-v4.md | 4 + .../controller/ibc_middleware.go | 5 +- .../controller/keeper/events.go | 31 +++++++ .../27-interchain-accounts/host/ibc_module.go | 4 +- .../host/ibc_module_test.go | 2 +- .../host/keeper/events.go | 14 +-- .../27-interchain-accounts/host/types/ack.go | 27 ------ .../host/types/ack_test.go | 19 ----- .../27-interchain-accounts/types/events.go | 7 +- modules/apps/transfer/ibc_module.go | 27 ++++-- .../apps/transfer/keeper/mbt_relay_test.go | 2 +- modules/apps/transfer/keeper/relay_test.go | 2 +- modules/apps/transfer/types/ack.go | 27 ------ modules/apps/transfer/types/ack_test.go | 71 ---------------- .../core/04-channel/types/acknowledgement.go | 15 +++- .../04-channel/types/acknowledgement_test.go | 85 +++++++++++++++++-- testing/mock/mock.go | 3 +- 18 files changed, 173 insertions(+), 175 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/events.go delete mode 100644 modules/apps/27-interchain-accounts/host/types/ack.go delete mode 100644 modules/apps/transfer/types/ack.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 281715f19ca..900ef224f5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`. * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`. * (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts. +* (apps/27-interchain-accounts)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. +* (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. +* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of non-deterministic writes to application state. ### State Machine Breaking diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index 1cd08f5bd49..903c027ce4b 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -26,6 +26,10 @@ This is an API breaking change and as such IBC application developers will have The `OnChanOpenInit` application callback has been modified. The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629). +The `NewErrorAcknowledgement` method signature has changed. +It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes. +All error acknowledgements now contain a deterministic ABCI code and error message. It is the responsibility of the application developer to emit error details in events. + ### ICS27 - Interchain Accounts The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets. diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index b7c65874bfa..cbcafbd248c 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -138,7 +138,10 @@ func (im IBCMiddleware) OnRecvPacket( packet channeltypes.Packet, _ sdk.AccAddress, ) ibcexported.Acknowledgement { - return channeltypes.NewErrorAcknowledgement("cannot receive packet on controller chain") + err := sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot receive packet on controller chain") + ack := channeltypes.NewErrorAcknowledgement(err) + keeper.EmitAcknowledgementEvent(ctx, packet, ack, err) + return ack } // OnAcknowledgementPacket implements the IBCMiddleware interface diff --git a/modules/apps/27-interchain-accounts/controller/keeper/events.go b/modules/apps/27-interchain-accounts/controller/keeper/events.go new file mode 100644 index 00000000000..3c820c74cb0 --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/events.go @@ -0,0 +1,31 @@ +package keeper + +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + + icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" + "github.com/cosmos/ibc-go/v3/modules/core/exported" +) + +// EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error +// details if any. +func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) { + attributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), + sdk.NewAttribute(icatypes.AttributeKeyControllerChannelID, packet.GetDestChannel()), + sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + } + + if err != nil { + attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error())) + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + icatypes.EventTypePacket, + attributes..., + ), + ) +} diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 2e1ba8e0805..6e77c464afe 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -106,13 +106,13 @@ func (im IBCModule) OnRecvPacket( _ sdk.AccAddress, ) ibcexported.Acknowledgement { if !im.keeper.IsHostEnabled(ctx) { - return types.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled) + return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled) } txResponse, err := im.keeper.OnRecvPacket(ctx, packet) ack := channeltypes.NewResultAcknowledgement(txResponse) if err != nil { - ack = types.NewErrorAcknowledgement(err) + ack = channeltypes.NewErrorAcknowledgement(err) } // Emit an event indicating a successful or failed acknowledgement. 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 78a21cfee01..ea602e89b4a 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -403,7 +403,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnRecvPacket = func( ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress, ) exported.Acknowledgement { - return channeltypes.NewErrorAcknowledgement("failed OnRecvPacket mock callback") + return channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed OnRecvPacket mock callback")) } }, true, }, diff --git a/modules/apps/27-interchain-accounts/host/keeper/events.go b/modules/apps/27-interchain-accounts/host/keeper/events.go index 116fb1cc3f7..2429acf18a3 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/events.go +++ b/modules/apps/27-interchain-accounts/host/keeper/events.go @@ -10,18 +10,20 @@ import ( // EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error // details if any. func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) { - var errorMsg string + attributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), + sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), + sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + } + if err != nil { - errorMsg = err.Error() + attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error())) } ctx.EventManager().EmitEvent( sdk.NewEvent( icatypes.EventTypePacket, - sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), - sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg), - sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), - sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + attributes..., ), ) } diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go deleted file mode 100644 index 202404fff3a..00000000000 --- a/modules/apps/27-interchain-accounts/host/types/ack.go +++ /dev/null @@ -1,27 +0,0 @@ -package types - -import ( - "fmt" - - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - - channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" -) - -const ( - // ackErrorString defines a string constant included in error acknowledgements - // NOTE: Changing this const is state machine breaking as acknowledgements are written into state - ackErrorString = "error handling packet on host chain: see events for details" -) - -// NewErrorAcknowledgement returns a deterministic error string which may be used in -// the packet acknowledgement. -func NewErrorAcknowledgement(err error) channeltypes.Acknowledgement { - // the ABCI code is included in the abcitypes.ResponseDeliverTx hash - // constructed in Tendermint and is therefore determinstic - _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-deterministic codespace and log values - - errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) - - return channeltypes.NewErrorAcknowledgement(errorString) -} diff --git a/modules/apps/27-interchain-accounts/host/types/ack_test.go b/modules/apps/27-interchain-accounts/host/types/ack_test.go index 6d2e6d1a95a..06cab3e0cc0 100644 --- a/modules/apps/27-interchain-accounts/host/types/ack_test.go +++ b/modules/apps/27-interchain-accounts/host/types/ack_test.go @@ -9,7 +9,6 @@ import ( 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" ibctesting "github.com/cosmos/ibc-go/v3/testing" ) @@ -80,21 +79,3 @@ func (suite *TypesTestSuite) TestABCICodeDeterminism() { suite.Require().Equal(hash, hashSameABCICode) suite.Require().NotEqual(hash, hashDifferentABCICode) } - -// TestAcknowledgementError will verify that only a constant string and -// ABCI error code are used in constructing the acknowledgement error string -func (suite *TypesTestSuite) TestAcknowledgementError() { - // same ABCI error code used - err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") - errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") - - // different ABCI error code used - errDifferentABCICode := sdkerrors.ErrNotFound - - ack := types.NewErrorAcknowledgement(err) - ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode) - ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode) - - suite.Require().Equal(ack, ackSameABCICode) - suite.Require().NotEqual(ack, ackDifferentABCICode) -} diff --git a/modules/apps/27-interchain-accounts/types/events.go b/modules/apps/27-interchain-accounts/types/events.go index 9bfd1df3049..3ca4b783b55 100644 --- a/modules/apps/27-interchain-accounts/types/events.go +++ b/modules/apps/27-interchain-accounts/types/events.go @@ -4,7 +4,8 @@ package types const ( EventTypePacket = "ics27_packet" - AttributeKeyAckError = "error" - AttributeKeyHostChannelID = "host_channel_id" - AttributeKeyAckSuccess = "success" + AttributeKeyAckError = "error" + AttributeKeyHostChannelID = "host_channel_id" + AttributeKeyControllerChannelID = "controller_channel_id" + AttributeKeyAckSuccess = "success" ) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 07469622bfc..d79ec986aab 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -9,6 +9,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" "github.com/cosmos/ibc-go/v3/modules/apps/transfer/keeper" "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" @@ -178,8 +179,10 @@ func (im IBCModule) OnRecvPacket( ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) var data types.FungibleTokenPacketData + var ackErr error if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data") + ackErr = sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot unmarshal ICS-20 transfer packet data") + ack = channeltypes.NewErrorAcknowledgement(ackErr) } // only attempt the application logic if the packet data @@ -187,19 +190,27 @@ func (im IBCModule) OnRecvPacket( if ack.Success() { err := im.keeper.OnRecvPacket(ctx, packet, data) if err != nil { - ack = types.NewErrorAcknowledgement(err) + ack = channeltypes.NewErrorAcknowledgement(err) + ackErr = err } } + eventAttributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), + sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver), + sdk.NewAttribute(types.AttributeKeyDenom, data.Denom), + sdk.NewAttribute(types.AttributeKeyAmount, data.Amount), + sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + } + + if ackErr != nil { + eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error())) + } + ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypePacket, - sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), - sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), - sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver), - sdk.NewAttribute(types.AttributeKeyDenom, data.Denom), - sdk.NewAttribute(types.AttributeKeyAmount, data.Amount), - sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + eventAttributes..., ), ) diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/modules/apps/transfer/keeper/mbt_relay_test.go index 59e4869e54e..4366101f932 100644 --- a/modules/apps/transfer/keeper/mbt_relay_test.go +++ b/modules/apps/transfer/keeper/mbt_relay_test.go @@ -360,7 +360,7 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() { registerDenom() err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket( suite.chainB.GetContext(), packet, tc.packet.Data, - channeltypes.NewErrorAcknowledgement("MBT Error Acknowledgement")) + channeltypes.NewErrorAcknowledgement(fmt.Errorf("MBT Error Acknowledgement"))) default: err = fmt.Errorf("Unknown handler: %s", tc.handler) } diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index 3ad851b587e..d9d6f8a68c7 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -255,7 +255,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { var ( successAck = channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - failedAck = channeltypes.NewErrorAcknowledgement("failed packet transfer") + failedAck = channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed packet transfer")) trace types.DenomTrace amount sdk.Int path *ibctesting.Path diff --git a/modules/apps/transfer/types/ack.go b/modules/apps/transfer/types/ack.go deleted file mode 100644 index 6512f2e8371..00000000000 --- a/modules/apps/transfer/types/ack.go +++ /dev/null @@ -1,27 +0,0 @@ -package types - -import ( - "fmt" - - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - - channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" -) - -const ( - // ackErrorString defines a string constant included in error acknowledgements - // NOTE: Changing this const is state machine breaking as acknowledgements are written into state - ackErrorString = "error handling packet on destination chain: see events for details" -) - -// NewErrorAcknowledgement returns a deterministic error string which may be used in -// the packet acknowledgement. -func NewErrorAcknowledgement(err error) channeltypes.Acknowledgement { - // the ABCI code is included in the abcitypes.ResponseDeliverTx hash - // constructed in Tendermint and is therefore deterministic - _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values - - errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) - - return channeltypes.NewErrorAcknowledgement(errorString) -} diff --git a/modules/apps/transfer/types/ack_test.go b/modules/apps/transfer/types/ack_test.go index ffc09da981e..2fc13a3290e 100644 --- a/modules/apps/transfer/types/ack_test.go +++ b/modules/apps/transfer/types/ack_test.go @@ -3,21 +3,11 @@ package types_test import ( "testing" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/stretchr/testify/suite" - abcitypes "github.com/tendermint/tendermint/abci/types" - tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" - tmstate "github.com/tendermint/tendermint/state" - "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" ) -const ( - gasUsed = uint64(100) - gasWanted = uint64(100) -) - type TypesTestSuite struct { suite.Suite @@ -37,64 +27,3 @@ func (suite *TypesTestSuite) SetupTest() { func TestTypesTestSuite(t *testing.T) { suite.Run(t, new(TypesTestSuite)) } - -// The safety of including ABCI error codes in the acknowledgement rests -// on the inclusion of these ABCI error codes in the abcitypes.ResposneDeliverTx -// hash. If the ABCI codes get removed from consensus they must no longer be used -// in the packet acknowledgement. -// -// This test acts as an indicator that the ABCI error codes may no longer be deterministic. -func (suite *TypesTestSuite) TestABCICodeDeterminism() { - // same ABCI error code used - err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") - errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") - - // different ABCI error code used - errDifferentABCICode := sdkerrors.ErrNotFound - - deliverTx := sdkerrors.ResponseDeliverTx(err, gasUsed, gasWanted, false) - responses := tmprotostate.ABCIResponses{ - DeliverTxs: []*abcitypes.ResponseDeliverTx{ - &deliverTx, - }, - } - - deliverTxSameABCICode := sdkerrors.ResponseDeliverTx(errSameABCICode, gasUsed, gasWanted, false) - responsesSameABCICode := tmprotostate.ABCIResponses{ - DeliverTxs: []*abcitypes.ResponseDeliverTx{ - &deliverTxSameABCICode, - }, - } - - deliverTxDifferentABCICode := sdkerrors.ResponseDeliverTx(errDifferentABCICode, gasUsed, gasWanted, false) - responsesDifferentABCICode := tmprotostate.ABCIResponses{ - DeliverTxs: []*abcitypes.ResponseDeliverTx{ - &deliverTxDifferentABCICode, - }, - } - - hash := tmstate.ABCIResponsesResultsHash(&responses) - hashSameABCICode := tmstate.ABCIResponsesResultsHash(&responsesSameABCICode) - hashDifferentABCICode := tmstate.ABCIResponsesResultsHash(&responsesDifferentABCICode) - - suite.Require().Equal(hash, hashSameABCICode) - suite.Require().NotEqual(hash, hashDifferentABCICode) -} - -// TestAcknowledgementError will verify that only a constant string and -// ABCI error code are used in constructing the acknowledgement error string -func (suite *TypesTestSuite) TestAcknowledgementError() { - // same ABCI error code used - err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") - errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") - - // different ABCI error code used - errDifferentABCICode := sdkerrors.ErrNotFound - - ack := types.NewErrorAcknowledgement(err) - ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode) - ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode) - - suite.Require().Equal(ack, ackSameABCICode) - suite.Require().NotEqual(ack, ackDifferentABCICode) -} diff --git a/modules/core/04-channel/types/acknowledgement.go b/modules/core/04-channel/types/acknowledgement.go index b46de2b981d..49c795d0d55 100644 --- a/modules/core/04-channel/types/acknowledgement.go +++ b/modules/core/04-channel/types/acknowledgement.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "reflect" "strings" @@ -8,6 +9,12 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) +const ( + // ackErrorString defines a string constant included in error acknowledgements + // NOTE: Changing this const is state machine breaking as acknowledgements are written into state. + ackErrorString = "error handling packet: see events for details" +) + // NewResultAcknowledgement returns a new instance of Acknowledgement using an Acknowledgement_Result // type in the Response field. func NewResultAcknowledgement(result []byte) Acknowledgement { @@ -22,10 +29,14 @@ func NewResultAcknowledgement(result []byte) Acknowledgement { // type in the Response field. // NOTE: Acknowledgements are written into state and thus, changes made to error strings included in packet acknowledgements // risk an app hash divergence when nodes in a network are running different patch versions of software. -func NewErrorAcknowledgement(err string) Acknowledgement { +func NewErrorAcknowledgement(err error) Acknowledgement { + // the ABCI code is included in the abcitypes.ResponseDeliverTx hash + // constructed in Tendermint and is therefore deterministic + _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values + return Acknowledgement{ Response: &Acknowledgement_Error{ - Error: err, + Error: fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString), }, } } diff --git a/modules/core/04-channel/types/acknowledgement_test.go b/modules/core/04-channel/types/acknowledgement_test.go index 658ff31a8b5..530d288bb32 100644 --- a/modules/core/04-channel/types/acknowledgement_test.go +++ b/modules/core/04-channel/types/acknowledgement_test.go @@ -1,6 +1,20 @@ package types_test -import "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" +import ( + "fmt" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + abcitypes "github.com/tendermint/tendermint/abci/types" + tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" + tmstate "github.com/tendermint/tendermint/state" + + "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" +) + +const ( + gasUsed = uint64(100) + gasWanted = uint64(100) +) // tests acknowledgement.ValidateBasic and acknowledgement.GetBytes func (suite TypesTestSuite) TestAcknowledgement() { @@ -18,7 +32,7 @@ func (suite TypesTestSuite) TestAcknowledgement() { }, { "valid failed ack", - types.NewErrorAcknowledgement("error"), + types.NewErrorAcknowledgement(fmt.Errorf("error")), false, true, }, @@ -29,10 +43,10 @@ func (suite TypesTestSuite) TestAcknowledgement() { false, }, { - "empty faied ack", - types.NewErrorAcknowledgement(" "), - false, + "empty failed ack", + types.NewErrorAcknowledgement(fmt.Errorf(" ")), false, + true, }, { "nil response", @@ -68,3 +82,64 @@ func (suite TypesTestSuite) TestAcknowledgement() { }) } } + +// The safety of including ABCI error codes in the acknowledgement rests +// on the inclusion of these ABCI error codes in the abcitypes.ResposneDeliverTx +// hash. If the ABCI codes get removed from consensus they must no longer be used +// in the packet acknowledgement. +// +// This test acts as an indicator that the ABCI error codes may no longer be deterministic. +func (suite *TypesTestSuite) TestABCICodeDeterminism() { + // same ABCI error code used + err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") + errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") + + // different ABCI error code used + errDifferentABCICode := sdkerrors.ErrNotFound + + deliverTx := sdkerrors.ResponseDeliverTx(err, gasUsed, gasWanted, false) + responses := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTx, + }, + } + + deliverTxSameABCICode := sdkerrors.ResponseDeliverTx(errSameABCICode, gasUsed, gasWanted, false) + responsesSameABCICode := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTxSameABCICode, + }, + } + + deliverTxDifferentABCICode := sdkerrors.ResponseDeliverTx(errDifferentABCICode, gasUsed, gasWanted, false) + responsesDifferentABCICode := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTxDifferentABCICode, + }, + } + + hash := tmstate.ABCIResponsesResultsHash(&responses) + hashSameABCICode := tmstate.ABCIResponsesResultsHash(&responsesSameABCICode) + hashDifferentABCICode := tmstate.ABCIResponsesResultsHash(&responsesDifferentABCICode) + + suite.Require().Equal(hash, hashSameABCICode) + suite.Require().NotEqual(hash, hashDifferentABCICode) +} + +// TestAcknowledgementError will verify that only a constant string and +// ABCI error code are used in constructing the acknowledgement error string +func (suite *TypesTestSuite) TestAcknowledgementError() { + // same ABCI error code used + err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") + errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") + + // different ABCI error code used + errDifferentABCICode := sdkerrors.ErrNotFound + + ack := types.NewErrorAcknowledgement(err) + ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode) + ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode) + + suite.Require().Equal(ack, ackSameABCICode) + suite.Require().NotEqual(ack, ackDifferentABCICode) +} diff --git a/testing/mock/mock.go b/testing/mock/mock.go index b621a05e9f7..a4ac465353d 100644 --- a/testing/mock/mock.go +++ b/testing/mock/mock.go @@ -2,6 +2,7 @@ package mock import ( "encoding/json" + "fmt" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" @@ -28,7 +29,7 @@ const ( var ( MockAcknowledgement = channeltypes.NewResultAcknowledgement([]byte("mock acknowledgement")) - MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement("mock failed acknowledgement") + MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement(fmt.Errorf("mock failed acknowledgement")) MockPacketData = []byte("mock packet data") MockFailPacketData = []byte("mock failed packet data") MockAsyncPacketData = []byte("mock async packet data") From f283d1241a11967ab2fb86b3db83905c47fd6153 Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 28 Jun 2022 22:04:41 +0200 Subject: [PATCH 2/2] docs: adding line about module accounts / invariants (#1597) * docs: adding line about module accounts / invariants * Update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: Damian Nolan * Update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: Damian Nolan Co-authored-by: Damian Nolan --- docs/middleware/ics29-fee/fee-distribution.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/middleware/ics29-fee/fee-distribution.md b/docs/middleware/ics29-fee/fee-distribution.md index 0ca1bce921f..9cf3a821a21 100644 --- a/docs/middleware/ics29-fee/fee-distribution.md +++ b/docs/middleware/ics29-fee/fee-distribution.md @@ -27,6 +27,8 @@ If a counterparty payee is not registered for the forward relayer on the destina A transaction must be submitted to the destination chain including a `CounterpartyPayee` address of an account on the source chain. The transaction must be signed by the `Relayer`. +Note: If a module account address is used as the `CounterpartyPayee` it is recommended to [turn off invariant checks](https://github.com/cosmos/ibc-go/blob/71d7480c923f4227453e8a80f51be01ae7ee845e/testing/simapp/app.go#L659) for that module. + ```go type MsgRegisterCounterpartyPayee struct { // unique port identifier @@ -63,6 +65,8 @@ If a payee is not registered for the reverse or timeout relayer on the source ch A transaction must be submitted to the source chain including a `Payee` address of an account on the source chain. The transaction must be signed by the `Relayer`. +Note: If a module account address is used as the `Payee` it is recommended to [turn off invariant checks](https://github.com/cosmos/ibc-go/blob/71d7480c923f4227453e8a80f51be01ae7ee845e/testing/simapp/app.go#L659) for that module. + ```go type MsgRegisterPayee struct { // unique port identifier