Timing out packets on ordered channels when in flushing state aborts upgrade and closes channel

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

@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"bytes"
+	"fmt"
 	"strconv"
 
 	errorsmod "cosmossdk.io/errors"
@@ -150,15 +151,8 @@ 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.FLUSHING {
+	if channel.State == types.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())
@@ -171,17 +165,31 @@ 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()) {
+			} 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.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: 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.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)
+		emitChannelClosedEvent(ctx, packet, channel)
+	}
+
 	k.Logger(ctx).Info(
 		"packet timed-out",
 		"sequence", strconv.FormatUint(packet.GetSequence(), 10),

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.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 {