From 55042f9d69c65fc49e33db5a5ed40544926de5c2 Mon Sep 17 00:00:00 2001 From: hoangdv2429 Date: Mon, 29 Jan 2024 17:16:28 +0700 Subject: [PATCH 1/6] delete instead of abort --- modules/core/04-channel/keeper/timeout.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index 15a7091ac09..3bb623ec01f 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -177,7 +177,7 @@ func (k Keeper) TimeoutExecuted( // 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(), errorsmod.Wrap(types.ErrTimeoutElapsed, "packet timeout elapsed on ORDERED channel")) + k.deleteUpgradeInfo(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) } channel.State = types.CLOSED From e70d16101d0444f5a442da027601e97698ea80f6 Mon Sep 17 00:00:00 2001 From: hoangdv2429 Date: Tue, 30 Jan 2024 00:54:04 +0700 Subject: [PATCH 2/6] no longer have error receipt --- modules/core/04-channel/keeper/timeout_test.go | 4 ---- 1 file changed, 4 deletions(-) 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, }, From bdf3a89ce3f578d9ef18c0558fb9d50b31385705 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 2 Feb 2024 10:18:34 +0200 Subject: [PATCH 3/6] nit: Remove stale inline comment Co-authored-by: Carlos Rodriguez --- modules/core/04-channel/keeper/timeout.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index ff4d71fd286..c46b3be3edd 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -176,7 +176,6 @@ func (k Keeper) TimeoutExecuted( // 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.deleteUpgradeInfo(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) } From fc3c1bdb6167b9667decdf7805f911a9d22d502c Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 2 Feb 2024 10:21:20 +0200 Subject: [PATCH 4/6] nit: tweak previous comment to remove reference to aborting upgrade. --- modules/core/04-channel/keeper/timeout.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go index c46b3be3edd..d9ef794ff65 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -174,7 +174,7 @@ 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 { k.deleteUpgradeInfo(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) } From e7abc45b95e9ba61d62b4f1fbeed80d0273d0f9f Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 6 Feb 2024 10:02:04 +0100 Subject: [PATCH 5/6] add log and delete upgrade info in chan close confirm --- modules/core/04-channel/keeper/handshake.go | 11 +++++++ .../core/04-channel/keeper/handshake_test.go | 31 ++++++++++++++++++- modules/core/04-channel/keeper/timeout.go | 6 ++++ 3 files changed, 47 insertions(+), 1 deletion(-) 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..a39e70d9d4f 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.EndpointA.ChannelConfig.PortID, ibctesting.FirstChannelID) + suite.Require().False(found) + _, found = suite.chainB.App.GetIBCKeeper().ChannelKeeper.GetCounterpartyUpgrade(ctx, path.EndpointA.ChannelConfig.PortID, ibctesting.FirstChannelID) + 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 cbc44fbcfa0..7bf2677d770 100644 --- a/modules/core/04-channel/keeper/timeout.go +++ b/modules/core/04-channel/keeper/timeout.go @@ -177,6 +177,12 @@ func (k Keeper) TimeoutExecuted( // all upgrade information is deleted and the channel is set to CLOSED. if channel.State == types.FLUSHING { 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 From 3783276f4583279f23568dac1df6c73f130d1dfd Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 6 Feb 2024 11:22:30 +0100 Subject: [PATCH 6/6] use correct port id, channel id --- modules/core/04-channel/keeper/handshake_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/handshake_test.go b/modules/core/04-channel/keeper/handshake_test.go index a39e70d9d4f..08b558d159d 100644 --- a/modules/core/04-channel/keeper/handshake_test.go +++ b/modules/core/04-channel/keeper/handshake_test.go @@ -841,9 +841,9 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() { 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.EndpointA.ChannelConfig.PortID, ibctesting.FirstChannelID) + _, 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.EndpointA.ChannelConfig.PortID, ibctesting.FirstChannelID) + _, found = suite.chainB.App.GetIBCKeeper().ChannelKeeper.GetCounterpartyUpgrade(ctx, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) suite.Require().False(found) } else { suite.Require().Error(err)