From b6b6e51aab27e64182f1886ef93e6b1aa99293b7 Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 28 Aug 2023 12:36:03 +0100 Subject: [PATCH 1/5] chore: moving flushing check to before the channel is closed --- modules/core/04-channel/keeper/timeout.go | 25 +++++++++++++---------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index ebc2c9e8044..6ce4f1ffb2d 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -150,13 +150,6 @@ func (k Keeper) TimeoutExecuted( k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - if channel.Ordering == types.ORDERED { - channel.State = types.CLOSED - k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) - emitChannelClosedEvent(ctx, packet, channel) - } - - // TODO: handle situation outlined in https://github.com/cosmos/ibc-go/issues/4454 // if an upgrade is in progress, handling packet flushing and update channel state appropriately if channel.State == types.STATE_FLUSHING { counterpartyUpgrade, found := k.GetCounterpartyUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) @@ -171,17 +164,27 @@ func (k Keeper) TimeoutExecuted( // packet flushing timeout has expired, abort the upgrade and return nil, // committing an error receipt to state, restoring the channel and successfully timing out the packet. k.MustAbortUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), err) - return nil - } - // set the channel state to flush complete if all packets have been flushed. - if !k.HasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) { + // note: we continue to close the channel even if the upgrade has been aborted. + // the end desired state is: + // - the channel is closed for an ordered channel, open for an unordered. + // - the upgrade info is always deleted (the upgrade is aborted) + // - an error receipt is written to state + + } else if !k.HasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) { + // set the channel state to flush complete if all packets have been flushed. channel.State = types.STATE_FLUSHCOMPLETE k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) } } } + if channel.Ordering == types.ORDERED { + channel.State = types.CLOSED + k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) + emitChannelClosedEvent(ctx, packet, channel) + } + k.Logger(ctx).Info( "packet timed-out", "sequence", strconv.FormatUint(packet.GetSequence(), 10), From d7a02076bac2435c09b5a5ec43cc63d9bf9b952b Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 28 Aug 2023 12:46:43 +0100 Subject: [PATCH 2/5] chore: adding test case for closed channel when the upgrade is aborted --- modules/core/04-channel/keeper/timeout.go | 13 +++--- .../core/04-channel/keeper/timeout_test.go | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index 6ce4f1ffb2d..ae07683a535 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -164,13 +164,6 @@ func (k Keeper) TimeoutExecuted( // packet flushing timeout has expired, abort the upgrade and return nil, // committing an error receipt to state, restoring the channel and successfully timing out the packet. k.MustAbortUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), err) - - // note: we continue to close the channel even if the upgrade has been aborted. - // the end desired state is: - // - the channel is closed for an ordered channel, open for an unordered. - // - the upgrade info is always deleted (the upgrade is aborted) - // - an error receipt is written to state - } else if !k.HasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) { // set the channel state to flush complete if all packets have been flushed. channel.State = types.STATE_FLUSHCOMPLETE @@ -180,6 +173,12 @@ func (k Keeper) TimeoutExecuted( } if channel.Ordering == types.ORDERED { + // note: we continue to close the unordered channel even if the upgrade has been aborted. + // the end desired state is: + // - the channel is closed. + // - the upgrade info is always deleted (if the upgrade is aborted) + // - an error receipt is written to state (if the upgrade is aborted) + channel.State = types.CLOSED k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) emitChannelClosedEvent(ctx, packet, channel) diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go index 4af271bc88e..ee451ee36ca 100644 --- a/modules/core/04-channel/keeper/timeout_test.go +++ b/modules/core/04-channel/keeper/timeout_test.go @@ -444,6 +444,51 @@ func (suite *KeeperTestSuite) TestTimeoutExecuted() { suite.Require().False(found, "error receipt should not be written") }, }, + { + "ordered channel is closed and upgrade is aborted when timeout is executed", + func() { + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + timeoutHeight := clienttypes.GetSelfHeight(suite.chainB.GetContext()) + timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano()) + + sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, 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, timeoutHeight, timeoutTimestamp) + chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + channel := path.EndpointA.GetChannel() + channel.State = types.STATE_FLUSHING + path.EndpointA.SetChannel(channel) + path.EndpointA.SetChannelUpgrade(types.Upgrade{ + Fields: path.EndpointA.GetProposedUpgrade().Fields, + Timeout: types.NewTimeout(clienttypes.ZeroHeight(), 1), + }) + path.EndpointA.SetChannelCounterpartyUpgrade(types.Upgrade{ + Timeout: types.NewTimeout(clienttypes.ZeroHeight(), 1), + }) + }, + func(packetCommitment []byte, err error) { + suite.Require().NoError(err) + + channel := path.EndpointA.GetChannel() + suite.Require().Equal(types.CLOSED, channel.State, "channel state should be CLOSED") + + upgrade, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "upgrade should not be present") + suite.Require().Equal(types.Upgrade{}, upgrade, "upgrade should be zero value") + + upgrade, found = suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetCounterpartyUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "counterparty upgrade should not be present") + suite.Require().Equal(types.Upgrade{}, upgrade, "counterparty upgrade should be zero value") + + errorReceipt, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(found, "error receipt should be present") + suite.Require().Equal(channel.UpgradeSequence, errorReceipt.Sequence, "error receipt sequence should be equal to channel upgrade sequence") + }, + }, } for i, tc := range testCases { From 99ecbaf8d96af3dff056115b3bc860de3bc9f9d6 Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 29 Aug 2023 10:50:27 +0100 Subject: [PATCH 3/5] wip --- modules/core/04-channel/keeper/timeout.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index ae07683a535..996741d22c2 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -2,6 +2,7 @@ package keeper import ( "bytes" + "fmt" "strconv" errorsmod "cosmossdk.io/errors" @@ -169,11 +170,15 @@ func (k Keeper) TimeoutExecuted( channel.State = types.STATE_FLUSHCOMPLETE k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) } + // upgrade fields have been set but the timeout has not. This can happen when the counterparty + // upgrade is partially written in WriteUpgradeTryChannel. + } else if counterpartyUpgrade.Fields.Version != "" { + k.MustAbortUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), fmt.Errorf("uh oh")) } } if channel.Ordering == types.ORDERED { - // note: we continue to close the unordered channel even if the upgrade has been aborted. + // note: we continue to close the ordered channel even if the upgrade has been aborted. // the end desired state is: // - the channel is closed. // - the upgrade info is always deleted (if the upgrade is aborted) From 680ce92b9858a4cd1d8ad4f3d6d97315ccc534e3 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 30 Aug 2023 13:43:29 +0200 Subject: [PATCH 4/5] refactor: adapting logic for aborting upgrade on timeout of ordered channel packets --- modules/core/04-channel/keeper/timeout.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index ae07683a535..8ce55e60464 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -151,7 +151,7 @@ func (k Keeper) TimeoutExecuted( k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) // if an upgrade is in progress, handling packet flushing and update channel state appropriately - if channel.State == types.STATE_FLUSHING { + if channel.State == types.STATE_FLUSHING && channel.Ordering == types.UNORDERED { counterpartyUpgrade, found := k.GetCounterpartyUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) if !found { return errorsmod.Wrapf(types.ErrUpgradeNotFound, "counterparty upgrade not found for channel: %s", packet.GetSourceChannel()) @@ -173,11 +173,12 @@ func (k Keeper) TimeoutExecuted( } if channel.Ordering == types.ORDERED { - // note: we continue to close the unordered channel even if the upgrade has been aborted. - // the end desired state is: - // - the channel is closed. - // - the upgrade info is always deleted (if the upgrade is aborted) - // - an error receipt is written to state (if the upgrade is aborted) + // NOTE: if the channel is ORDERED and a packet is timed out in FLUSHING state then + // the upgrade is aborted and the channel is set to CLOSED. + if channel.State == types.STATE_FLUSHING { + // an error receipt is written to state and the channel is restored to OPEN + k.MustAbortUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), types.ErrPacketTimeout) + } channel.State = types.CLOSED k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) From a634bed68310518289c325038916d0c822f7067b Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 30 Aug 2023 14:38:25 +0100 Subject: [PATCH 5/5] chore: fix build errors --- modules/core/04-channel/keeper/timeout_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go index d8f155f2f6f..1565bc5adf3 100644 --- a/modules/core/04-channel/keeper/timeout_test.go +++ b/modules/core/04-channel/keeper/timeout_test.go @@ -460,7 +460,7 @@ func (suite *KeeperTestSuite) TestTimeoutExecuted() { chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) channel := path.EndpointA.GetChannel() - channel.State = types.STATE_FLUSHING + channel.State = types.FLUSHING path.EndpointA.SetChannel(channel) path.EndpointA.SetChannelUpgrade(types.Upgrade{ Fields: path.EndpointA.GetProposedUpgrade().Fields,