From fc0bfab227d0787b3801b00f453f8a5d60ad4309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=90=E1=BB=97=20Vi=E1=BB=87t=20Ho=C3=A0ng?= Date: Tue, 6 Feb 2024 19:43:14 +0700 Subject: [PATCH] Feat: Delete upgrade information only rather than calling abort in TimeoutExecuted (#5764) * delete instead of abort * no longer have error receipt * nit: Remove stale inline comment Co-authored-by: Carlos Rodriguez * 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 Co-authored-by: Carlos Rodriguez --- modules/core/04-channel/keeper/handshake.go | 11 +++++++ .../core/04-channel/keeper/handshake_test.go | 31 ++++++++++++++++++- modules/core/04-channel/keeper/timeout.go | 11 +++++-- .../core/04-channel/keeper/timeout_test.go | 4 --- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/handshake.go b/modules/core/04-channel/keeper/handshake.go index fdfae21f944..a4c6ed57f51 100644 --- 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 index f99db4eac47..08b558d159d 100644 --- 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 index 169822a1875..7bf2677d770 100644 --- 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 index d6154d7aec1..0749a719569 100644 --- 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, },