startFlushUpgradeHandshake #3671
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
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 |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "reflect" | ||
|
|
||
| errorsmod "cosmossdk.io/errors" | ||
| sdkmath "cosmossdk.io/math" | ||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
||
|
|
@@ -109,6 +110,110 @@ func (k Keeper) WriteUpgradeTryChannel( | |
| emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) | ||
| } | ||
|
|
||
| // startFlushUpgradeHandshake will verify the counterparty proposed upgrade and the current channel state. | ||
| // Once the counterparty information has been verified, it will be validated against the self proposed upgrade. | ||
| // If any of the proposed upgrade fields are incompatible, an upgrade error will be returned resulting in an | ||
| // aborted upgrade. | ||
| // | ||
| //lint:ignore U1000 Ignore unused function temporarily for debugging | ||
| func (k Keeper) startFlushUpgradeHandshake( | ||
|
There was a problem hiding this comment. My understanding is that the writes that happen in the spec will happen in a separate function in the implementation? There was a problem hiding this comment. they will happen in the |
||
| ctx sdk.Context, | ||
| portID, | ||
| channelID string, | ||
| proposedUpgradeFields types.UpgradeFields, | ||
| counterpartyChannel types.Channel, | ||
| counterpartyUpgrade types.Upgrade, | ||
| proofCounterpartyChannel, | ||
| proofCounterpartyUpgrade []byte, | ||
| proofHeight clienttypes.Height, | ||
| ) error { | ||
| channel, found := k.GetChannel(ctx, portID, channelID) | ||
| if !found { | ||
| return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) | ||
| } | ||
|
|
||
| connection, err := k.GetConnection(ctx, channel.ConnectionHops[0]) | ||
| if err != nil { | ||
| return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") | ||
| } | ||
|
|
||
| if connection.GetState() != int32(connectiontypes.OPEN) { | ||
| return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String()) | ||
| } | ||
|
|
||
| // verify the counterparty channel state containing the upgrade sequence | ||
| if err := k.connectionKeeper.VerifyChannelState( | ||
| ctx, | ||
| connection, | ||
| proofHeight, proofCounterpartyChannel, | ||
| channel.Counterparty.PortId, | ||
| channel.Counterparty.ChannelId, | ||
| counterpartyChannel, | ||
| ); err != nil { | ||
| return errorsmod.Wrap(err, "failed to verify counterparty channel state") | ||
| } | ||
|
|
||
| // verifies the proof that a particular proposed upgrade has been stored in the upgrade path of the counterparty | ||
| if err := k.connectionKeeper.VerifyChannelUpgrade( | ||
| ctx, | ||
| connection, | ||
| proofHeight, proofCounterpartyUpgrade, | ||
| channel.Counterparty.PortId, | ||
| channel.Counterparty.ChannelId, | ||
| counterpartyUpgrade, | ||
| ); err != nil { | ||
| return errorsmod.Wrap(err, "failed to verify counterparty upgrade") | ||
| } | ||
|
|
||
| // the current upgrade handshake must only continue if both channels are using the same upgrade sequence, | ||
| // otherwise an error receipt must be written so that the upgrade handshake may be attempted again with synchronized sequences | ||
| if counterpartyChannel.UpgradeSequence != channel.UpgradeSequence { | ||
| // save the previous upgrade sequence for the error message | ||
| prevUpgradeSequence := channel.UpgradeSequence | ||
|
|
||
| // error on the higher sequence so that both chains synchronize on a fresh sequence | ||
| channel.UpgradeSequence = sdkmath.Max(counterpartyChannel.UpgradeSequence, channel.UpgradeSequence) | ||
| k.SetChannel(ctx, portID, channelID, channel) | ||
|
|
||
| return types.NewUpgradeError(channel.UpgradeSequence, errorsmod.Wrapf( | ||
| types.ErrIncompatibleCounterpartyUpgrade, "expected upgrade sequence (%d) to match counterparty upgrade sequence (%d)", prevUpgradeSequence, counterpartyChannel.UpgradeSequence), | ||
| ) | ||
| } | ||
|
|
||
| // assert that both sides propose the same channel ordering | ||
|
There was a problem hiding this comment. if we think we might need to add more checks in the future, we could consider making a helper function for all the below checks to validate the counterparty upgrade against the self upgrade, maybe something like: err := k.checkUpgradesForCompatibility(proposedUpgrade, counterpartyUpgrade)
if err != nil {
return types.NewUpgradeError(channel.UpgradeSequence, err)
}There was a problem hiding this comment. I like this suggestion a lot, this also adds a clear location for where all checks can go, and also clarifies the purpose of certain checks a lot more. |
||
| if proposedUpgradeFields.Ordering != counterpartyUpgrade.Fields.Ordering { | ||
| return types.NewUpgradeError(channel.UpgradeSequence, errorsmod.Wrapf( | ||
| types.ErrIncompatibleCounterpartyUpgrade, "expected upgrade ordering (%s) to match counterparty upgrade ordering (%d)", proposedUpgradeFields.Ordering, counterpartyUpgrade.Fields.Ordering), | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
| } | ||
|
|
||
| proposedConnection, err := k.GetConnection(ctx, proposedUpgradeFields.ConnectionHops[0]) | ||
| if err != nil { | ||
| // NOTE: this error is expected to be unreachable as the proposed upgrade connectionID should have been | ||
| // validated in the upgrade INIT and TRY handlers | ||
|
There was a problem hiding this comment. What's the reason to validate in both places? The spec seems to be doing the validation only in this function and not in the handlers, right? If we keep validation in the handlers then behaviour of the implementation deviates from the spec because the implementation will fail the transaction, but not write the error receipt, right? There was a problem hiding this comment. we need to use the proposedConnection in the handlers to construct the There was a problem hiding this comment. The spec does this check twice: // initUpgradeHandshake
...
// proposedConnection must exist and be in OPEN state for
// channel upgrade to be accepted
proposedConnection = provableStore.Get(connectionPath(proposedUpgradeFields.connectionHops[0])
abortTransactionUnless(proposedConnection != null && proposedConnection.state == OPEN)and // startFlushUpgradeHandshake
...
// connectionHops can change in a channelUpgrade, however both sides must still be each other's counterparty.
proposedConnection = provableStore.get(connectionPath(proposedUpgradeFields.connectionHops[0])
if (proposedConnection == null || proposedConnection.state != OPEN) {
restoreChannel(portIdentifier, channelIdentifier)
} |
||
| return types.NewUpgradeError(channel.UpgradeSequence, errorsmod.Wrap( | ||
| err, "expected proposed connection to be found"), | ||
| ) | ||
| } | ||
|
|
||
| if proposedConnection.GetState() != int32(connectiontypes.OPEN) { | ||
| // NOTE: this error is expected to be unreachable as the proposed upgrade connectionID should have been | ||
| // validated in the upgrade INIT and TRY handlers | ||
| return types.NewUpgradeError(channel.UpgradeSequence, errorsmod.Wrapf( | ||
| connectiontypes.ErrInvalidConnectionState, "expected proposed connection to be OPEN (got %s)", connectiontypes.State(proposedConnection.GetState()).String()), | ||
| ) | ||
| } | ||
|
|
||
| // connectionHops can change in a channelUpgrade, however both sides must still be each other's counterparty. | ||
| if counterpartyUpgrade.Fields.ConnectionHops[0] != proposedConnection.GetCounterparty().GetConnectionID() { | ||
| return types.NewUpgradeError(channel.UpgradeSequence, errorsmod.Wrapf( | ||
| types.ErrIncompatibleCounterpartyUpgrade, "counterparty upgrade connection end is not a counterparty of self proposed connection end (%s != %s)", counterpartyUpgrade.Fields.ConnectionHops[0], proposedConnection.GetCounterparty().GetConnectionID()), | ||
| ) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // validateUpgradeFields validates the proposed upgrade fields against the existing channel. | ||
| // It returns an error if the following constraints are not met: | ||
| // - there exists at least one valid proposed change to the existing channel fields | ||
|
|
||
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
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
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.
is there a strong reason that we don't just want to let
StartFlushUpgradeHandshakebe an exported function and not need these wrappers?To me, adding this additional level of indirection hurts my ability to reason about where things can be used in certain ways and why.
I don't think it's so bad to just export
k.startFlushUpgradeHandshake.This is a mild preference though, if the general consensus is to have this additional wrapper I'm okay with it 👍
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.
I did a quick check on this since it wasn't clear if having the function in a
_test.gofile would prevent it being used.I added the method sig to the expected keeper interface of
ChannelKeeperin transfer and there was no compiler issues and you could call the method with no issues. So to me, it seems like just making it exported from the get go is the same thingThere 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.
wait, the function should not be added to the binary in non-test mode, this is in
go help build:the compiler as in the LSP that does checking in the IDE or does a
go buildsucceed? (the second case would sound like a serious bug 😄, edit: well, so would the first, actually)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.
You sir are correct. It was the LSP that has me misled. A
make installfails rightly as you say!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.
startFlushUpgradeHandshakeshould only be used during the upgrade handshake and thus does not need to be referenced outside of the04-channel/keeperpackage. Given that our versioning is dependent upon API breaking changes, it gives us more flexibility to handle fixes without incrementing our major version if this function is unexported. In my opinion, only theMsg's and grpc handlers should be exported. It is unsafe to reference this function outside of the context it is called in (the upgrade message handlers).I think in general we should aim to limit our public API as it gives us maximum development flexibility while also establishing a clear public API external developers can rely on. Unless there's a reason to make the function public, I think it is healthy to default to leaving the function private