From 187dd22df5dff9ab9c652d7a83a08c8bdd506579 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 21 May 2021 14:01:58 +0200 Subject: [PATCH 1/5] improve error messages --- modules/core/04-channel/keeper/packet.go | 20 ++- modules/core/04-channel/keeper/packet_test.go | 118 ++++++++++++++++-- modules/core/04-channel/keeper/timeout.go | 4 + .../core/04-channel/keeper/timeout_test.go | 25 ++++ modules/core/04-channel/types/errors.go | 13 +- modules/core/23-commitment/types/merkle.go | 4 +- .../07-tendermint/types/client_state.go | 2 +- testing/endpoint.go | 28 ++++- 8 files changed, 190 insertions(+), 24 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 72a1ff5b50c..46d7434c959 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -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(), ) } @@ -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.ErrPacketReceiptOutOfOrder, "packet sequence ≠ next receive sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceRecv, ) } @@ -461,6 +469,10 @@ func (k Keeper) AcknowledgePacket( commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + if len(commitment) == 0 { + return sdkerrors.Wrapf(types.ErrPacketAcknowledged, "packet commitment does not exist") + } + packetCommitment := types.CommitPacket(k.cdc, packet) // verify we sent the packet and haven't cleared it out yet @@ -472,7 +484,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") + return err } // assert packets acknowledged in order diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 4916ae9d995..af05b176de3 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -235,6 +235,29 @@ func (suite *KeeperTestSuite) TestRecvPacket() { // attempts to receive packet 2 without receiving packet 1 channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, true}, + {"packet already relayed ORDERED channel", func() { + 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, timeoutHeight, disabledTimeoutTimestamp) + err := path.EndpointA.SendPacket(packet) + suite.Require().NoError(err) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + err = path.EndpointB.RecvPacket(packet.(types.Packet)) + suite.Require().NoError(err) + }, false}, + {"packet already relayed UNORDERED channel", func() { + // setup uses an UNORDERED channel + 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.SendPacket(packet) + suite.Require().NoError(err) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + err = path.EndpointB.RecvPacket(packet.(types.Packet)) + suite.Require().NoError(err) + }, false}, {"out of order packet failure with ORDERED channel", func() { path.SetChannelOrdered() suite.coordinator.Setup(path) @@ -285,6 +308,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"connection not found", func() { + suite.coordinator.Setup(path) + // pass channel check suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetChannel( suite.chainB.GetContext(), @@ -323,9 +348,11 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"next receive sequence is not found", func() { - path := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.SetupConnections(path) + path.EndpointA.ChannelID = ibctesting.FirstChannelID + path.EndpointB.ChannelID = ibctesting.FirstChannelID + // manually creating channel prevents next recv sequence from being set suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetChannel( suite.chainB.GetContext(), @@ -336,10 +363,13 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp) // manually set packet commitment - suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), ibctesting.MockPacketData) + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), types.CommitPacket(suite.chainA.App.AppCodec(), packet)) suite.chainB.CreateChannelCapability(suite.chainB.GetSimApp().ScopedIBCMockKeeper, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + path.EndpointA.UpdateClient() + path.EndpointB.UpdateClient() }, false}, {"receipt already stored", func() { suite.coordinator.Setup(path) @@ -366,7 +396,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { // get proof of packet commitment from chainA packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - proof, proofHeight := suite.chainA.QueryProof(packetKey) + proof, proofHeight := path.EndpointA.QueryProof(packetKey) err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.RecvPacket(suite.chainB.GetContext(), channelCap, packet, proof, proofHeight) @@ -520,6 +550,41 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, true}, + {"packet already acknowledged ordered channel", func() { + 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, timeoutHeight, disabledTimeoutTimestamp) + // create packet commitment + err := path.EndpointA.SendPacket(packet) + suite.Require().NoError(err) + + // create packet receipt and acknowledgement + err = path.EndpointB.RecvPacket(packet) + suite.Require().NoError(err) + + channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + err = path.EndpointA.AcknowledgePacket(packet, ack.Acknowledgement()) + suite.Require().NoError(err) + }, false}, + {"packet already acknowledged unordered channel", func() { + // setup uses an UNORDERED channel + 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) + + // create packet commitment + err := path.EndpointA.SendPacket(packet) + suite.Require().NoError(err) + + // create packet receipt and acknowledgement + err = path.EndpointB.RecvPacket(packet) + suite.Require().NoError(err) + + channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + err = path.EndpointA.AcknowledgePacket(packet, ack.Acknowledgement()) + suite.Require().NoError(err) + }, false}, {"channel not found", func() { // use wrong channel naming suite.coordinator.Setup(path) @@ -560,11 +625,13 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"connection not found", func() { + suite.coordinator.Setup(path) + // pass channel check - suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetChannel( - suite.chainB.GetContext(), - path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, - types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{connIDB}, path.EndpointB.ChannelConfig.Version), + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{"connection-1000"}, path.EndpointA.ChannelConfig.Version), ) packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp) suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) @@ -601,23 +668,50 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { path.EndpointA.SendPacket(packet) channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, + {"packet commitment bytes do not match", func() { + // setup uses an UNORDERED channel + 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) + + // create packet commitment + err := path.EndpointA.SendPacket(packet) + suite.Require().NoError(err) + + // create packet receipt and acknowledgement + err = path.EndpointB.RecvPacket(packet) + suite.Require().NoError(err) + + channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + packet.Data = []byte("invalid packet commitment") + }, false}, {"next ack sequence not found", func() { - path := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.SetupConnections(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.ChannelID = ibctesting.FirstChannelID + path.EndpointB.ChannelID = ibctesting.FirstChannelID + // manually creating channel prevents next sequence acknowledgement from being set suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel( suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{path.EndpointA.ConnectionID}, path.EndpointA.ChannelConfig.Version), ) + + packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp) // manually set packet commitment - suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), ibctesting.MockPacketData) + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), types.CommitPacket(suite.chainA.App.AppCodec(), packet)) // manually set packet acknowledgement and capability - suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, packet.GetSequence(), ibctesting.MockAcknowledgement) + suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, packet.GetSequence(), types.CommitAcknowledgement(ack.Acknowledgement())) + suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + suite.coordinator.CommitBlock(path.EndpointA.Chain, path.EndpointB.Chain) + + path.EndpointA.UpdateClient() + path.EndpointB.UpdateClient() }, false}, {"next ack sequence mismatch ORDERED", func() { path.SetChannelOrdered() @@ -646,7 +740,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { tc.malleate() packetKey := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) - proof, proofHeight := suite.chainB.QueryProof(packetKey) + proof, proofHeight := path.EndpointB.QueryProof(packetKey) err := suite.chainA.App.GetIBCKeeper().ChannelKeeper.AcknowledgePacket(suite.chainA.GetContext(), channelCap, packet, ack.Acknowledgement(), proof, proofHeight) pc := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index 1f6357d34d3..6342145870b 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -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.ErrPacketTimedOut, "packet sequence (%d)", packet.GetSequence()) + } + packetCommitment := types.CommitPacket(k.cdc, packet) // verify we sent the packet and haven't cleared it out yet diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go index ab6c4e49d2c..bdbb3349713 100644 --- a/modules/core/04-channel/keeper/timeout_test.go +++ b/modules/core/04-channel/keeper/timeout_test.go @@ -42,6 +42,31 @@ 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() { + 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() { + 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() { // use wrong channel naming suite.coordinator.Setup(path) diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 82cf773057d..547524717a6 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -22,7 +22,14 @@ var ( 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, 18, "acknowledgement for packet already exists") + ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 19, "invalid channel identifier") + + // packets already relayed errors + ErrPacketReceived = sdkerrors.Register(SubModuleName, 20, "packet already received") + ErrPacketAcknowledged = sdkerrors.Register(SubModuleName, 21, "packet acknowledgement already processed") + ErrPacketTimedOut = sdkerrors.Register(SubModuleName, 22, "packet already timed out") + + // ORDERED channel error + ErrPacketReceiptOutOfOrder = sdkerrors.Register(SubModuleName, 23, "packet cannot be received out of order") ) diff --git a/modules/core/23-commitment/types/merkle.go b/modules/core/23-commitment/types/merkle.go index 597a1ac9983..72b3de47b4f 100644 --- a/modules/core/23-commitment/types/merkle.go +++ b/modules/core/23-commitment/types/merkle.go @@ -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) diff --git a/modules/light-clients/07-tendermint/types/client_state.go b/modules/light-clients/07-tendermint/types/client_state.go index 4928a6819aa..5e302436897 100644 --- a/modules/light-clients/07-tendermint/types/client_state.go +++ b/modules/light-clients/07-tendermint/types/client_state.go @@ -522,7 +522,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, ) } diff --git a/testing/endpoint.go b/testing/endpoint.go index e32d0cdfbf7..bc03c5482e6 100644 --- a/testing/endpoint.go +++ b/testing/endpoint.go @@ -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()) @@ -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() From 79c9c5b2e0e882db73f6c7cc9c26c7919b691403 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 21 May 2021 14:11:27 +0200 Subject: [PATCH 2/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8119e1019a7..cc2dbd2adb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,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. * (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. * (modules/light-clients/07-tendermint) [\#125](https://github.com/cosmos/ibc-go/pull/125) Implement efficient iteration of consensus states and pruning of earliest expired consensus state on UpdateClient. From b9757eb664acbe6bf8d0629f798e7478ccee0f32 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 May 2021 13:36:38 +0200 Subject: [PATCH 3/5] update recvpacket errors and test --- modules/core/04-channel/keeper/packet.go | 2 +- modules/core/04-channel/keeper/packet_test.go | 39 +++++++++++++++++++ modules/core/04-channel/keeper/timeout.go | 2 +- modules/core/04-channel/types/errors.go | 10 ++--- 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 46d7434c959..2156e9a5118 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -470,7 +470,7 @@ func (k Keeper) AcknowledgePacket( commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) if len(commitment) == 0 { - return sdkerrors.Wrapf(types.ErrPacketAcknowledged, "packet commitment does not exist") + 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()) } packetCommitment := types.CommitPacket(k.cdc, packet) diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index af05b176de3..5eb9f708ec0 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -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" @@ -200,6 +204,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { path *ibctesting.Path packet exported.PacketI channelCap *capabilitytypes.Capability + expError *sdkerrors.Error ) testCases := []testCase{ @@ -236,6 +241,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, true}, {"packet already relayed ORDERED channel", func() { + expError = types.ErrPacketReceived + path.SetChannelOrdered() suite.coordinator.Setup(path) @@ -248,6 +255,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.Require().NoError(err) }, false}, {"packet already relayed UNORDERED channel", func() { + expError = types.ErrPacketReceived + // setup uses an UNORDERED channel 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) @@ -259,6 +268,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.Require().NoError(err) }, false}, {"out of order packet failure with ORDERED channel", func() { + expError = types.ErrPacketReceiptOutOfOrder + 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, timeoutHeight, disabledTimeoutTimestamp) @@ -274,12 +285,16 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"channel not found", func() { + expError = types.ErrChannelNotFound + // use wrong channel naming suite.coordinator.Setup(path) packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ibctesting.InvalidID, ibctesting.InvalidID, timeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, 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) @@ -288,26 +303,34 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"capability cannot authenticate ORDERED", func() { + expError = types.ErrInvalidChannelCapability + 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, timeoutHeight, disabledTimeoutTimestamp) err := path.EndpointA.SendPacket(packet) suite.Require().NoError(err) channelCap = capabilitytypes.NewCapability(3) }, false}, {"packet source port ≠ channel counterparty port", func() { + expError = types.ErrInvalidPacket suite.coordinator.Setup(path) + // use wrong port for dest packet = types.NewPacket(ibctesting.MockPacketData, 1, ibctesting.InvalidID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"packet source channel ID ≠ channel counterparty channel ID", func() { + expError = types.ErrInvalidPacket suite.coordinator.Setup(path) + // use wrong port for dest packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, ibctesting.InvalidID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"connection not found", func() { + expError = connectiontypes.ErrConnectionNotFound suite.coordinator.Setup(path) // pass channel check @@ -321,6 +344,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"connection not OPEN", func() { + expError = connectiontypes.ErrInvalidConnectionState suite.coordinator.SetupClients(path) // connection on chainB is in INIT @@ -338,16 +362,21 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"timeout height passed", 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, clienttypes.GetSelfHeight(suite.chainB.GetContext()), disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"timeout timestamp passed", 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, disabledTimeoutHeight, uint64(suite.chainB.GetContext().BlockTime().UnixNano())) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"next receive sequence is not found", func() { + expError = types.ErrSequenceReceiveNotFound suite.coordinator.SetupConnections(path) path.EndpointA.ChannelID = ibctesting.FirstChannelID @@ -372,13 +401,17 @@ func (suite *KeeperTestSuite) TestRecvPacket() { path.EndpointB.UpdateClient() }, false}, {"receipt already stored", func() { + expError = types.ErrPacketReceived 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) suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, 1) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false}, {"validation failed", func() { + // skip error code check, downstream error code is used from light-client implementations + // packet commitment not set resulting in invalid proof 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) @@ -390,6 +423,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { tc := tc suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { suite.SetupTest() // reset + expError = nil // must explicitly set for failed cases path = ibctesting.NewPath(suite.chainA, suite.chainB) tc.malleate() @@ -418,6 +452,11 @@ func (suite *KeeperTestSuite) TestRecvPacket() { } } 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)) + } } }) } diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index 6342145870b..004e9d211ef 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -81,7 +81,7 @@ func (k Keeper) TimeoutPacket( commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) if len(commitment) == 0 { - return sdkerrors.Wrapf(types.ErrPacketTimedOut, "packet sequence (%d)", packet.GetSequence()) + 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()) } packetCommitment := types.CommitPacket(k.cdc, packet) diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 547524717a6..23b3bdf5988 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -21,14 +21,12 @@ 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") - ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 18, "acknowledgement for packet already exists") - ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 19, "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, 20, "packet already received") - ErrPacketAcknowledged = sdkerrors.Register(SubModuleName, 21, "packet acknowledgement already processed") - ErrPacketTimedOut = sdkerrors.Register(SubModuleName, 22, "packet already timed out") + 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 ErrPacketReceiptOutOfOrder = sdkerrors.Register(SubModuleName, 23, "packet cannot be received out of order") From 7044cd8daa84872c178e045003a86e9bc6525629 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 May 2021 15:52:07 +0200 Subject: [PATCH 4/5] add acknowledge packet tests --- modules/core/04-channel/keeper/packet.go | 4 +-- modules/core/04-channel/keeper/packet_test.go | 33 ++++++++++++++++++- modules/core/04-channel/types/errors.go | 2 +- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 2156e9a5118..32ecb14e654 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -272,7 +272,7 @@ func (k Keeper) RecvPacket( if packet.GetSequence() != nextSequenceRecv { return sdkerrors.Wrapf( - types.ErrPacketReceiptOutOfOrder, + types.ErrPacketSequenceOutOfOrder, "packet sequence ≠ next receive sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceRecv, ) } @@ -499,7 +499,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, ) } diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 5eb9f708ec0..91ddc7d15d6 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -268,7 +268,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.Require().NoError(err) }, false}, {"out of order packet failure with ORDERED channel", func() { - expError = types.ErrPacketReceiptOutOfOrder + expError = types.ErrPacketSequenceOutOfOrder path.SetChannelOrdered() suite.coordinator.Setup(path) @@ -557,6 +557,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { ack = ibcmock.MockAcknowledgement channelCap *capabilitytypes.Capability + expError *sdkerrors.Error ) testCases := []testCase{ @@ -590,6 +591,8 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, true}, {"packet already acknowledged ordered channel", func() { + expError = types.ErrPacketCommitmentNotFound + 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, timeoutHeight, disabledTimeoutTimestamp) @@ -607,6 +610,8 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { suite.Require().NoError(err) }, false}, {"packet already acknowledged unordered channel", func() { + expError = types.ErrPacketCommitmentNotFound + // setup uses an UNORDERED channel 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) @@ -625,11 +630,15 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { 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) @@ -638,8 +647,11 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"capability authentication failed ORDERED", func() { + expError = types.ErrInvalidChannelCapability + 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, timeoutHeight, disabledTimeoutTimestamp) // create packet commitment err := path.EndpointA.SendPacket(packet) @@ -652,18 +664,23 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = capabilitytypes.NewCapability(3) }, 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) channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, 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) channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"connection not found", func() { + expError = connectiontypes.ErrConnectionNotFound suite.coordinator.Setup(path) // pass channel check @@ -677,6 +694,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"connection not OPEN", func() { + expError = connectiontypes.ErrInvalidConnectionState suite.coordinator.SetupClients(path) // connection on chainA is in INIT err := path.EndpointA.ConnOpenInit() @@ -693,12 +711,16 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"packet hasn't been sent", func() { + expError = types.ErrPacketCommitmentNotFound + // packet commitment never written 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) channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"packet ack verification failed", func() { + // skip error code check since error occurs in light-clients + // ack never written 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) @@ -708,6 +730,8 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) }, false}, {"packet commitment bytes do not match", func() { + expError = types.ErrInvalidPacket + // setup uses an UNORDERED channel 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) @@ -725,6 +749,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { packet.Data = []byte("invalid packet commitment") }, false}, {"next ack sequence not found", func() { + expError = types.ErrSequenceAckNotFound suite.coordinator.SetupConnections(path) path.EndpointA.ChannelID = ibctesting.FirstChannelID @@ -753,6 +778,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { path.EndpointB.UpdateClient() }, false}, {"next ack sequence mismatch ORDERED", func() { + expError = types.ErrPacketSequenceOutOfOrder 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, timeoutHeight, disabledTimeoutTimestamp) @@ -774,6 +800,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { tc := tc suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { suite.SetupTest() // reset + expError = nil // must explcitly set error for failed cases path = ibctesting.NewPath(suite.chainA, suite.chainB) tc.malleate() @@ -798,6 +825,10 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { } } else { suite.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)) + } } }) } diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 23b3bdf5988..30293eddf07 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -29,5 +29,5 @@ var ( 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 - ErrPacketReceiptOutOfOrder = sdkerrors.Register(SubModuleName, 23, "packet cannot be received out of order") + ErrPacketSequenceOutOfOrder = sdkerrors.Register(SubModuleName, 21, "packet sequence is out of order") ) From ef95d451d90e1f14ffbca18746e16c07e9ba5126 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 May 2021 16:01:51 +0200 Subject: [PATCH 5/5] update timeout tests --- modules/core/04-channel/keeper/timeout.go | 2 +- .../core/04-channel/keeper/timeout_test.go | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index 004e9d211ef..08d10f83612 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -96,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(), ) } diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go index bdbb3349713..460e6097632 100644 --- a/modules/core/04-channel/keeper/timeout_test.go +++ b/modules/core/04-channel/keeper/timeout_test.go @@ -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" @@ -20,6 +24,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { packet types.Packet nextSeqRecv uint64 ordered bool + expError *sdkerrors.Error ) testCases := []testCase{ @@ -43,6 +48,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { path.EndpointA.UpdateClient() }, true}, {"packet already timed out: ORDERED", func() { + expError = types.ErrInvalidChannelState ordered = true path.SetChannelOrdered() @@ -56,6 +62,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { suite.Require().NoError(err) }, false}, {"packet already timed out: UNORDERED", func() { + expError = types.ErrPacketCommitmentNotFound ordered = false suite.coordinator.Setup(path) @@ -68,11 +75,13 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { 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) @@ -80,16 +89,19 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { 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(), @@ -99,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() @@ -116,6 +130,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { path.EndpointA.UpdateClient() }, false}, {"packet hasn't been sent", func() { + expError = types.ErrPacketCommitmentNotFound ordered = true path.SetChannelOrdered() @@ -124,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 @@ -135,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 @@ -154,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) @@ -174,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)) + } + } }) }