ChanUpgradeTimeout handler for 04-channel
#3829
Conversation
charleenfei
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
…/ibc-go into charly/#3747-upgrade-timeout-hand
| @@ -45,4 +45,8 @@ var ( | |||
| ErrInvalidUpgradeSequence = errorsmod.Register(SubModuleName, 29, "invalid upgrade sequence") | |||
| ErrUpgradeNotFound = errorsmod.Register(SubModuleName, 30, "upgrade not found") | |||
| ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") | |||
| ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") | |||
| ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 33, "restore failed") | |||
| ErrUpgradeTimeout = errorsmod.Register(SubModuleName, 34, "upgrade timed-out") | |||
There was a problem hiding this comment.
i find these two upgrade timeout errors a bit confusingly similar, but i wasnt really sure exactly how to modify it to make it better.
| @@ -45,4 +45,8 @@ var ( | |||
| ErrInvalidUpgradeSequence = errorsmod.Register(SubModuleName, 29, "invalid upgrade sequence") | |||
| ErrUpgradeNotFound = errorsmod.Register(SubModuleName, 30, "upgrade not found") | |||
| ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") | |||
| ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") | |||
| ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 33, "restore failed") | |||
There was a problem hiding this comment.
this error is coming from the WriteUpgradeTimeoutPR
charleenfei
changed the title
ChanUpgradeTimeout handler for 04-channel
| // was for a previous sequence by the timeout deadline. | ||
| upgradeSequence := channel.UpgradeSequence | ||
| if upgradeSequence < prevErrorReceipt.Sequence { | ||
| return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "previous counterparty error receipt sequence is greater than our current upgrade sequence: %d > %d", prevErrorReceipt.Sequence, upgradeSequence) |
There was a problem hiding this comment.
idea: maybe we could also just call ChanUpgradeCancel here in this case, instead of forcing the user to call.
| if (timeout.Height.IsZero() || proofHeight.LT(timeout.Height)) && | ||
| (timeout.Timestamp == 0 || proofTimestamp < timeout.Timestamp) { |
There was a problem hiding this comment.
maybe we can split this up into some named booleans to aid readability.
proofHeightIsInvalid := timeout.Height.IsZero() || proofHeight.LT(timeout.Height)
proofTimestampIsInvalid := timeout.Timestamp == 0 || proofTimestamp < timeout.Timestamp
if proofHeightIsInvalid && proofTimestampIsInvalid {
return errorsmod.Wrap(types.ErrInvalidUpgradeTimeout, "timeout has not yet passed on counterparty chain")
} 
There was a problem hiding this comment.
I also stared at this for a bit trying to grok it. Naming is definitely beneficial here.
| } | ||
|
|
||
| // Error receipt passed in is either nil or it is a stale error receipt from a previous upgrade | ||
| if (prevErrorReceipt == types.ErrorReceipt{}) { |
There was a problem hiding this comment.
do we want to change the prevErrorReceipt to be of type *types.ErrorReceipt. This would be more in line with the spec. There is no functional difference anyway.
| if err != nil { | ||
| return errorsmod.Wrap(err, "failed to verify absence of counterparty channel upgrade error receipt") | ||
| } | ||
| } else { |
There was a problem hiding this comment.
[nit] to reduce indentation we can remove the else, and just add a return nil the line above this.
| "timeout has not passed", | ||
| func() { | ||
| upgrade := path.EndpointA.GetProposedUpgrade() | ||
| upgrade.Timeout.Height = clienttypes.NewHeight(1, uint64(path.EndpointA.Chain.GetContext().BlockHeight()+100)) |
There was a problem hiding this comment.
| upgrade.Timeout.Height = clienttypes.NewHeight(1, uint64(path.EndpointA.Chain.GetContext().BlockHeight()+100)) | |
| upgrade.Timeout.Height = suite.chainA.GetTimeoutHeight() |
| // timeout for this sequence can only succeed if the error receipt written into the error path on the counterparty | ||
| // was for a previous sequence by the timeout deadline. | ||
| upgradeSequence := channel.UpgradeSequence | ||
| if upgradeSequence < prevErrorReceipt.Sequence { |
There was a problem hiding this comment.
I think this needs to be <=. We should not timeout an upgrade where our sequence matches the counterparty upgrade sequence and there exists an error receipt for that upgrade attempt. That should indicate a cancellation
note, I think using <= would match the spec as the spec asserts:
abortTransactionUnless(sequence > prevErrorReceipt.sequence)
Thus if the sequence == error receipt sequence, the spec fails but this implementation does not
There was a problem hiding this comment.
i can make a followup PR for this issue! but i think also, now looking at the two functions, im really wondering if there is a way to combine these two such that there is first a check on proofHeight to see if it has passed the Timeout. if so, then we follow the Timeout flow. Otherwise, we follow the Cancel flow. and then we just expose one API/msg_server function. wdyt?
Description
closes: #3747
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.