Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve error messages, indicate already relayed packets #184

Merged
merged 11 commits into from
May 27, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (modules/core) [\#184](https://github.com/cosmos/ibc-go/pull/184) Improve error messages. Uses unique error codes to indicate already relayed packets.
* (07-tendermint) [\#182](https://github.com/cosmos/ibc-go/pull/182) Remove duplicate checks in upgrade logic.
* (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.
* (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.
Expand Down
22 changes: 17 additions & 5 deletions modules/core/04-channel/keeper/packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ func (k Keeper) RecvPacket(
_, found := k.GetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
if found {
return sdkerrors.Wrapf(
types.ErrInvalidPacket,
"packet sequence (%d) already has been received", packet.GetSequence(),
types.ErrPacketReceived,
"packet sequence (%d)", packet.GetSequence(),
)
}

Expand All @@ -262,9 +262,17 @@ func (k Keeper) RecvPacket(
)
}

// helpful error message for relayers
if packet.GetSequence() < nextSequenceRecv {
return sdkerrors.Wrapf(
types.ErrPacketReceived,
"packet sequence (%d), next sequence receive (%d)", packet.GetSequence(), nextSequenceRecv,
)
}

if packet.GetSequence() != nextSequenceRecv {
return sdkerrors.Wrapf(
types.ErrInvalidPacket,
types.ErrPacketSequenceOutOfOrder,
"packet sequence ≠ next receive sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceRecv,
)
}
Expand Down Expand Up @@ -462,6 +470,10 @@ func (k Keeper) AcknowledgePacket(

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

if len(commitment) == 0 {
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean, "in rare cases"? In the case of incorrect relayer behaviour, perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, should I rephrase? My thought process is it seems very unlikely a relayer would have a proof for a packet never sent (I'm not even sure how this would happen). The only case I can think of is if the relayer sets the packet sequence incorrectly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth noting that this indicates some relayer misconfiguration, yeah. Very minor though.

}

packetCommitment := types.CommitPacket(k.cdc, packet)

// verify we sent the packet and haven't cleared it out yet
Expand All @@ -473,7 +485,7 @@ func (k Keeper) AcknowledgePacket(
ctx, connectionEnd, proofHeight, proof, packet.GetDestPort(), packet.GetDestChannel(),
packet.GetSequence(), acknowledgement,
); err != nil {
return sdkerrors.Wrap(err, "packet acknowledgement verification failed")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return err
}

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

if packet.GetSequence() != nextSequenceAck {
return sdkerrors.Wrapf(
sdkerrors.ErrInvalidSequence,
types.ErrPacketSequenceOutOfOrder,
"packet sequence ≠ next ack sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceAck,
)
}
Expand Down
188 changes: 176 additions & 12 deletions modules/core/04-channel/keeper/packet_test.go

Large diffs are not rendered by default.

6 changes: 5 additions & 1 deletion modules/core/04-channel/keeper/timeout.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ func (k Keeper) TimeoutPacket(

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

if len(commitment) == 0 {
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on the rare cases question

}

packetCommitment := types.CommitPacket(k.cdc, packet)

// verify we sent the packet and haven't cleared it out yet
Expand All @@ -92,7 +96,7 @@ func (k Keeper) TimeoutPacket(
// check that packet has not been received
if nextSequenceRecv > packet.GetSequence() {
return sdkerrors.Wrapf(
types.ErrInvalidPacket,
types.ErrPacketReceived,
"packet already received, next sequence receive > packet sequence (%d > %d)", nextSequenceRecv, packet.GetSequence(),
)
}
Expand Down
50 changes: 50 additions & 0 deletions modules/core/04-channel/keeper/timeout_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package keeper_test

import (
"errors"
"fmt"

sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types"
"github.com/cosmos/ibc-go/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
"github.com/cosmos/ibc-go/modules/core/exported"
Expand All @@ -20,6 +24,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
packet types.Packet
nextSeqRecv uint64
ordered bool
expError *sdkerrors.Error
)

testCases := []testCase{
Expand All @@ -42,29 +47,61 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()
}, true},
{"packet already timed out: ORDERED", func() {
expError = types.ErrInvalidChannelState
ordered = true
path.SetChannelOrdered()

suite.coordinator.Setup(path)
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()))
path.EndpointA.SendPacket(packet)
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()

err := path.EndpointA.TimeoutPacket(packet)
suite.Require().NoError(err)
}, false},
{"packet already timed out: UNORDERED", func() {
expError = types.ErrPacketCommitmentNotFound
ordered = false

suite.coordinator.Setup(path)
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)
path.EndpointA.SendPacket(packet)
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()

err := path.EndpointA.TimeoutPacket(packet)
suite.Require().NoError(err)
}, false},
{"channel not found", func() {
expError = types.ErrChannelNotFound
// use wrong channel naming
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, ibctesting.InvalidID, ibctesting.InvalidID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
}, false},
{"channel not open", func() {
expError = types.ErrInvalidChannelState
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)

err := path.EndpointA.SetChannelClosed()
suite.Require().NoError(err)
}, false},
{"packet destination port ≠ channel counterparty port", func() {
expError = types.ErrInvalidPacket
suite.coordinator.Setup(path)
// use wrong port for dest
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ibctesting.InvalidID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
}, false},
{"packet destination channel ID ≠ channel counterparty channel ID", func() {
expError = types.ErrInvalidPacket
suite.coordinator.Setup(path)
// use wrong channel for dest
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, ibctesting.InvalidID, timeoutHeight, disabledTimeoutTimestamp)
}, false},
{"connection not found", func() {
expError = connectiontypes.ErrConnectionNotFound
// pass channel check
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel(
suite.chainA.GetContext(),
Expand All @@ -74,12 +111,14 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
}, false},
{"timeout", func() {
expError = types.ErrPacketTimeout
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
path.EndpointA.SendPacket(packet)
path.EndpointA.UpdateClient()
}, false},
{"packet already received ", func() {
expError = types.ErrPacketReceived
ordered = true
path.SetChannelOrdered()

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

Expand All @@ -99,6 +139,8 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
path.EndpointA.UpdateClient()
}, false},
{"next seq receive verification failed", func() {
// skip error check, error occurs in light-clients

// set ordered to false resulting in wrong proof provided
ordered = false

Expand All @@ -110,6 +152,8 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
path.EndpointA.UpdateClient()
}, false},
{"packet ack verification failed", func() {
// skip error check, error occurs in light-clients

// set ordered to true resulting in wrong proof provided
ordered = true

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

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

Expand All @@ -149,6 +194,11 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
// only check if expError is set, since not all error codes can be known
if expError != nil {
suite.Require().True(errors.Is(err, expError))
}

}
})
}
Expand Down
13 changes: 9 additions & 4 deletions modules/core/04-channel/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@ var (
ErrPacketTimeout = sdkerrors.Register(SubModuleName, 14, "packet timeout")
ErrTooManyConnectionHops = sdkerrors.Register(SubModuleName, 15, "too many connection hops")
ErrInvalidAcknowledgement = sdkerrors.Register(SubModuleName, 16, "invalid acknowledgement")
ErrPacketCommitmentNotFound = sdkerrors.Register(SubModuleName, 17, "packet commitment not found")
ErrPacketReceived = sdkerrors.Register(SubModuleName, 18, "packet already received")
ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 19, "acknowledgement for packet already exists")
ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 20, "invalid channel identifier")
ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 17, "acknowledgement for packet already exists")
ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 18, "invalid channel identifier")

// packets already relayed errors
ErrPacketReceived = sdkerrors.Register(SubModuleName, 19, "packet already received")
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

// ORDERED channel error
ErrPacketSequenceOutOfOrder = sdkerrors.Register(SubModuleName, 21, "packet sequence is out of order")
)
4 changes: 2 additions & 2 deletions modules/core/23-commitment/types/merkle.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ func verifyChainedMembershipProof(root []byte, specs []*ics23.ProofSpec, proofs
value = subroot
case *ics23.CommitmentProof_Nonexist:
return sdkerrors.Wrapf(ErrInvalidProof,
"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.",
i)
"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",
i, keys)
default:
return sdkerrors.Wrapf(ErrInvalidProof,
"expected proof type: %T, got: %T", &ics23.CommitmentProof_Exist{}, proofs[i].Proof)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ func produceVerificationArgs(
if cs.GetLatestHeight().LT(height) {
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrapf(
sdkerrors.ErrInvalidHeight,
"client state height < proof height (%d < %d)", cs.GetLatestHeight(), height,
"client state height < proof height (%d < %d), please ensure the client has been updated", cs.GetLatestHeight(), height,
)
}

Expand Down
28 changes: 26 additions & 2 deletions testing/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,6 @@ func (endpoint *Endpoint) WriteAcknowledgement(ack exported.Acknowledgement, pac
}

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

// TimeoutPacket sends a MsgTimeout to the channel associated with the endpoint.
func (endpoint *Endpoint) TimeoutPacket(packet channeltypes.Packet) error {
// get proof for timeout based on channel order
var packetKey []byte

switch endpoint.ChannelConfig.Order {
case channeltypes.ORDERED:
packetKey = host.NextSequenceRecvKey(packet.GetDestPort(), packet.GetDestChannel())
case channeltypes.UNORDERED:
packetKey = host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
default:
return fmt.Errorf("unsupported order type %s", endpoint.ChannelConfig.Order)
}

proof, proofHeight := endpoint.Counterparty.QueryProof(packetKey)
nextSeqRecv, found := endpoint.Counterparty.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextSequenceRecv(endpoint.Counterparty.Chain.GetContext(), endpoint.ChannelConfig.PortID, endpoint.ChannelID)
require.True(endpoint.Chain.t, found)

timeoutMsg := channeltypes.NewMsgTimeout(
packet, nextSeqRecv,
proof, proofHeight, endpoint.Chain.SenderAccount.GetAddress().String(),
)

return endpoint.Chain.sendMsgs(timeoutMsg)
}

// SetChannelClosed sets a channel state to CLOSED.
func (endpoint *Endpoint) SetChannelClosed() error {
channel := endpoint.GetChannel()
Expand Down