From 3da7ba790c213fcd0e0ea72d4b0823d500aaad68 Mon Sep 17 00:00:00 2001 From: Aditya Date: Thu, 25 Jan 2024 17:38:23 +0100 Subject: [PATCH] ICS4: fix off by one error in non-crossing-hello case (#5722) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix off by one error in non-crossing-hello case * Update modules/core/04-channel/keeper/upgrade.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * chore: formatting for linter ci * Update modules/core/04-channel/keeper/upgrade.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * lint --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan (cherry picked from commit bddbe493a0f9df7cc7878f0f74f054cbf4fdd59c) --- modules/core/04-channel/keeper/upgrade.go | 47 +++++++++++-------- .../core/04-channel/keeper/upgrade_test.go | 24 ++++++++++ 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index ae50fcf66c6..e4c29a34560 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -123,30 +123,37 @@ 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 { + // 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 { // 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 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 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 @@ -154,7 +161,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 a4260a6f381..ce430466135 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)