Implement Channel Upgrade Try functionality for 04 channel #3333
Conversation
…/1618-chan-upgrade-init
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
…datebasic, pull in subset ordering suggestions and adding tests
…-verifychannelupgradesequence-method-to-03-connection
…-to-03-connection' into cian/issue#1619-implement-channelupgradetry-functionality-for-04-channel
There was a problem hiding this comment.
Great work so far, @charleenfei and @chatton. This handler is quite a beast! 🐻 I left some comments for now, will take another look again in a couple of days. :)
|
|
||
| if proposedUpgradeChannel.Counterparty.PortId != channel.Counterparty.PortId || | ||
| proposedUpgradeChannel.Counterparty.ChannelId != channel.Counterparty.ChannelId { | ||
| return 0, "", errorsmod.Wrap(types.ErrInvalidChannel, "proposed channel upgrade is invalid") |
There was a problem hiding this comment.
Should we use same error message as in INIT?
| if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { | ||
| return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||
| } | ||
| return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, "upgrade timed out") |
There was a problem hiding this comment.
Should we have a similar error message as for receive packet?
| if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { | ||
| return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||
| } | ||
| return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, "upgrade timed out") |
There was a problem hiding this comment.
Ditto about the error message in receive packet.
| upgradeSequence = counterpartyUpgradeSequence | ||
| k.SetUpgradeSequence(ctx, portID, channelID, upgradeSequence) | ||
| } else { | ||
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not smaller than the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) |
There was a problem hiding this comment.
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not smaller than the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) | |
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "counterparty chain upgrade sequence <= upgrade sequence (%d <= %d)", counterpartyUpgradeSequence, upgradeSequence)) |
| case types.INITUPGRADE: | ||
| upgradeSequence, found = k.GetUpgradeSequence(ctx, portID, channelID) | ||
| if !found { | ||
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not smaller than the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) |
There was a problem hiding this comment.
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not smaller than the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) | |
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrap(types.ErrUpgradeAborted, "upgrade sequence not found") |
| } | ||
|
|
||
| if upgradeSequence != counterpartyUpgradeSequence { | ||
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not equal to the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) |
There was a problem hiding this comment.
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not equal to the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) | |
| errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence ≠ counterparty chain upgrade sequence (%d ≠ %d)", upgradeSequence, counterpartyUpgradeSequence)) |
| } | ||
|
|
||
| k.SetUpgradeErrorReceipt(ctx, portID, channelID, errorReceipt) | ||
| return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade aborted, error receipt written for upgrade sequence: %d", upgradeSequence) |
There was a problem hiding this comment.
The upgrade sequence mentioned in this error message could be different than the one in the error receipt if it gets updated in the if clause just above.
| // except for the channel state (upgrade channel will be in TRYUPGRADE and current channel will be in INITUPGRADE) | ||
| // if the proposed upgrades on either side are incompatible, then we will restore the channel and cancel the upgrade. | ||
| channel.State = types.TRYUPGRADE | ||
| k.SetChannel(ctx, portID, channelID, channel) |
There was a problem hiding this comment.
Should this SetChannel (maybe for clarity) be done at the end?
There was a problem hiding this comment.
The updated channel state is used just a few lines down and I think separating them out makes things a little harder to follow in this case
There was a problem hiding this comment.
Yes, but to use the channel state you don't need it stored, right? For me it's just that I think it would be more clear if you set the channel when all the checks have passed (like in upgrade init). I know that if there's an error, the state changes will be reverted, but feels like the code would be more clear if it's at the end, instead of in between this complicated logic.
There was a problem hiding this comment.
I tend to agree with @chatton that it is clearer to keep the logic together, rather than having the channel state being updated in one place and then actually set further down with the potential restore channel logic happening in between
| } | ||
|
|
||
| // ChanUpgradeTry is called by a module to accept the first step of a channel upgrade | ||
| // handshake initiated by a module on another chain. |
There was a problem hiding this comment.
Maybe it would be good to add something here in the good about the upgrade sequence that is returned, because if this function returns a not nil error, then upgradeSequence will be zero, but in fact the upgrade sequence stored could be different.
| ErrChannelUpgradeRestoreFailure = errorsmod.Register(SubModuleName, 29, "failed to restore channel during upgrade") | ||
| ErrUpgradeAborted = errorsmod.Register(SubModuleName, 30, "upgrade aborted") | ||
| ErrUpgradeTimeout = errorsmod.Register(SubModuleName, 31, "upgrade timeout") | ||
| ErrUpgradeErrorNotFound = errorsmod.Register(SubModuleName, 32, "upgrade error receipt not found") |
There was a problem hiding this comment.
| ErrUpgradeErrorNotFound = errorsmod.Register(SubModuleName, 32, "upgrade error receipt not found") | |
| ErrUpgradeErrorReceiptNotFound = errorsmod.Register(SubModuleName, 32, "upgrade error receipt not found") |
…nnelupgradetry-functionality-for-04-channel
| types.EventTypeChannelUpgradeTry, | ||
| sdk.NewAttribute(types.AttributeKeyPortID, portID), | ||
| sdk.NewAttribute(types.AttributeKeyChannelID, channelID), | ||
| sdk.NewAttribute(types.AttributeCounterpartyPortID, channel.Counterparty.PortId), | ||
| sdk.NewAttribute(types.AttributeCounterpartyChannelID, channel.Counterparty.ChannelId), | ||
| sdk.NewAttribute(types.AttributeKeyConnectionID, channel.ConnectionHops[0]), | ||
| sdk.NewAttribute(types.AttributeVersion, channel.Version), | ||
| sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", upgradeSequence)), |
There was a problem hiding this comment.
we should emit the channel order as well since it can change. Looks like it is missing in Init as well
| defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-init") | ||
|
|
||
| channel, found := k.GetChannel(ctx, portID, channelID) | ||
| if !found { | ||
| return errorsmod.Wrapf(types.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID) |
There was a problem hiding this comment.
This should panic in my opinion. Then we can remove the error on this function. Write/Set functions should either succeed or panic
| channel.Ordering = channelUpgrade.Ordering | ||
| channel.ConnectionHops = channelUpgrade.ConnectionHops | ||
|
|
||
| // TODO: do a deep equal on channel / channelUpgrade and return a failure? |
There was a problem hiding this comment.
I don't think this is necessary, our existing handlers are responsible for validation of the provided argument
| if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) { | ||
| return 0, "", errorsmod.Wrapf(types.ErrChannelCapabilityNotFound, "caller does not own capability for channel, port ID (%s) channel ID (%s)", portID, channelID) | ||
| } |
There was a problem hiding this comment.
note: I'm fairly certain this is unnecessary, see spec issue. I would be so happy to see these unnecessary checks removed 😄
| return 0, "", errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) | ||
| } | ||
|
|
||
| if !collections.Contains(channel.State, []types.State{types.OPEN, types.INITUPGRADE}) { |
There was a problem hiding this comment.
this looks really nice. Should we add a comment indicating that this is accounting for crossing hellos?
| portID string, | ||
| channelID string, | ||
| chanCap *capabilitytypes.Capability, | ||
| counterpartyChannel types.Channel, |
There was a problem hiding this comment.
counterpartyUpgradeChannel?
| if err := k.connectionKeeper.VerifyChannelUpgradeSequence(ctx, connectionEnd, proofHeight, proofUpgradeSequence, proposedUpgradeChannel.Counterparty.PortId, | ||
| proposedUpgradeChannel.Counterparty.ChannelId, counterpartyUpgradeSequence); err != nil { |
There was a problem hiding this comment.
ditto on Counterparty.PortId
| // check if upgrade timed out by comparing it with the latest height of the chain | ||
| selfHeight := clienttypes.GetSelfHeight(ctx) | ||
| if !timeoutHeight.IsZero() && selfHeight.GTE(timeoutHeight) { | ||
| if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { | ||
| return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||
| } | ||
| return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, "block height >= upgrade timeout height (%s >= %s)", selfHeight, timeoutHeight) | ||
| } | ||
|
|
||
| // check if upgrade timed out by comparing it with the latest timestamp of the chain | ||
| if timeoutTimestamp != 0 && uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp { | ||
| if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { | ||
| return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||
| } | ||
| return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, "block timestamp >= upgrade timeout timestamp (%s >= %s)", ctx.BlockTime(), time.Unix(0, int64(timeoutTimestamp))) | ||
| } |
There was a problem hiding this comment.
Lets add a helper function to the Timeout type. This function should take in the current height and the current timestamp, it should return an error if the timeout has passed. The error can then indicate if it is the block height or the timestamp that has passed (or potentially both), but it appears the same logic is performed within both if statements:
if timeoutInfo, hasPassed := timeout.Status(currentHeight, currentTimestamp); hasPassed {
// restore channel logic
return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, timeoutInfo)
}This could be done in a follow up pr
| return 0, "", errorsmod.Wrapf(connectiontypes.ErrInvalidConnection, "unexpected counterparty channel connection hops, expected %s but got %s", connectionEnd.GetCounterparty().GetConnectionID(), counterpartyChannel.ConnectionHops[0]) | ||
| } | ||
|
|
||
| if err := k.connectionKeeper.VerifyChannelState(ctx, connectionEnd, proofHeight, proofChannel, proposedUpgradeChannel.Counterparty.PortId, |
There was a problem hiding this comment.
So we are verifying 3 things:
- channel
- sequence
- timeout
From my understanding, there is no particular reason that sequence and timeout are separated within the provable store (they could be under the same upgrade type). The channel is under its' own path for no strong reason, but one benefit is that you can be certain the counterparty has paused packet processing. There could be a construction where all 3 are under the same provable upgrade type:
interface Upgrade {
proposedChannel Channel
sequence uint64
timeout Timeout
}
We could add a k.connectionKeeper.VerifyUpgrade which does all 3, but I don't have a strong preference, just mentioning as food for thought. The main downside of such a function is passing 3 proofs/values at once. The argument list would be very large:
(ctx, connectionEnd, channel.Counterparty.PortId, channel.Counterparty.ChannelId, proofHeight, proofChannel, counterpartyChannel, proofUpgradeTimeout, upgradeTimeout, proofUpgradeSequence, upgradeSequenceAfter seeing that argument list, I think I actually prefer doing the split here
|
|
||
| switch channel.State { | ||
| case types.OPEN: | ||
| upgradeSequence = uint64(0) |
There was a problem hiding this comment.
shouldn't it actually still be 0 because we are in channel state OPEN? we update the sequence further down the code block
| } | ||
|
|
||
| // check if upgrade timed out by comparing it with the latest timestamp of the chain | ||
| if timeoutTimestamp != 0 && uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp { |
There was a problem hiding this comment.
Should we move this check up and just abort the tx, the relayer can call timout at this point if needed.
However, we get into an awkward situation if we are also at UPGRADE_INIT, then we would have to wait for timeout on the other side as well
modules/core/keeper/msg_server.go
Outdated
| return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) | ||
| } | ||
|
|
||
| upgradeSequence, previousVersion, err := k.ChannelKeeper.ChanUpgradeTry( |
There was a problem hiding this comment.
I still prefer getting the previousVersion separately in a new store call.
I know its more gas, but just sat for a couple minutes wondering why this would return previous version.
There was a problem hiding this comment.
Actually technically it's not more gas, the current code was doing a call to the restore channel in order to return it anyway, I'm happy enough to pull this out to the msg server level.
| channel.State = types.TRYUPGRADE | ||
| channel.Version = proposedUpgradeVersion | ||
| channel.Ordering = channelUpgrade.Ordering | ||
| channel.ConnectionHops = channelUpgrade.ConnectionHops |
There was a problem hiding this comment.
Nit: this code updating the channel fields is duplicated in WriteUpgradeInitChannel. Maybe we can have a method on the channel end struct to update all upgradable fields?
There was a problem hiding this comment.
Are you happy if we introduce a helper function in a follow up?
There was a problem hiding this comment.
Yes, more than happy! :)
| @@ -726,22 +726,92 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC | |||
| return nil, errorsmod.Wrapf(err, "channel upgrade init callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId) | |||
| } | |||
|
|
|||
| msg.ProposedUpgradeChannel.Version = version | |||
There was a problem hiding this comment.
Why was this removed? In WriteUpgradeInitChannel the version of the channel is updated with the one in the proposed upgrade channel, but without this, we wouldn't take into account the version returned by the application callback. Maybe instead of modifying the proposed upgrade channel, the version can be passed as a separate argument, as it is done in this PR for WriteUpgradeTryChannel.
There was a problem hiding this comment.
Nice catch! It looks like this is an oversight from a refactor that we did to WriteUpgradeTryChannel
that refactor was intended to be more explicit by not modifying the input message and passing the version directly. I think we should still not modify the message, but we should pass the version into WriteUpgradeInit in the same way we do in the linked code.
| // check if upgrade timed out by comparing it with the latest height of the chain | ||
| selfHeight := clienttypes.GetSelfHeight(ctx) | ||
| if !timeoutHeight.IsZero() && selfHeight.GTE(timeoutHeight) { | ||
| if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { |
There was a problem hiding this comment.
Restoring the channel and writing a receipt here would only be needed if the channel state is in INITUPGRADE, right? (crossing hello). Otherwise, just aborting the transaction and calling timeout on the counterparty would be enough, right? I think the same applies for the timestamp check below.
There was a problem hiding this comment.
yes you're right, this actually ended up happening in #3418 which is probably not where that change should have been introduced 😅
…ty-for-04-channel' of github.com:cosmos/ibc-go into channel-upgrade-try
…ty-for-04-channel' of https://github.com/cosmos/ibc-go into cian/issue#1619-implement-channelupgradetry-functionality-for-04-channel
…nnelupgradetry-functionality-for-04-channel
…ty-for-04-channel' of https://github.com/cosmos/ibc-go into cian/issue#1619-implement-channelupgradetry-functionality-for-04-channel
|
closing in favour of #3478 |
damiannolan
deleted the
cian/issue#1619-implement-channelupgradetry-functionality-for-04-channel
branch
Description
closes: #1619
This PR adds the ChanUpgradeTry handshake.
A few smaller items will be addressed in follow up issues.
validateProposedUpgradeChannelFieldsfunctionk.getConnectionForVerificationfunctionif !connectionEnd.IsOpen()Commit Message / Changelog Entry
N/A - part of feature branch
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.