From 46ff2aea6f5e48b83e396a29c3db42fdc308ceb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 26 Jan 2022 11:59:13 +0100 Subject: [PATCH 1/3] refactor: construct ics27 error acknowledgement with determinstic ABCI code Move error acknowledgement construction to host types as it is only created/set on host chain Create test to ensure ABCI code remains determinstic Create test to ensure the error acknowledgement constructor only relies on ABCI code --- .../27-interchain-accounts/host/ibc_module.go | 2 +- .../27-interchain-accounts/host/types/ack.go | 27 +++++ .../host/types/ack_test.go | 101 ++++++++++++++++++ .../27-interchain-accounts/types/errors.go | 6 -- 4 files changed, 129 insertions(+), 7 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/host/types/ack.go create mode 100644 modules/apps/27-interchain-accounts/host/types/ack_test.go diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 15606eb9e74..cec21d258c3 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -111,7 +111,7 @@ func (im IBCModule) OnRecvPacket( ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) if err := im.keeper.OnRecvPacket(ctx, packet); err != nil { - ack = channeltypes.NewErrorAcknowledgement(icatypes.AcknowledgementError) + ack = types.NewErrorAcknowledgement(err) // Emit an event including the error msg keeper.EmitWriteErrorAcknowledgementEvent(ctx, packet, err) diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go new file mode 100644 index 00000000000..1fe8a800a1d --- /dev/null +++ b/modules/apps/27-interchain-accounts/host/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 host chain: see events for details" +) + +// AcknowledgementErrorString returns a determinstic 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 + + 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 new file mode 100644 index 00000000000..9e7841070b2 --- /dev/null +++ b/modules/apps/27-interchain-accounts/host/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(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/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index 3f2cd77d488..e0a5c141de9 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -4,12 +4,6 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -const ( - // AcknowledgementError defines a string constant included in error acknowledgements - // NOTE: Changing this const is state machine breaking as acknowledgements are written into state - AcknowledgementError = "error handling packet on host chain: see events for details" -) - var ( ErrUnknownDataType = sdkerrors.Register(ModuleName, 2, "unknown data type") ErrAccountAlreadyExist = sdkerrors.Register(ModuleName, 3, "account already exist") From 50f88586402996bc41eb57f6f4864dc59d50cf8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 28 Jan 2022 10:50:06 +0100 Subject: [PATCH 2/3] Update modules/apps/27-interchain-accounts/host/types/ack.go --- modules/apps/27-interchain-accounts/host/types/ack.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go index 1fe8a800a1d..047a7063e46 100644 --- a/modules/apps/27-interchain-accounts/host/types/ack.go +++ b/modules/apps/27-interchain-accounts/host/types/ack.go @@ -14,7 +14,7 @@ const ( ackErrorString = "error handling packet on host chain: see events for details" ) -// AcknowledgementErrorString returns a determinstic error string which may be used in +// AcknowledgementErrorString 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 From 7fed6e2a42f7d05e2d3de1d70c5e751b2c2ade96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 28 Jan 2022 11:36:19 +0100 Subject: [PATCH 3/3] fix test from merge conflict --- modules/apps/27-interchain-accounts/host/types/ack_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 9e7841070b2..bc4e2d07afc 100644 --- a/modules/apps/27-interchain-accounts/host/types/ack_test.go +++ b/modules/apps/27-interchain-accounts/host/types/ack_test.go @@ -30,8 +30,8 @@ type TypesTestSuite struct { 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)) + suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) + suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) } func TestTypesTestSuite(t *testing.T) {