Skip to content

Commit 4ecfe16

Browse files
colin-axnerAdityaSripal
colin-axner
and
AdityaSripal
authored
improve error messages, indicate already relayed packets (#184)
* improve error messages * changelog * update recvpacket errors and test * add acknowledge packet tests * update timeout tests Co-authored-by: Aditya <adityasripal@gmail.com>
1 parent 269164c commit 4ecfe16

File tree

9 files changed

+287
-27
lines changed

9 files changed

+287
-27
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
6868

6969
### Improvements
7070

71+
* (modules/core) [\#184](https://github.com/cosmos/ibc-go/pull/184) Improve error messages. Uses unique error codes to indicate already relayed packets.
7172
* (07-tendermint) [\#182](https://github.com/cosmos/ibc-go/pull/182) Remove duplicate checks in upgrade logic.
7273
* (modules/core/04-channel) [\#7949](https://github.com/cosmos/cosmos-sdk/issues/7949) Standardized channel `Acknowledgement` moved to its own file. Codec registration redundancy removed.
7374
* (modules/core/04-channel) [\#144](https://github.com/cosmos/ibc-go/pull/144) Introduced a `packet_data_hex` attribute to emit the hex-encoded packet data in events. This allows for raw binary (proto-encoded message) to be sent over events and decoded correctly on relayer. Original `packet_data` is DEPRECATED. All relayers and IBC event consumers are encouraged to switch to `packet_data_hex` as soon as possible.

‎modules/core/04-channel/keeper/packet.go

+17-5
Original file line numberDiff line numberDiff line change
@@ -241,8 +241,8 @@ func (k Keeper) RecvPacket(
241241
_, found := k.GetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
242242
if found {
243243
return sdkerrors.Wrapf(
244-
types.ErrInvalidPacket,
245-
"packet sequence (%d) already has been received", packet.GetSequence(),
244+
types.ErrPacketReceived,
245+
"packet sequence (%d)", packet.GetSequence(),
246246
)
247247
}
248248

@@ -262,9 +262,17 @@ func (k Keeper) RecvPacket(
262262
)
263263
}
264264

265+
// helpful error message for relayers
266+
if packet.GetSequence() < nextSequenceRecv {
267+
return sdkerrors.Wrapf(
268+
types.ErrPacketReceived,
269+
"packet sequence (%d), next sequence receive (%d)", packet.GetSequence(), nextSequenceRecv,
270+
)
271+
}
272+
265273
if packet.GetSequence() != nextSequenceRecv {
266274
return sdkerrors.Wrapf(
267-
types.ErrInvalidPacket,
275+
types.ErrPacketSequenceOutOfOrder,
268276
"packet sequence ≠ next receive sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceRecv,
269277
)
270278
}
@@ -462,6 +470,10 @@ func (k Keeper) AcknowledgePacket(
462470

463471
commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
464472

473+
if len(commitment) == 0 {
474+
return sdkerrors.Wrapf(types.ErrPacketCommitmentNotFound, "packet with sequence (%d) has been acknowledged, or timed out. In rare cases the packet was never sent or the packet sequence is incorrect", packet.GetSequence())
475+
}
476+
465477
packetCommitment := types.CommitPacket(k.cdc, packet)
466478

467479
// verify we sent the packet and haven't cleared it out yet
@@ -473,7 +485,7 @@ func (k Keeper) AcknowledgePacket(
473485
ctx, connectionEnd, proofHeight, proof, packet.GetDestPort(), packet.GetDestChannel(),
474486
packet.GetSequence(), acknowledgement,
475487
); err != nil {
476-
return sdkerrors.Wrap(err, "packet acknowledgement verification failed")
488+
return err
477489
}
478490

479491
// assert packets acknowledged in order
@@ -488,7 +500,7 @@ func (k Keeper) AcknowledgePacket(
488500

489501
if packet.GetSequence() != nextSequenceAck {
490502
return sdkerrors.Wrapf(
491-
sdkerrors.ErrInvalidSequence,
503+
types.ErrPacketSequenceOutOfOrder,
492504
"packet sequence ≠ next ack sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceAck,
493505
)
494506
}

‎modules/core/04-channel/keeper/packet_test.go

+176-12
Large diffs are not rendered by default.

‎modules/core/04-channel/keeper/timeout.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ func (k Keeper) TimeoutPacket(
8080

8181
commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
8282

83+
if len(commitment) == 0 {
84+
return sdkerrors.Wrapf(types.ErrPacketCommitmentNotFound, "packet with sequence (%d) has been acknowledged or timed out. In rare cases the packet was never sent or the packet sequence is incorrect", packet.GetSequence())
85+
}
86+
8387
packetCommitment := types.CommitPacket(k.cdc, packet)
8488

8589
// verify we sent the packet and haven't cleared it out yet
@@ -92,7 +96,7 @@ func (k Keeper) TimeoutPacket(
9296
// check that packet has not been received
9397
if nextSequenceRecv > packet.GetSequence() {
9498
return sdkerrors.Wrapf(
95-
types.ErrInvalidPacket,
99+
types.ErrPacketReceived,
96100
"packet already received, next sequence receive > packet sequence (%d > %d)", nextSequenceRecv, packet.GetSequence(),
97101
)
98102
}

‎modules/core/04-channel/keeper/timeout_test.go

+50
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
package keeper_test
22

33
import (
4+
"errors"
45
"fmt"
56

7+
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
68
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
9+
710
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
11+
connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types"
812
"github.com/cosmos/ibc-go/modules/core/04-channel/types"
913
host "github.com/cosmos/ibc-go/modules/core/24-host"
1014
"github.com/cosmos/ibc-go/modules/core/exported"
@@ -20,6 +24,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
2024
packet types.Packet
2125
nextSeqRecv uint64
2226
ordered bool
27+
expError *sdkerrors.Error
2328
)
2429

2530
testCases := []testCase{
@@ -42,29 +47,61 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
4247
// need to update chainA's client representing chainB to prove missing ack
4348
path.EndpointA.UpdateClient()
4449
}, true},
50+
{"packet already timed out: ORDERED", func() {
51+
expError = types.ErrInvalidChannelState
52+
ordered = true
53+
path.SetChannelOrdered()
54+
55+
suite.coordinator.Setup(path)
56+
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.GetSelfHeight(suite.chainB.GetContext()), uint64(suite.chainB.GetContext().BlockTime().UnixNano()))
57+
path.EndpointA.SendPacket(packet)
58+
// need to update chainA's client representing chainB to prove missing ack
59+
path.EndpointA.UpdateClient()
60+
61+
err := path.EndpointA.TimeoutPacket(packet)
62+
suite.Require().NoError(err)
63+
}, false},
64+
{"packet already timed out: UNORDERED", func() {
65+
expError = types.ErrPacketCommitmentNotFound
66+
ordered = false
67+
68+
suite.coordinator.Setup(path)
69+
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.GetSelfHeight(suite.chainB.GetContext()), disabledTimeoutTimestamp)
70+
path.EndpointA.SendPacket(packet)
71+
// need to update chainA's client representing chainB to prove missing ack
72+
path.EndpointA.UpdateClient()
73+
74+
err := path.EndpointA.TimeoutPacket(packet)
75+
suite.Require().NoError(err)
76+
}, false},
4577
{"channel not found", func() {
78+
expError = types.ErrChannelNotFound
4679
// use wrong channel naming
4780
suite.coordinator.Setup(path)
4881
packet = types.NewPacket(ibctesting.MockPacketData, 1, ibctesting.InvalidID, ibctesting.InvalidID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
4982
}, false},
5083
{"channel not open", func() {
84+
expError = types.ErrInvalidChannelState
5185
suite.coordinator.Setup(path)
5286
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
5387

5488
err := path.EndpointA.SetChannelClosed()
5589
suite.Require().NoError(err)
5690
}, false},
5791
{"packet destination port ≠ channel counterparty port", func() {
92+
expError = types.ErrInvalidPacket
5893
suite.coordinator.Setup(path)
5994
// use wrong port for dest
6095
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ibctesting.InvalidID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
6196
}, false},
6297
{"packet destination channel ID ≠ channel counterparty channel ID", func() {
98+
expError = types.ErrInvalidPacket
6399
suite.coordinator.Setup(path)
64100
// use wrong channel for dest
65101
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, ibctesting.InvalidID, timeoutHeight, disabledTimeoutTimestamp)
66102
}, false},
67103
{"connection not found", func() {
104+
expError = connectiontypes.ErrConnectionNotFound
68105
// pass channel check
69106
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel(
70107
suite.chainA.GetContext(),
@@ -74,12 +111,14 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
74111
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
75112
}, false},
76113
{"timeout", func() {
114+
expError = types.ErrPacketTimeout
77115
suite.coordinator.Setup(path)
78116
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
79117
path.EndpointA.SendPacket(packet)
80118
path.EndpointA.UpdateClient()
81119
}, false},
82120
{"packet already received ", func() {
121+
expError = types.ErrPacketReceived
83122
ordered = true
84123
path.SetChannelOrdered()
85124

@@ -91,6 +130,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
91130
path.EndpointA.UpdateClient()
92131
}, false},
93132
{"packet hasn't been sent", func() {
133+
expError = types.ErrPacketCommitmentNotFound
94134
ordered = true
95135
path.SetChannelOrdered()
96136

@@ -99,6 +139,8 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
99139
path.EndpointA.UpdateClient()
100140
}, false},
101141
{"next seq receive verification failed", func() {
142+
// skip error check, error occurs in light-clients
143+
102144
// set ordered to false resulting in wrong proof provided
103145
ordered = false
104146

@@ -110,6 +152,8 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
110152
path.EndpointA.UpdateClient()
111153
}, false},
112154
{"packet ack verification failed", func() {
155+
// skip error check, error occurs in light-clients
156+
113157
// set ordered to true resulting in wrong proof provided
114158
ordered = true
115159

@@ -129,6 +173,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
129173
)
130174

131175
suite.SetupTest() // reset
176+
expError = nil // must be expliticly changed by failed cases
132177
nextSeqRecv = 1 // must be explicitly changed
133178
path = ibctesting.NewPath(suite.chainA, suite.chainB)
134179

@@ -149,6 +194,11 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
149194
suite.Require().NoError(err)
150195
} else {
151196
suite.Require().Error(err)
197+
// only check if expError is set, since not all error codes can be known
198+
if expError != nil {
199+
suite.Require().True(errors.Is(err, expError))
200+
}
201+
152202
}
153203
})
154204
}

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

+9-4
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,13 @@ var (
2121
ErrPacketTimeout = sdkerrors.Register(SubModuleName, 14, "packet timeout")
2222
ErrTooManyConnectionHops = sdkerrors.Register(SubModuleName, 15, "too many connection hops")
2323
ErrInvalidAcknowledgement = sdkerrors.Register(SubModuleName, 16, "invalid acknowledgement")
24-
ErrPacketCommitmentNotFound = sdkerrors.Register(SubModuleName, 17, "packet commitment not found")
25-
ErrPacketReceived = sdkerrors.Register(SubModuleName, 18, "packet already received")
26-
ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 19, "acknowledgement for packet already exists")
27-
ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 20, "invalid channel identifier")
24+
ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 17, "acknowledgement for packet already exists")
25+
ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 18, "invalid channel identifier")
26+
27+
// packets already relayed errors
28+
ErrPacketReceived = sdkerrors.Register(SubModuleName, 19, "packet already received")
29+
ErrPacketCommitmentNotFound = sdkerrors.Register(SubModuleName, 20, "packet commitment not found") // may occur for already received acknowledgements or timeouts and in rare cases for packets never sent
30+
31+
// ORDERED channel error
32+
ErrPacketSequenceOutOfOrder = sdkerrors.Register(SubModuleName, 21, "packet sequence is out of order")
2833
)

‎modules/core/23-commitment/types/merkle.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,8 @@ func verifyChainedMembershipProof(root []byte, specs []*ics23.ProofSpec, proofs
253253
value = subroot
254254
case *ics23.CommitmentProof_Nonexist:
255255
return sdkerrors.Wrapf(ErrInvalidProof,
256-
"chained membership proof contains nonexistence proof at index %d. If this is unexpected, please ensure that proof was queried from the height that contained the value in store and was queried with the correct key.",
257-
i)
256+
"chained membership proof contains nonexistence proof at index %d. If this is unexpected, please ensure that proof was queried from a height that contained the value in store and was queried with the correct key. The key used: %s",
257+
i, keys)
258258
default:
259259
return sdkerrors.Wrapf(ErrInvalidProof,
260260
"expected proof type: %T, got: %T", &ics23.CommitmentProof_Exist{}, proofs[i].Proof)

‎modules/light-clients/07-tendermint/types/client_state.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ func produceVerificationArgs(
545545
if cs.GetLatestHeight().LT(height) {
546546
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrapf(
547547
sdkerrors.ErrInvalidHeight,
548-
"client state height < proof height (%d < %d)", cs.GetLatestHeight(), height,
548+
"client state height < proof height (%d < %d), please ensure the client has been updated", cs.GetLatestHeight(), height,
549549
)
550550
}
551551

‎testing/endpoint.go

+26-2
Original file line numberDiff line numberDiff line change
@@ -403,8 +403,6 @@ func (endpoint *Endpoint) WriteAcknowledgement(ack exported.Acknowledgement, pac
403403
}
404404

405405
// AcknowledgePacket sends a MsgAcknowledgement to the channel associated with the endpoint.
406-
// TODO: add a query for the acknowledgement by events
407-
// - https://github.com/cosmos/cosmos-sdk/issues/6509
408406
func (endpoint *Endpoint) AcknowledgePacket(packet channeltypes.Packet, ack []byte) error {
409407
// get proof of acknowledgement on counterparty
410408
packetKey := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
@@ -415,6 +413,32 @@ func (endpoint *Endpoint) AcknowledgePacket(packet channeltypes.Packet, ack []by
415413
return endpoint.Chain.sendMsgs(ackMsg)
416414
}
417415

416+
// TimeoutPacket sends a MsgTimeout to the channel associated with the endpoint.
417+
func (endpoint *Endpoint) TimeoutPacket(packet channeltypes.Packet) error {
418+
// get proof for timeout based on channel order
419+
var packetKey []byte
420+
421+
switch endpoint.ChannelConfig.Order {
422+
case channeltypes.ORDERED:
423+
packetKey = host.NextSequenceRecvKey(packet.GetDestPort(), packet.GetDestChannel())
424+
case channeltypes.UNORDERED:
425+
packetKey = host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
426+
default:
427+
return fmt.Errorf("unsupported order type %s", endpoint.ChannelConfig.Order)
428+
}
429+
430+
proof, proofHeight := endpoint.Counterparty.QueryProof(packetKey)
431+
nextSeqRecv, found := endpoint.Counterparty.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextSequenceRecv(endpoint.Counterparty.Chain.GetContext(), endpoint.ChannelConfig.PortID, endpoint.ChannelID)
432+
require.True(endpoint.Chain.t, found)
433+
434+
timeoutMsg := channeltypes.NewMsgTimeout(
435+
packet, nextSeqRecv,
436+
proof, proofHeight, endpoint.Chain.SenderAccount.GetAddress().String(),
437+
)
438+
439+
return endpoint.Chain.sendMsgs(timeoutMsg)
440+
}
441+
418442
// SetChannelClosed sets a channel state to CLOSED.
419443
func (endpoint *Endpoint) SetChannelClosed() error {
420444
channel := endpoint.GetChannel()

0 commit comments

Comments
 (0)