From addad5a957820798003906f1349943f065fff6d3 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Thu, 25 Jan 2024 00:48:38 +0100 Subject: [PATCH 1/5] fix off by one error in non-crossing-hello case --- modules/core/04-channel/keeper/upgrade.go | 43 ++++++++++--------- .../core/04-channel/keeper/upgrade_test.go | 24 +++++++++++ 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 887772a05d5..c259088133c 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -123,30 +123,33 @@ func (k Keeper) ChanUpgradeTry( } var ( - err error - upgrade types.Upgrade - isCrossingHello bool + err error + upgrade types.Upgrade + isCrossingHello bool + expectedUpgradeSequence uint64 ) upgrade, isCrossingHello = k.GetUpgrade(ctx, portID, channelID) - if counterpartyUpgradeSequence < channel.UpgradeSequence { + if isCrossingHello { + expectedUpgradeSequence = channel.UpgradeSequence + } else { + expectedUpgradeSequence = channel.UpgradeSequence + 1 + } + if counterpartyUpgradeSequence < expectedUpgradeSequence { // In this case, the counterparty upgrade is outdated. We want to force the counterparty // to abort their upgrade and come back to sync with our own upgrade sequence. - var upgradeSequence uint64 - if isCrossingHello { - // In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty. - // Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty - // upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1. - // The upgrade handshake may then proceed on the counterparty with our sequence - upgradeSequence = channel.UpgradeSequence - 1 - } else { - // In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side), - // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. - // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty - // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either - // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1) - upgradeSequence = channel.UpgradeSequence - } + + // In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty. + // Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty + // upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1. + // Since our channel upgrade sequence was incremented in the previous step, the counterparty will be forced to abort their upgrade + // and we will be able to proceed with our own upgrade. + // The upgrade handshake may then proceed on the counterparty with our sequence + // In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side), + // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. + // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty + // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either + // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1) // NOTE: Two possible outcomes may occur in this scenario. // The ChanUpgradeCancel datagram may reach the counterparty first, which will cause the counterparty to cancel. The counterparty @@ -154,7 +157,7 @@ func (k Keeper) ChanUpgradeTry( // The ChanUpgradeTry message may arrive first, in this case, **IF** the upgrade fields are mutually compatible; the counterparty will simply // fast forward their sequence to our own and continue the upgrade. The following ChanUpgradeCancel message will be rejected as it is below the current sequence. - return channel, upgrade, types.NewUpgradeError(upgradeSequence, errorsmod.Wrapf( + return channel, upgrade, types.NewUpgradeError(expectedUpgradeSequence-1, errorsmod.Wrapf( types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence < current upgrade sequence (%d < %d)", counterpartyUpgradeSequence, channel.UpgradeSequence, )) } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 46af50d4a5a..a8185ffe929 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -237,6 +237,30 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { // channel sequence will be returned so that counterparty inits on completely fresh sequence for both sides types.NewUpgradeError(5, types.ErrInvalidUpgradeSequence), }, + { + "fails due to mismatch in upgrade sequences: chainB is on incremented sequence without an upgrade indicating it has already processed upgrade at this sequence.", + func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 1 + errorReceipt := types.NewUpgradeError(1, types.ErrInvalidUpgrade) + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.WriteErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt) + path.EndpointB.SetChannel(channel) + }, + types.NewUpgradeError(1, types.ErrInvalidUpgradeSequence), + }, + { + "fails due to mismatch in upgrade sequences, crossing hello with the TRY chain having a higher sequence", + func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + + // upgrade sequence is 5 after this call + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + }, + types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence), + }, { // ChainA(Sequence: 0, mock-version-v2), ChainB(Sequence: 0, mock-version-v3) // ChainA.INIT(Sequence: 1) From e8b2e0a2c7029ae94a22eaf911ed0432e55755cd Mon Sep 17 00:00:00 2001 From: Aditya Date: Thu, 25 Jan 2024 12:56:03 +0100 Subject: [PATCH 2/5] Update modules/core/04-channel/keeper/upgrade.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/core/04-channel/keeper/upgrade.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 3cc5842ed7c..1ac871b95a9 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -133,6 +133,9 @@ func (k Keeper) ChanUpgradeTry( if isCrossingHello { expectedUpgradeSequence = channel.UpgradeSequence } else { + // at the end of the TRY step, the current upgrade sequence will be incremented in the non-crossing + // hello case due to calling chanUpgradeInit, we should use this expected upgrade sequence for + // sequence mismatch comparison expectedUpgradeSequence = channel.UpgradeSequence + 1 } if counterpartyUpgradeSequence < expectedUpgradeSequence { From 0502fe7e6adfad84cad0cfe43e92e801ebb45458 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 25 Jan 2024 15:27:30 +0100 Subject: [PATCH 3/5] chore: formatting for linter ci --- modules/core/04-channel/keeper/upgrade.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 1ac871b95a9..8889ecd64aa 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -133,9 +133,9 @@ func (k Keeper) ChanUpgradeTry( if isCrossingHello { expectedUpgradeSequence = channel.UpgradeSequence } else { - // at the end of the TRY step, the current upgrade sequence will be incremented in the non-crossing - // hello case due to calling chanUpgradeInit, we should use this expected upgrade sequence for - // sequence mismatch comparison + // at the end of the TRY step, the current upgrade sequence will be incremented in the non-crossing + // hello case due to calling chanUpgradeInit, we should use this expected upgrade sequence for + // sequence mismatch comparison expectedUpgradeSequence = channel.UpgradeSequence + 1 } if counterpartyUpgradeSequence < expectedUpgradeSequence { From fbf3ed9ccaab8f4c76683bec201111c6f9e51b62 Mon Sep 17 00:00:00 2001 From: Aditya Date: Thu, 25 Jan 2024 15:57:28 +0100 Subject: [PATCH 4/5] Update modules/core/04-channel/keeper/upgrade.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/core/04-channel/keeper/upgrade.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 8889ecd64aa..c66637aa5d5 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -148,11 +148,12 @@ func (k Keeper) ChanUpgradeTry( // Since our channel upgrade sequence was incremented in the previous step, the counterparty will be forced to abort their upgrade // and we will be able to proceed with our own upgrade. // The upgrade handshake may then proceed on the counterparty with our sequence - // In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side), + // In the non-crossing hello (i.e. upgrade does not exist on our side), // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either - // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1) + // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1). + // Note that expectedUpgradeSequence - 1 == channel.UpgradeSequence in the non-crossing hello case. // NOTE: Two possible outcomes may occur in this scenario. // The ChanUpgradeCancel datagram may reach the counterparty first, which will cause the counterparty to cancel. The counterparty From c710ddac9a714c796fb45d7b5e1ec24824322e13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 25 Jan 2024 16:55:15 +0100 Subject: [PATCH 5/5] lint --- modules/core/04-channel/keeper/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index c66637aa5d5..e4c29a34560 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -152,7 +152,7 @@ func (k Keeper) ChanUpgradeTry( // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either - // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1). + // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1). // Note that expectedUpgradeSequence - 1 == channel.UpgradeSequence in the non-crossing hello case. // NOTE: Two possible outcomes may occur in this scenario.