ICS4: Fix Sequencing logic in UpgradeTry #5419
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,44 +99,6 @@ func (k Keeper) ChanUpgradeTry( | |
| ) | ||
| } | ||
|
|
||
| // 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 { | ||
| // 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( | ||
|
There was a problem hiding this comment. Its intensional that the channel and upgrade are returned here instead of empty structs, right? This is the only place where an upgrade error is return from the try handler and it looks like the channel returned is used for emitting error receipt events in this case: |
||
| 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 | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -344,6 +344,14 @@ func (chain *TestChain) SendMsgs(msgs ...sdk.Msg) (*abci.ExecTxResult, error) { | |
| // ensure the chain has the latest time | ||
| chain.Coordinator.UpdateTimeForChain(chain) | ||
|
|
||
| // increment acc sequence regardless of success or failure tx execution | ||
|
There was a problem hiding this comment. great find @damiannolan 👀 |
||
| defer func() { | ||
| err := chain.SenderAccount.SetSequence(chain.SenderAccount.GetSequence() + 1) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| }() | ||
|
|
||
| resp, err := simapp.SignAndDeliver( | ||
| chain.TB, | ||
| chain.TxConfig, | ||
|
|
@@ -370,12 +378,6 @@ func (chain *TestChain) SendMsgs(msgs ...sdk.Msg) (*abci.ExecTxResult, error) { | |
| return txResult, fmt.Errorf("%s/%d: %q", txResult.Codespace, txResult.Code, txResult.Log) | ||
| } | ||
|
|
||
| chain.Coordinator.IncrementTime() | ||
|
|
||
| return txResult, nil | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delicious documentation @AdityaSripal, very helpful