From a290feddfba3babd1e59f003c4fc1a2498b43f8e Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 13:24:42 +0100 Subject: [PATCH 01/17] add check for not equal NOTINFLUSH for SendPacket --- modules/core/04-channel/keeper/packet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 59be12b73ec..6c774c745ba 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -33,7 +33,7 @@ func (k Keeper) SendPacket( return 0, errorsmod.Wrap(types.ErrChannelNotFound, sourceChannel) } - if channel.State != types.OPEN { + if channel.State != types.OPEN || channel.FlushStatus != types.NOTINFLUSH { return 0, errorsmod.Wrapf( types.ErrInvalidChannelState, "channel is not OPEN (got %s)", channel.State.String(), From adfc53e3acc0b3a489162228fc2c698098eaddf5 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 13:29:42 +0100 Subject: [PATCH 02/17] added test cases --- modules/core/04-channel/keeper/packet_test.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 64a1154a2a2..a474ff2ce35 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -217,6 +217,30 @@ func (suite *KeeperTestSuite) TestSendPacket() { channelCap = capabilitytypes.NewCapability(5) }, false}, + { + "invalid flush status: FLUSHING", + func() { + suite.coordinator.Setup(path) + sourceChannel = path.EndpointA.ChannelID + + channel := path.EndpointA.GetChannel() + channel.FlushStatus = types.FLUSHING + path.EndpointA.SetChannel(channel) + }, + false, + }, + { + "invalid flush status: FLUSHCOMPLETE", + func() { + suite.coordinator.Setup(path) + sourceChannel = path.EndpointA.ChannelID + + channel := path.EndpointA.GetChannel() + channel.FlushStatus = types.FLUSHCOMPLETE + path.EndpointA.SetChannel(channel) + }, + false, + }, } for i, tc := range testCases { From 6a36bc58db8e9235d240127ea473627b753d75ba Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 15:26:37 +0100 Subject: [PATCH 03/17] adding test cases and conditional first pass --- modules/core/04-channel/keeper/packet.go | 21 ++++++-- modules/core/04-channel/keeper/packet_test.go | 54 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 6c774c745ba..496409e14b7 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -129,11 +129,22 @@ func (k Keeper) RecvPacket( return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel()) } - if channel.State != types.OPEN { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) + // TODO: clean this up (alot!) + counterpartyLastPacketSent, ok := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) + if !ok { + if channel.State != types.OPEN { + return errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel state is not OPEN (got %s)", channel.State.String(), + ) + } + } else { + if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { + return errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel state is not OPEN (got %s)", channel.State.String(), + ) + } } // Authenticate capability to ensure caller has authority to receive packet on this channel diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index a474ff2ce35..44be79dce37 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -318,6 +318,60 @@ 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}, + { + msg: "success with closed channel: FLUSHING status", + malleate: func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.CLOSED + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + }, + expPass: true, + }, + { + msg: "failure with closed channel counterparty sequence > packet sequence", + malleate: func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.CLOSED + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) + }, + expPass: false, + }, + { + msg: "failure with closed channel not flushing", + malleate: func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.CLOSED + channel.FlushStatus = types.FLUSHCOMPLETE + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + }, + expPass: false, + }, {"packet already relayed ORDERED channel (no-op)", func() { expError = types.ErrNoOpMsg From 40d935154f6c5951bd65066d71672e18deca7e75 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 15:38:16 +0100 Subject: [PATCH 04/17] split out status flush check into separate conditional --- modules/core/04-channel/keeper/packet.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 6c774c745ba..a43f1312384 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -33,6 +33,13 @@ func (k Keeper) SendPacket( return 0, errorsmod.Wrap(types.ErrChannelNotFound, sourceChannel) } + if channel.FlushStatus != types.NOTINFLUSH { + return 0, errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel should not be in %s state", types.NOTINFLUSH.String(), + ) + } + if channel.State != types.OPEN || channel.FlushStatus != types.NOTINFLUSH { return 0, errorsmod.Wrapf( types.ErrInvalidChannelState, From 79be2345197f5d0f62f31e8e99cb2052412d490a Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 15:57:32 +0100 Subject: [PATCH 05/17] refactoring to single if condition --- modules/core/04-channel/keeper/packet.go | 22 +++++-------------- modules/core/04-channel/keeper/packet_test.go | 2 -- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index f8330116c7b..05b9a7b286f 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -136,22 +136,12 @@ func (k Keeper) RecvPacket( return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel()) } - // TODO: clean this up (alot!) - counterpartyLastPacketSent, ok := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) - if !ok { - if channel.State != types.OPEN { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) - } - } else { - if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) - } + counterpartyLastPacketSent, _ := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) + if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { + return errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel state is not OPEN (got %s)", channel.State.String(), + ) } // Authenticate capability to ensure caller has authority to receive packet on this channel diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 44be79dce37..7726e9ea5e5 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -331,8 +331,6 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channel.State = types.CLOSED channel.FlushStatus = types.FLUSHING path.EndpointB.SetChannel(channel) - - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) }, expPass: true, }, From 20da85f28f3d4697ee50785db2a22e1253fbcbfc Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 23 Jun 2023 11:51:45 +0300 Subject: [PATCH 06/17] Address feedback. --- modules/core/04-channel/keeper/packet.go | 2 +- modules/core/04-channel/keeper/packet_test.go | 20 +++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 05b9a7b286f..29711f5779a 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -140,7 +140,7 @@ func (k Keeper) RecvPacket( if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { return errorsmod.Wrapf( types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), + "packets cannot be received on channel with state (%s) and flush status (%s)", channel.State, channel.FlushStatus, ) } diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 7726e9ea5e5..961f6e92f28 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -319,8 +319,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, true}, { - msg: "success with closed channel: FLUSHING status", - malleate: func() { + "success with channel in ACKUPGRADE: FLUSHING status", + func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -328,15 +328,15 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) channel := path.EndpointB.GetChannel() - channel.State = types.CLOSED + channel.State = types.ACKUPGRADE channel.FlushStatus = types.FLUSHING path.EndpointB.SetChannel(channel) }, - expPass: true, + true, }, { - msg: "failure with closed channel counterparty sequence > packet sequence", - malleate: func() { + "failure with closed channel counterparty sequence > packet sequence", + func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -350,11 +350,11 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) }, - expPass: false, + false, }, { - msg: "failure with closed channel not flushing", - malleate: func() { + "failure with closed channel not flushing", + func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -368,7 +368,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) }, - expPass: false, + false, }, {"packet already relayed ORDERED channel (no-op)", func() { expError = types.ErrNoOpMsg From b590bed8a0abf03cec060306e089a1ef39b904a2 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 23 Jun 2023 16:44:31 +0300 Subject: [PATCH 07/17] Tweak them tests. --- modules/core/04-channel/keeper/packet_test.go | 41 +++++++------------ 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 961f6e92f28..9e6033afb78 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -327,33 +327,23 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := path.EndpointB.GetChannel() - channel.State = types.ACKUPGRADE - channel.FlushStatus = types.FLUSHING - path.EndpointB.SetChannel(channel) - }, - true, - }, - { - "failure with closed channel counterparty sequence > packet sequence", - func() { - suite.coordinator.Setup(path) - sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + // Move channel to correct state. + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + + err = path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) - packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) - channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := path.EndpointB.GetChannel() - channel.State = types.CLOSED - channel.FlushStatus = types.FLUSHING - path.EndpointB.SetChannel(channel) + err = path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) + err = path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) }, - false, + true, }, { - "failure with closed channel not flushing", + "failure with closed channel counterparty sequence > packet sequence", func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) @@ -361,12 +351,9 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := path.EndpointB.GetChannel() - channel.State = types.CLOSED - channel.FlushStatus = types.FLUSHCOMPLETE - path.EndpointB.SetChannel(channel) - - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + err = path.EndpointB.SetChannelState(types.CLOSED) + suite.Require().NoError(err) + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) }, false, }, From fb983ccba72b7a6b367dca06b7ddfe7517d18170 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 15:26:37 +0100 Subject: [PATCH 08/17] adding test cases and conditional first pass --- modules/core/04-channel/keeper/packet.go | 21 ++++++-- modules/core/04-channel/keeper/packet_test.go | 54 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 0c212d55c3e..e76fcd4f8bd 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -133,11 +133,22 @@ func (k Keeper) RecvPacket( return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel()) } - if channel.State != types.OPEN { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) + // TODO: clean this up (alot!) + counterpartyLastPacketSent, ok := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) + if !ok { + if channel.State != types.OPEN { + return errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel state is not OPEN (got %s)", channel.State.String(), + ) + } + } else { + if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { + return errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel state is not OPEN (got %s)", channel.State.String(), + ) + } } // Authenticate capability to ensure caller has authority to receive packet on this channel diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 6aeb51cf4ca..fb51003f0d5 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -330,6 +330,60 @@ 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}, + { + msg: "success with closed channel: FLUSHING status", + malleate: func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.CLOSED + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + }, + expPass: true, + }, + { + msg: "failure with closed channel counterparty sequence > packet sequence", + malleate: func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.CLOSED + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) + }, + expPass: false, + }, + { + msg: "failure with closed channel not flushing", + malleate: func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.CLOSED + channel.FlushStatus = types.FLUSHCOMPLETE + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + }, + expPass: false, + }, {"packet already relayed ORDERED channel (no-op)", func() { expError = types.ErrNoOpMsg From f8ac3378748172060db9073ec3c7f33f748fb37f Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 15:38:16 +0100 Subject: [PATCH 09/17] split out status flush check into separate conditional --- modules/core/04-channel/keeper/packet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index e76fcd4f8bd..0570372ddec 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -143,7 +143,7 @@ func (k Keeper) RecvPacket( ) } } else { - if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { + if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { return errorsmod.Wrapf( types.ErrInvalidChannelState, "channel state is not OPEN (got %s)", channel.State.String(), From 4f4ecf977a21646da9b1b035b1c4d212ef6eb33c Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 15:57:32 +0100 Subject: [PATCH 10/17] refactoring to single if condition --- modules/core/04-channel/keeper/packet.go | 22 +++++-------------- modules/core/04-channel/keeper/packet_test.go | 2 -- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 0570372ddec..feb97e91d3a 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -133,22 +133,12 @@ func (k Keeper) RecvPacket( return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel()) } - // TODO: clean this up (alot!) - counterpartyLastPacketSent, ok := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) - if !ok { - if channel.State != types.OPEN { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) - } - } else { - if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) - } + counterpartyLastPacketSent, _ := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) + if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { + return errorsmod.Wrapf( + types.ErrInvalidChannelState, + "channel state is not OPEN (got %s)", channel.State.String(), + ) } // Authenticate capability to ensure caller has authority to receive packet on this channel diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index fb51003f0d5..26f6cca48bb 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -343,8 +343,6 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channel.State = types.CLOSED channel.FlushStatus = types.FLUSHING path.EndpointB.SetChannel(channel) - - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) }, expPass: true, }, From cbdc846c291d39adb7566676b624970f239ba4dd Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 23 Jun 2023 11:51:45 +0300 Subject: [PATCH 11/17] Address feedback. --- modules/core/04-channel/keeper/packet.go | 2 +- modules/core/04-channel/keeper/packet_test.go | 20 +++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index feb97e91d3a..47f0ea514b5 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -137,7 +137,7 @@ func (k Keeper) RecvPacket( if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { return errorsmod.Wrapf( types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), + "packets cannot be received on channel with state (%s) and flush status (%s)", channel.State, channel.FlushStatus, ) } diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 26f6cca48bb..7127298b45b 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -331,8 +331,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, true}, { - msg: "success with closed channel: FLUSHING status", - malleate: func() { + "success with channel in ACKUPGRADE: FLUSHING status", + func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -340,15 +340,15 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) channel := path.EndpointB.GetChannel() - channel.State = types.CLOSED + channel.State = types.ACKUPGRADE channel.FlushStatus = types.FLUSHING path.EndpointB.SetChannel(channel) }, - expPass: true, + true, }, { - msg: "failure with closed channel counterparty sequence > packet sequence", - malleate: func() { + "failure with closed channel counterparty sequence > packet sequence", + func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -362,11 +362,11 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) }, - expPass: false, + false, }, { - msg: "failure with closed channel not flushing", - malleate: func() { + "failure with closed channel not flushing", + func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -380,7 +380,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) }, - expPass: false, + false, }, {"packet already relayed ORDERED channel (no-op)", func() { expError = types.ErrNoOpMsg From 9396ea4ddf7cecbea978848689f5fae7a1f54c09 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 23 Jun 2023 16:44:31 +0300 Subject: [PATCH 12/17] Tweak them tests. --- modules/core/04-channel/keeper/packet_test.go | 41 +++++++------------ 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 7127298b45b..45233b61340 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -339,33 +339,23 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := path.EndpointB.GetChannel() - channel.State = types.ACKUPGRADE - channel.FlushStatus = types.FLUSHING - path.EndpointB.SetChannel(channel) - }, - true, - }, - { - "failure with closed channel counterparty sequence > packet sequence", - func() { - suite.coordinator.Setup(path) - sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + // Move channel to correct state. + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + + err = path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) - packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) - channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := path.EndpointB.GetChannel() - channel.State = types.CLOSED - channel.FlushStatus = types.FLUSHING - path.EndpointB.SetChannel(channel) + err = path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) + err = path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) }, - false, + true, }, { - "failure with closed channel not flushing", + "failure with closed channel counterparty sequence > packet sequence", func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) @@ -373,12 +363,9 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := path.EndpointB.GetChannel() - channel.State = types.CLOSED - channel.FlushStatus = types.FLUSHCOMPLETE - path.EndpointB.SetChannel(channel) - - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + err = path.EndpointB.SetChannelState(types.CLOSED) + suite.Require().NoError(err) + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) }, false, }, From 4edd3b93b3e6d0fa4d1e13f862a55e5e27b5cd5f Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 29 Jun 2023 22:38:23 +0300 Subject: [PATCH 13/17] Don't set version for non-initiating chain. --- modules/core/04-channel/keeper/packet_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 45233b61340..5b27d5f2369 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -340,7 +340,6 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) // Move channel to correct state. - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion err = path.EndpointB.ChanUpgradeInit() From 6f094390729636225aef805dd2c0ca5056b6223d Mon Sep 17 00:00:00 2001 From: Charly Date: Fri, 30 Jun 2023 13:44:17 +0200 Subject: [PATCH 14/17] update code+test --- modules/core/04-channel/keeper/packet.go | 9 ++++-- modules/core/04-channel/keeper/packet_test.go | 31 +++++++++++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index e0e9f8b37ef..a984db93f1d 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -141,10 +141,15 @@ func (k Keeper) RecvPacket( } counterpartyLastPacketSent, _ := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) - if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) { + + if channel.State != types.OPEN && !(channel.FlushStatus == types.FLUSHING && packet.GetSequence() <= counterpartyLastPacketSent) { return errorsmod.Wrapf( types.ErrInvalidChannelState, - "packets cannot be received on channel with state (%s) and flush status (%s)", channel.State, channel.FlushStatus, + "packets cannot be received: either channel with non OPEN state (%s) & not in upgrade, or channel is in upgrade but flush status (%s) is not FLUSHING & packet sequence %d must be less than or equal to counterparty last packet sent sequence %d", + channel.State, + channel.FlushStatus, + packet.GetSequence(), + counterpartyLastPacketSent, ) } diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 5b27d5f2369..7d11c3bd99b 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -350,11 +350,17 @@ func (suite *KeeperTestSuite) TestRecvPacket() { err = path.EndpointB.ChanUpgradeAck() suite.Require().NoError(err) + + channel := path.EndpointB.GetChannel() + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) }, true, }, { - "failure with closed channel counterparty sequence > packet sequence", + "failure while upgrading channel, packet sequence > counterparty last send sequence", func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) @@ -362,9 +368,28 @@ func (suite *KeeperTestSuite) TestRecvPacket() { packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - err = path.EndpointB.SetChannelState(types.CLOSED) + channel := path.EndpointB.GetChannel() + channel.State = types.INITUPGRADE + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence-1) + }, + false, + }, + { + "failure while upgrading channel, channel not flushing", + func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) suite.Require().NoError(err) - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.State = types.INITUPGRADE + channel.FlushStatus = types.FLUSHCOMPLETE + path.EndpointB.SetChannel(channel) }, false, }, From aee46230f77af638f6390940e73b7f031f9528f1 Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 4 Jul 2023 11:03:35 +0100 Subject: [PATCH 15/17] applied suggestion from PR feedback --- modules/core/04-channel/keeper/packet.go | 27 ++++++++---------------- 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index a984db93f1d..9674a3883d7 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -9,6 +9,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/cosmos/ibc-go/v7/internal/collections" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" @@ -33,14 +34,7 @@ func (k Keeper) SendPacket( return 0, errorsmod.Wrap(types.ErrChannelNotFound, sourceChannel) } - if channel.FlushStatus != types.NOTINFLUSH { - return 0, errorsmod.Wrapf( - types.ErrInvalidChannelState, - "channel should not be in %s state", types.NOTINFLUSH.String(), - ) - } - - if channel.State != types.OPEN || channel.FlushStatus != types.NOTINFLUSH { + if channel.State != types.OPEN { return 0, errorsmod.Wrapf( types.ErrInvalidChannelState, "channel is not OPEN (got %s)", channel.State.String(), @@ -140,17 +134,14 @@ func (k Keeper) RecvPacket( return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel()) } - counterpartyLastPacketSent, _ := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()) + if !collections.Contains(channel.State, []types.State{types.OPEN, types.TRYUPGRADE, types.ACKUPGRADE}) { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "channel state was not one of [%s, %s, %s] (got %s)", types.OPEN.String(), types.TRYUPGRADE.String(), types.ACKUPGRADE.String(), channel.State.String()) + } - if channel.State != types.OPEN && !(channel.FlushStatus == types.FLUSHING && packet.GetSequence() <= counterpartyLastPacketSent) { - return errorsmod.Wrapf( - types.ErrInvalidChannelState, - "packets cannot be received: either channel with non OPEN state (%s) & not in upgrade, or channel is in upgrade but flush status (%s) is not FLUSHING & packet sequence %d must be less than or equal to counterparty last packet sent sequence %d", - channel.State, - channel.FlushStatus, - packet.GetSequence(), - counterpartyLastPacketSent, - ) + if counterpartyLastSequenceSend, found := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()); found { + if channel.FlushStatus != types.FLUSHING && packet.GetSequence() > counterpartyLastSequenceSend { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel flush status to not be (%s) and for the counterparty last sequence send (%d) to be less than the packet sequence (%d)", types.FLUSHING.String(), counterpartyLastSequenceSend, packet.GetSequence()) + } } // Authenticate capability to ensure caller has authority to receive packet on this channel From f047c409c5b761dcad1739c7db298dade7dc2aff Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 4 Jul 2023 11:29:52 +0100 Subject: [PATCH 16/17] added test cases to check individual or conditions --- modules/core/04-channel/keeper/packet.go | 6 +++-- modules/core/04-channel/keeper/packet_test.go | 22 +++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 9674a3883d7..42dbb956899 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -138,9 +138,11 @@ func (k Keeper) RecvPacket( return errorsmod.Wrapf(types.ErrInvalidChannelState, "channel state was not one of [%s, %s, %s] (got %s)", types.OPEN.String(), types.TRYUPGRADE.String(), types.ACKUPGRADE.String(), channel.State.String()) } + // in the case of the channel being in TRYUPGRADE or ACKUPGRADE we need to ensure that the channel is not in flushing, + // and that the counterparty last sequence send is less than or equal to the packet sequence. if counterpartyLastSequenceSend, found := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()); found { - if channel.FlushStatus != types.FLUSHING && packet.GetSequence() > counterpartyLastSequenceSend { - return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel flush status to not be (%s) and for the counterparty last sequence send (%d) to be less than the packet sequence (%d)", types.FLUSHING.String(), counterpartyLastSequenceSend, packet.GetSequence()) + if channel.FlushStatus != types.FLUSHING || packet.GetSequence() > counterpartyLastSequenceSend { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel flush status to not be (%s) and for the packet sequence (%d) to be less than or equal to the counterparty last sequence send (%d)", types.FLUSHING.String(), packet.GetSequence(), counterpartyLastSequenceSend) } } diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 7d11c3bd99b..4a843446fab 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -378,7 +378,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() { false, }, { - "failure while upgrading channel, channel not flushing", + "failure while upgrading channel, channel not flushing, packet sequence == counterparty last send sequence", func() { suite.coordinator.Setup(path) sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) @@ -387,9 +387,27 @@ func (suite *KeeperTestSuite) TestRecvPacket() { channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) channel := path.EndpointB.GetChannel() - channel.State = types.INITUPGRADE channel.FlushStatus = types.FLUSHCOMPLETE path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence) + }, + false, + }, + { + "failure while upgrading channel, channel flushing, packet sequence > counterparty last send sequence", + func() { + suite.coordinator.Setup(path) + sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) + suite.Require().NoError(err) + packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp) + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + channel := path.EndpointB.GetChannel() + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence-1) }, false, }, From f146dc319fe7153052663e7a2fbf666e5b1f667e Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 5 Jul 2023 10:44:58 +0100 Subject: [PATCH 17/17] apply PR feedback about error messages --- modules/core/04-channel/keeper/packet.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 42dbb956899..468658bd5fd 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -135,14 +135,14 @@ func (k Keeper) RecvPacket( } if !collections.Contains(channel.State, []types.State{types.OPEN, types.TRYUPGRADE, types.ACKUPGRADE}) { - return errorsmod.Wrapf(types.ErrInvalidChannelState, "channel state was not one of [%s, %s, %s] (got %s)", types.OPEN.String(), types.TRYUPGRADE.String(), types.ACKUPGRADE.String(), channel.State.String()) + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel state to be one of [%s, %s, %s], but got %s", types.OPEN.String(), types.TRYUPGRADE.String(), types.ACKUPGRADE.String(), channel.State.String()) } // in the case of the channel being in TRYUPGRADE or ACKUPGRADE we need to ensure that the channel is not in flushing, // and that the counterparty last sequence send is less than or equal to the packet sequence. if counterpartyLastSequenceSend, found := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()); found { if channel.FlushStatus != types.FLUSHING || packet.GetSequence() > counterpartyLastSequenceSend { - return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel flush status to not be (%s) and for the packet sequence (%d) to be less than or equal to the counterparty last sequence send (%d)", types.FLUSHING.String(), packet.GetSequence(), counterpartyLastSequenceSend) + return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected channel flush status to be (%s) when counterparty last sequence send (%d) is set, failed to recv packet (%d)", types.FLUSHING, counterpartyLastSequenceSend, packet.GetSequence()) } }