Move channel to OPEN if all in-flight packets have been flushed in Up…

…gradeAck. (#4075)

* Move channel to OPEN if all in-flight packets have been flushed in UpgradeAck.

* Add comment noting that counterparty flush status has been verified before usage.

* Fix failing tests.

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

@@ -862,11 +862,16 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
 			packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
 			channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 
+			// Send a packet on B to disallow channel automatically moving to OPEN on UpgradeAck
+			sequence, err := path.EndpointB.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
+			suite.Require().Equal(uint64(1), sequence)
+			suite.Require().NoError(err)
+
 			// Move channel to correct state.
 			path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
 			path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
 
-			err := path.EndpointA.ChanUpgradeInit()
+			err = path.EndpointA.ChanUpgradeInit()
 			suite.Require().NoError(err)
 
 			err = path.EndpointB.ChanUpgradeTry()

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

@@ -775,11 +775,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterpartyStates() {
 				err = path.EndpointB.ChanUpgradeAck()
 				suite.Require().NoError(err)
 
-				// TODO: Remove when #4030 is closed. Channel will automatically
-				// move to OPEN in that case.
-				err = path.EndpointB.ChanUpgradeOpen()
-				suite.Require().NoError(err)
-
 				suite.coordinator.CommitBlock(suite.chainA, suite.chainB)
 				suite.Require().NoError(path.EndpointA.UpdateClient())
 			},
@@ -788,14 +783,25 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterpartyStates() {
 		{
 			"success, counterparty in TRYUPGRADE",
 			func() {
-				err := path.EndpointA.ChanUpgradeInit()
+				// Need to create a packet commitment on A so as to keep it from going to OPEN if no inflight packets exist.
+				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)
+				err = path.EndpointB.RecvPacket(packet)
+				suite.Require().NoError(err)
+
+				err = path.EndpointA.ChanUpgradeInit()
 				suite.Require().NoError(err)
 
 				err = path.EndpointB.ChanUpgradeTry()
 				suite.Require().NoError(err)
 
 				err = path.EndpointA.ChanUpgradeAck()
 				suite.Require().NoError(err)
+
+				// Ack packet to delete packet commitment before calling ChanUpgradeOpen
+				err = path.EndpointA.AcknowledgePacket(packet, ibctesting.MockAcknowledgement)
+				suite.Require().NoError(err)
 			},
 			nil,
 		},
@@ -846,6 +852,13 @@ func (suite *KeeperTestSuite) TestWriteUpgradeOpenChannel() {
 	path := ibctesting.NewPath(suite.chainA, suite.chainB)
 	suite.coordinator.Setup(path)
 
+	// Need to create a packet commitment on A so as to keep it from going to OPEN if no inflight packets exist.
+	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)
+	err = path.EndpointB.RecvPacket(packet)
+	suite.Require().NoError(err)
+
 	path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
 	path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
 	path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Ordering = types.ORDERED
@@ -855,6 +868,10 @@ func (suite *KeeperTestSuite) TestWriteUpgradeOpenChannel() {
 	suite.Require().NoError(path.EndpointB.ChanUpgradeTry())
 	suite.Require().NoError(path.EndpointA.ChanUpgradeAck())
 
+	// Ack packet to delete packet commitment before calling WriteUpgradeOpenChannel
+	err = path.EndpointA.AcknowledgePacket(packet, ibctesting.MockAcknowledgement)
+	suite.Require().NoError(err)
+
 	suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.WriteUpgradeOpenChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 
 	channel := path.EndpointA.GetChannel()

modules/core/keeper/msg_server.go

@@ -852,6 +852,22 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
 
 	ctx.Logger().Info("channel upgrade ack succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)
 
+	// Move channel to OPEN state if both chains have finished flushing any in-flight packets. Counterparty flush status
+	// has been verified in ChanUpgradeAck.
+	if msg.CounterpartyFlushStatus == channeltypes.FLUSHCOMPLETE {
+		channel, found := k.ChannelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId)
+		if !found {
+			return nil, errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", msg.PortId, msg.ChannelId)
+		}
+		if channel.FlushStatus == channeltypes.FLUSHCOMPLETE {
+			cbs.OnChanUpgradeOpen(ctx, msg.PortId, msg.ChannelId)
+
+			k.ChannelKeeper.WriteUpgradeOpenChannel(ctx, msg.PortId, msg.ChannelId)
+
+			ctx.Logger().Info("channel upgrade open succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)
+		}
+	}
+
 	return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.SUCCESS}, nil
 }
 

modules/core/keeper/msg_server_test.go

@@ -937,17 +937,36 @@ func (suite *KeeperTestSuite) TestChannelUpgradeAck() {
 		expResult func(res *channeltypes.MsgChannelUpgradeAckResponse, err error)
 	}{
 		{
-			"success",
+			"success, no pending in-flight packets",
 			func() {},
 			func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
 				suite.Require().NoError(err)
 				suite.Require().NotNil(res)
 				suite.Require().Equal(channeltypes.SUCCESS, res.Result)
 
+				channel := path.EndpointA.GetChannel()
+				suite.Require().Equal(channeltypes.OPEN, channel.State)
+				suite.Require().Equal(uint64(1), channel.UpgradeSequence)
+				suite.Require().Equal(channeltypes.NOTINFLUSH, channel.FlushStatus)
+			},
+		},
+		{
+			"success, pending in-flight packets",
+			func() {
+				portID := path.EndpointA.ChannelConfig.PortID
+				channelID := path.EndpointA.ChannelID
+				// Set a dummy packet commitment to simulate in-flight packets
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), portID, channelID, 1, []byte("hash"))
+			},
+			func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
+				suite.Require().NoError(err)
+				suite.Require().NotNil(res)
+				suite.Require().Equal(channeltypes.SUCCESS, res.Result)
+
 				channel := path.EndpointA.GetChannel()
 				suite.Require().Equal(channeltypes.ACKUPGRADE, channel.State)
 				suite.Require().Equal(uint64(1), channel.UpgradeSequence)
-				suite.Require().Equal(channeltypes.FLUSHCOMPLETE, channel.FlushStatus)
+				suite.Require().Equal(channeltypes.FLUSHING, channel.FlushStatus)
 			},
 		},
 		{