ICS4: Fix Sequencing logic in UpgradeTry (#5419)

* fix sequencing logic and unit tests * added tests * reorder comments * add test * fix typo * refactored a couple of tests * relocate comment * fix: defer incrementing acc sequencing such that is happens on both success and failure tx execution * chore: update version strings in comments * test: wrap test code in suite.Run and add additional assertions * refactor: adapt crossing hellos cancellation test case to use suite.Run and add more assertions --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
cosmos · Dec 20, 2023 · ab1f96f · ab1f96f
1 parent ef9a479
commit ab1f96f
Show file tree

Hide file tree

Showing 3 changed files with 342 additions and 52 deletions.
diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go
@@ -99,44 +99,6 @@ func (k Keeper) ChanUpgradeTry(
 		)
 	}
 
-	// construct counterpartyChannel from existing information and provided counterpartyUpgradeSequence
-	// create upgrade fields from counterparty proposed upgrade and own verified connection hops
-	proposedUpgradeFields := types.UpgradeFields{
-		Ordering:       counterpartyUpgradeFields.Ordering,
-		ConnectionHops: proposedConnectionHops,
-		Version:        counterpartyUpgradeFields.Version,
-	}
-
-	var (
-		err     error
-		upgrade types.Upgrade
-	)
-
-	// NOTE: if an upgrade exists (crossing hellos) then use existing upgrade fields
-	// otherwise, run the upgrade init sub-protocol
-	upgrade, found = k.GetUpgrade(ctx, portID, channelID)
-	if found {
-		proposedUpgradeFields = upgrade.Fields
-	} else {
-		// NOTE: OnChanUpgradeInit will not be executed by the application
-		upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields)
-		if err != nil {
-			return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade")
-		}
-
-		channel, upgrade = k.WriteUpgradeInitChannel(ctx, portID, channelID, upgrade, upgrade.Fields.Version)
-
-		// if the counterparty sequence is greater than the current sequence, we fast-forward to the counterparty sequence.
-		if counterpartyUpgradeSequence > channel.UpgradeSequence {
-			channel.UpgradeSequence = counterpartyUpgradeSequence
-			k.SetChannel(ctx, portID, channelID, channel)
-		}
-	}
-
-	if err := k.checkForUpgradeCompatibility(ctx, proposedUpgradeFields, counterpartyUpgradeFields); err != nil {
-		return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed upgrade compatibility check")
-	}
-
 	// construct expected counterparty channel from information in state
 	// only the counterpartyUpgradeSequence is provided by the relayer
 	counterpartyConnectionHops := []string{connection.GetCounterparty().GetConnectionID()}
@@ -161,8 +123,39 @@ func (k Keeper) ChanUpgradeTry(
 		return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to verify counterparty channel state")
 	}
 
+	var (
+		err             error
+		upgrade         types.Upgrade
+		isCrossingHello bool
+	)
+
+	upgrade, isCrossingHello = k.GetUpgrade(ctx, portID, channelID)
 	if counterpartyUpgradeSequence < channel.UpgradeSequence {
-		return channel, upgrade, types.NewUpgradeError(channel.UpgradeSequence-1, errorsmod.Wrapf(
+		// 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
+		}
+
+		// 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
+		// may then receive a TRY with our channel upgrade sequence and correctly increment their sequence to become synced with our upgrade attempt.
+		// 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(
 			types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence < current upgrade sequence (%d < %d)", counterpartyUpgradeSequence, channel.UpgradeSequence,
 		))
 	}
@@ -179,6 +172,38 @@ func (k Keeper) ChanUpgradeTry(
 		return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to verify counterparty upgrade")
 	}
 
+	// construct counterpartyChannel from existing information and provided counterpartyUpgradeSequence
+	// create upgrade fields from counterparty proposed upgrade and own verified connection hops
+	proposedUpgradeFields := types.UpgradeFields{
+		Ordering:       counterpartyUpgradeFields.Ordering,
+		ConnectionHops: proposedConnectionHops,
+		Version:        counterpartyUpgradeFields.Version,
+	}
+
+	// NOTE: if an upgrade exists (crossing hellos) then use existing upgrade fields
+	// otherwise, run the upgrade init sub-protocol
+	if isCrossingHello {
+		proposedUpgradeFields = upgrade.Fields
+	} else {
+		// NOTE: OnChanUpgradeInit will not be executed by the application
+		upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields)
+		if err != nil {
+			return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade")
+		}
+
+		channel, upgrade = k.WriteUpgradeInitChannel(ctx, portID, channelID, upgrade, upgrade.Fields.Version)
+	}
+
+	if err := k.checkForUpgradeCompatibility(ctx, proposedUpgradeFields, counterpartyUpgradeFields); err != nil {
+		return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed upgrade compatibility check")
+	}
+
+	// if the counterparty sequence is greater than the current sequence, we fast-forward to the counterparty sequence.
+	if counterpartyUpgradeSequence > channel.UpgradeSequence {
+		channel.UpgradeSequence = counterpartyUpgradeSequence
+		k.SetChannel(ctx, portID, channelID, channel)
+	}
+
 	if err := k.startFlushing(ctx, portID, channelID, &upgrade); err != nil {
 		return types.Channel{}, types.Upgrade{}, err
 	}