From a8058e269df59a6294d3ec6fbc1f50f572d27c9b Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 1 Mar 2022 13:03:34 +0100 Subject: [PATCH] chore: replace error string in transfer acks with const (backport #818) (#992) * 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 (cherry picked from commit ac46ac06084f586a460b092b2b293a321b7c43d6) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/types/ack.go # modules/apps/transfer/ibc_module.go * fix merge conflicts * fix tests Co-authored-by: Damian Nolan Co-authored-by: Carlos Rodriguez Co-authored-by: Carlos Rodriguez --- CHANGELOG.md | 4 + modules/apps/transfer/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 + 5 files changed, 135 insertions(+), 1 deletion(-) 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 c9fde910c4f..960f63aa123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#851](https://github.com/cosmos/ibc-go/pull/851) Bump SDK version to v0.45.1 * [\#948](https://github.com/cosmos/ibc-go/pull/948) Bump ics23/go to v0.7 +### 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. + ### Features * [\#679](https://github.com/cosmos/ibc-go/pull/679) New CLI command `query ibc-transfer denom-hash ` to get the denom hash for a denom trace; this might be useful for debug diff --git a/modules/apps/transfer/module.go b/modules/apps/transfer/module.go index 23a8afd02e7..a2a7a32a6b9 100644 --- a/modules/apps/transfer/module.go +++ b/modules/apps/transfer/module.go @@ -337,7 +337,7 @@ func (am AppModule) OnRecvPacket( if ack.Success() { err := am.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..6b928898001 --- /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/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..e3a689dd3e3 --- /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/modules/apps/transfer/types" + ibctesting "github.com/cosmos/ibc-go/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(0)) + suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1)) +} + +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{