From ac46ac06084f586a460b092b2b293a321b7c43d6 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 2 Feb 2022 14:56:14 +0100 Subject: [PATCH] chore: replace error string in transfer acks with const (#818) * fix: adding ack error string const for transfer * updating godoc * adding warning note to godoc in 04-channel * updating to include abci error code, and copy tests from ica * adding changelog entry --- CHANGELOG.md | 2 + .../27-interchain-accounts/host/types/ack.go | 4 +- modules/apps/transfer/ibc_module.go | 2 +- modules/apps/transfer/types/ack.go | 27 +++++ modules/apps/transfer/types/ack_test.go | 101 ++++++++++++++++++ .../core/04-channel/types/acknowledgement.go | 2 + 6 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 modules/apps/transfer/types/ack.go create mode 100644 modules/apps/transfer/types/ack_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 95c129cb08b..df182d73b6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* (transfer) [\#818](https://github.com/cosmos/ibc-go/pull/818) Error acknowledgements returned from Transfer `OnRecvPacket` now include a deterministic ABCI code and error message. + ### Improvements * (testing) [\#810](https://github.com/cosmos/ibc-go/pull/810) Additional testing function added to `Endpoint` type called `RecvPacketWithResult`. Performs the same functionality as the existing `RecvPacket` function but also returns the message result. `path.RelayPacket` no longer uses the provided acknowledgement argument and instead obtains the acknowledgement via MsgRecvPacket events. diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go index 047a7063e46..202404fff3a 100644 --- a/modules/apps/27-interchain-accounts/host/types/ack.go +++ b/modules/apps/27-interchain-accounts/host/types/ack.go @@ -14,12 +14,12 @@ const ( ackErrorString = "error handling packet on host chain: see events for details" ) -// AcknowledgementErrorString returns a deterministic error string which may be used in +// 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-determinstic codespace and log values + _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-deterministic codespace and log values errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 1ad67d16a85..26f1c533434 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -181,7 +181,7 @@ func (im IBCModule) OnRecvPacket( if ack.Success() { err := im.keeper.OnRecvPacket(ctx, packet, data) if err != nil { - ack = channeltypes.NewErrorAcknowledgement(err.Error()) + ack = types.NewErrorAcknowledgement(err) } } diff --git a/modules/apps/transfer/types/ack.go b/modules/apps/transfer/types/ack.go new file mode 100644 index 00000000000..6512f2e8371 --- /dev/null +++ b/modules/apps/transfer/types/ack.go @@ -0,0 +1,27 @@ +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 new file mode 100644 index 00000000000..bc4e2d07afc --- /dev/null +++ b/modules/apps/transfer/types/ack_test.go @@ -0,0 +1,101 @@ +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/27-interchain-accounts/host/types" + ibctesting "github.com/cosmos/ibc-go/v3/testing" +) + +const ( + gasUsed = uint64(100) + gasWanted = uint64(100) +) + +type TypesTestSuite struct { + suite.Suite + + coordinator *ibctesting.Coordinator + + chainA *ibctesting.TestChain + chainB *ibctesting.TestChain +} + +func (suite *TypesTestSuite) SetupTest() { + suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) + + suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) + suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) +} + +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 cfc088ab0c9..b46de2b981d 100644 --- a/modules/core/04-channel/types/acknowledgement.go +++ b/modules/core/04-channel/types/acknowledgement.go @@ -20,6 +20,8 @@ func NewResultAcknowledgement(result []byte) Acknowledgement { // NewErrorAcknowledgement returns a new instance of Acknowledgement using an Acknowledgement_Error // 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 { return Acknowledgement{ Response: &Acknowledgement_Error{