-
Notifications
You must be signed in to change notification settings - Fork 586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Channel Upgrade Try functionality for 04 channel #3333
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this SetChannel
(maybe for clarity) be done at the end?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Ay, excellent work y'all!! Looking good 😎 🌞
I left a lot of thoughts/suggestions for improvements. I've only gotten part way through the upgradeTry handler. Will continue my review later
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, upgradeSequence
After 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be 1?
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.
shouldn't it actually still be 0 because we are in channel state OPEN
? we update the sequence further down the code block
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.
Looking good. Have a couple comments for discussion
} | ||
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 guys are rockin' it with this one. 🪨 🎸
Left a couple of extra comments. :)
channel.State = types.TRYUPGRADE | ||
channel.Version = proposedUpgradeVersion | ||
channel.Ordering = channelUpgrade.Ordering | ||
channel.ConnectionHops = channelUpgrade.ConnectionHops |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you happy if we introduce a helper function in a follow up?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Description
closes: #1619
This PR adds the ChanUpgradeTry handshake.
A few smaller items will be addressed in follow up issues.
validateProposedUpgradeChannelFields
functionk.getConnectionForVerification
functionif !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/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.