Skip to content

Commit 46ba51c

Browse files
mergify[bot]damiannolancrodriguezvega
authored
chore: replace error string in transfer acks with const (backport #818) (#993)
* 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 ac46ac0) # Conflicts: # modules/apps/27-interchain-accounts/host/types/ack.go # modules/apps/transfer/ibc_module.go * fix merge conflicts * fix changelog Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
1 parent 1ff6913 commit 46ba51c

File tree

5 files changed

+135
-1
lines changed

5 files changed

+135
-1
lines changed

‎CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
4141
* [\#851](https://github.com/cosmos/ibc-go/pull/851) Bump SDK version to v0.45.1
4242
* [\#948](https://github.com/cosmos/ibc-go/pull/948) Bump ics23/go to v0.7
4343

44+
### State Machine Breaking
45+
46+
* (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.
47+
4448
### Features
4549

4650
* [\#679](https://github.com/cosmos/ibc-go/pull/679) New CLI command `query ibc-transfer denom-hash <denom trace>` to get the denom hash for a denom trace; this might be useful for debug

‎modules/apps/transfer/module.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ func (am AppModule) OnRecvPacket(
337337
if ack.Success() {
338338
err := am.keeper.OnRecvPacket(ctx, packet, data)
339339
if err != nil {
340-
ack = channeltypes.NewErrorAcknowledgement(err.Error())
340+
ack = types.NewErrorAcknowledgement(err)
341341
}
342342
}
343343

‎modules/apps/transfer/types/ack.go

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package types
2+
3+
import (
4+
"fmt"
5+
6+
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
7+
8+
channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
9+
)
10+
11+
const (
12+
// ackErrorString defines a string constant included in error acknowledgements
13+
// NOTE: Changing this const is state machine breaking as acknowledgements are written into state
14+
ackErrorString = "error handling packet on destination chain: see events for details"
15+
)
16+
17+
// NewErrorAcknowledgement returns a deterministic error string which may be used in
18+
// the packet acknowledgement.
19+
func NewErrorAcknowledgement(err error) channeltypes.Acknowledgement {
20+
// the ABCI code is included in the abcitypes.ResponseDeliverTx hash
21+
// constructed in Tendermint and is therefore deterministic
22+
_, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values
23+
24+
errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString)
25+
26+
return channeltypes.NewErrorAcknowledgement(errorString)
27+
}

‎modules/apps/transfer/types/ack_test.go

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package types_test
2+
3+
import (
4+
"testing"
5+
6+
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
7+
"github.com/stretchr/testify/suite"
8+
abcitypes "github.com/tendermint/tendermint/abci/types"
9+
tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state"
10+
tmstate "github.com/tendermint/tendermint/state"
11+
12+
"github.com/cosmos/ibc-go/v2/modules/apps/transfer/types"
13+
ibctesting "github.com/cosmos/ibc-go/v2/testing"
14+
)
15+
16+
const (
17+
gasUsed = uint64(100)
18+
gasWanted = uint64(100)
19+
)
20+
21+
type TypesTestSuite struct {
22+
suite.Suite
23+
24+
coordinator *ibctesting.Coordinator
25+
26+
chainA *ibctesting.TestChain
27+
chainB *ibctesting.TestChain
28+
}
29+
30+
func (suite *TypesTestSuite) SetupTest() {
31+
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
32+
33+
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
34+
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
35+
}
36+
37+
func TestTypesTestSuite(t *testing.T) {
38+
suite.Run(t, new(TypesTestSuite))
39+
}
40+
41+
// The safety of including ABCI error codes in the acknowledgement rests
42+
// on the inclusion of these ABCI error codes in the abcitypes.ResposneDeliverTx
43+
// hash. If the ABCI codes get removed from consensus they must no longer be used
44+
// in the packet acknowledgement.
45+
//
46+
// This test acts as an indicator that the ABCI error codes may no longer be deterministic.
47+
func (suite *TypesTestSuite) TestABCICodeDeterminism() {
48+
// same ABCI error code used
49+
err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1")
50+
errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2")
51+
52+
// different ABCI error code used
53+
errDifferentABCICode := sdkerrors.ErrNotFound
54+
55+
deliverTx := sdkerrors.ResponseDeliverTx(err, gasUsed, gasWanted, false)
56+
responses := tmprotostate.ABCIResponses{
57+
DeliverTxs: []*abcitypes.ResponseDeliverTx{
58+
&deliverTx,
59+
},
60+
}
61+
62+
deliverTxSameABCICode := sdkerrors.ResponseDeliverTx(errSameABCICode, gasUsed, gasWanted, false)
63+
responsesSameABCICode := tmprotostate.ABCIResponses{
64+
DeliverTxs: []*abcitypes.ResponseDeliverTx{
65+
&deliverTxSameABCICode,
66+
},
67+
}
68+
69+
deliverTxDifferentABCICode := sdkerrors.ResponseDeliverTx(errDifferentABCICode, gasUsed, gasWanted, false)
70+
responsesDifferentABCICode := tmprotostate.ABCIResponses{
71+
DeliverTxs: []*abcitypes.ResponseDeliverTx{
72+
&deliverTxDifferentABCICode,
73+
},
74+
}
75+
76+
hash := tmstate.ABCIResponsesResultsHash(&responses)
77+
hashSameABCICode := tmstate.ABCIResponsesResultsHash(&responsesSameABCICode)
78+
hashDifferentABCICode := tmstate.ABCIResponsesResultsHash(&responsesDifferentABCICode)
79+
80+
suite.Require().Equal(hash, hashSameABCICode)
81+
suite.Require().NotEqual(hash, hashDifferentABCICode)
82+
}
83+
84+
// TestAcknowledgementError will verify that only a constant string and
85+
// ABCI error code are used in constructing the acknowledgement error string
86+
func (suite *TypesTestSuite) TestAcknowledgementError() {
87+
// same ABCI error code used
88+
err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1")
89+
errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2")
90+
91+
// different ABCI error code used
92+
errDifferentABCICode := sdkerrors.ErrNotFound
93+
94+
ack := types.NewErrorAcknowledgement(err)
95+
ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode)
96+
ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode)
97+
98+
suite.Require().Equal(ack, ackSameABCICode)
99+
suite.Require().NotEqual(ack, ackDifferentABCICode)
100+
101+
}

‎modules/core/04-channel/types/acknowledgement.go

+2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ func NewResultAcknowledgement(result []byte) Acknowledgement {
2020

2121
// NewErrorAcknowledgement returns a new instance of Acknowledgement using an Acknowledgement_Error
2222
// type in the Response field.
23+
// NOTE: Acknowledgements are written into state and thus, changes made to error strings included in packet acknowledgements
24+
// risk an app hash divergence when nodes in a network are running different patch versions of software.
2325
func NewErrorAcknowledgement(err string) Acknowledgement {
2426
return Acknowledgement{
2527
Response: &Acknowledgement_Error{

0 commit comments

Comments
 (0)