Feat: Delete upgrade information only rather than calling abort in Ti…

…meoutExecuted (cosmos#5764) * delete instead of abort * no longer have error receipt * nit: Remove stale inline comment Co-authored-by: Carlos Rodriguez <[email protected]> * nit: tweak previous comment to remove reference to aborting upgrade. * add log and delete upgrade info in chan close confirm * use correct port id, channel id --------- Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
notional-labs · Feb 6, 2024 · fc0bfab · fc0bfab
1 parent d14bdff
commit fc0bfab
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 8 deletions.
diff --git a/modules/core/04-channel/keeper/handshake.go b/modules/core/04-channel/keeper/handshake.go
@@ -465,6 +465,17 @@ func (k Keeper) ChanCloseConfirm(
 		return err
 	}
 
+	// If the channel is closing during an upgrade, then we can delete all upgrade information.
+	if k.hasUpgrade(ctx, portID, channelID) {
+		k.deleteUpgradeInfo(ctx, portID, channelID)
+		k.Logger(ctx).Info(
+			"upgrade info deleted",
+			"port_id", portID,
+			"channel_id", channelID,
+			"upgrade_sequence", channel.UpgradeSequence,
+		)
+	}
+
 	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", channel.State.String(), "new-state", types.CLOSED.String())
 
 	defer telemetry.IncrCounter(1, "ibc", "channel", "close-confirm")

diff --git a/modules/core/04-channel/keeper/handshake_test.go b/modules/core/04-channel/keeper/handshake_test.go
@@ -713,6 +713,28 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() {
 			err := path.EndpointA.SetChannelState(types.CLOSED)
 			suite.Require().NoError(err)
 		}, true},
+		{"success with upgrade info", func() {
+			path.Setup()
+			channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+
+			err := path.EndpointA.SetChannelState(types.CLOSED)
+			suite.Require().NoError(err)
+
+			// add mock upgrade info to simulate that the channel is closing during
+			// an upgrade and verify that the upgrade information is deleted
+			upgrade := types.Upgrade{
+				Fields:  types.NewUpgradeFields(types.UNORDERED, []string{ibctesting.FirstConnectionID}, mock.UpgradeVersion),
+				Timeout: types.NewTimeout(clienttypes.ZeroHeight(), 1),
+			}
+
+			counterpartyUpgrade := types.Upgrade{
+				Fields:  types.NewUpgradeFields(types.UNORDERED, []string{ibctesting.FirstConnectionID}, mock.UpgradeVersion),
+				Timeout: types.NewTimeout(clienttypes.ZeroHeight(), 1),
+			}
+
+			path.EndpointB.SetChannelUpgrade(upgrade)
+			path.EndpointB.SetChannelCounterpartyUpgrade(counterpartyUpgrade)
+		}, true},
 		{"channel doesn't exist", func() {
 			// any non-nil values work for connections
 			path.EndpointA.ChannelID = ibctesting.FirstChannelID
@@ -809,13 +831,20 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() {
 			channelKey := host.ChannelKey(path.EndpointA.ChannelConfig.PortID, ibctesting.FirstChannelID)
 			proof, proofHeight := suite.chainA.QueryProof(channelKey)
 
+			ctx := suite.chainB.GetContext()
 			err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.ChanCloseConfirm(
-				suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, ibctesting.FirstChannelID, channelCap,
+				ctx, path.EndpointB.ChannelConfig.PortID, ibctesting.FirstChannelID, channelCap,
 				proof, malleateHeight(proofHeight, heightDiff), counterpartyUpgradeSequence,
 			)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
+
+				// if the channel closed during an upgrade, there should not be any upgrade information
+				_, found := suite.chainB.App.GetIBCKeeper().ChannelKeeper.GetUpgrade(ctx, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+				suite.Require().False(found)
+				_, found = suite.chainB.App.GetIBCKeeper().ChannelKeeper.GetCounterpartyUpgrade(ctx, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+				suite.Require().False(found)
 			} else {
 				suite.Require().Error(err)
 			}

diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go
@@ -174,10 +174,15 @@ func (k Keeper) TimeoutExecuted(
 
 	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.
+		// all upgrade information is deleted 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(), errorsmod.Wrap(types.ErrTimeoutElapsed, "packet timeout elapsed on ORDERED channel"))
+			k.deleteUpgradeInfo(ctx, packet.GetSourcePort(), packet.GetSourceChannel())
+			k.Logger(ctx).Info(
+				"upgrade info deleted",
+				"port_id", packet.GetSourcePort(),
+				"channel_id", packet.GetSourceChannel(),
+				"upgrade_sequence", channel.UpgradeSequence,
+			)
 		}
 
 		channel.State = types.CLOSED

diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go
@@ -506,10 +506,6 @@ func (suite *KeeperTestSuite) TestTimeoutExecuted() {
 				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")
 			},
 			nil,
 		},