Small consistency updates for channel upgrade spec #1036
Conversation
chatton
requested review from
mpoke,
AdityaSripal,
cwgoes,
colin-axner and
angbrav
as code owners
| // counterparty has already successfully upgraded so we cannot timeout | ||
| abortTransactionUnless(false) | ||
| } | ||
| } | ||
| abortTransactionUnless(counterpartyChannel.upgradeSequence >== channel.upgradeSequence) | ||
| abortTransactionUnless(counterpartyChannel.upgradeSequence >= channel.upgradeSequence) |
There was a problem hiding this comment.
Another consideration for the comment at line 999 :
"Both parties must not complete the upgrade handshake if the counterparty upgrade timeout has already passed. Even if both sides could have successfully moved to FLUSHCOMPLETE. This will prevent the channel ends from reaching incompatible states."
Note that the timeoutChannelUpgrade cannot be called if we reach the state where both chain are in FLUSHING_COMPLETE because abortTransactionUnless(counterpartyChannel.state !== FLUSHCOMPLETE) will bring to failure. That means that in this state we cannot send a ChanUpgradeTimeout datagram.
Thus to ensure we can still timeout in this state, we should add a check in ChanUpgradeOpen checking if CounterPartyTimeoutHasExpired. If yes we could write an error, error that the counterparty chain can use to restore its channelEnd.
There was a problem hiding this comment.
Please correct me if I'm wrong: we check the counterpartyUpgradeTimeout in timeoutPacket and acknowledgePacket and if the timeout has passed, then we execute restoreChannel, which should write an error receipt that can be used to cancel the upgrade on the counterparty. So in this case we wouldn't need to call timeoutChannelUpgrade, but we would call cancelChannelUpgrade instead?
And one question that was wondering after reading your comment: would it be possible for both channel ends to move to FLUSH_COMPLETE and then detect that the counterpartyUpgradeTimeout has passed? Asking because if the channel end moves to FLUSH_COMPLETE then we don't check the counterpartyUpgradeTimeout anymore in timeoutPacket and acknowledgePacket, right?
There was a problem hiding this comment.
In my understanding:
- We only send a
timeoutChannelUpgradedatagram if the timeout is detected in our end. Otherwise we write the error and then send acancelChannelUpgrade. Thus, your statement is correct. - Once we reach the (
FLUSH_COMPLETE,FLUSH_COMPLETE) the upgrade is mostly finalised and we only need to move toOPEN. At this point we don't care anymore about timeouts because all packets have been flushed.
I also open this PR to clarify the spec comment in bold mentioned in the previous message. As per this fix, we don't need to to ensure we can still timeout in this state. Thus no modification to ChanUpgradeOpen is required.
There was a problem hiding this comment.
Thank you, @chatton. I think I wouldn't change the ==/!= to ===/!== for the regular if statements. In the rest of the specs the usage of ===/!== is (mostly) limited to the boolean statements inside assert or abortTransactionUnless. In most of the if statements ==/!=` are used, which is also closer to typescript, since that's the programming language we kinda use for the pseudocode.
While re-reviewing the channel upgrades spec to regain context, I noticed a few small inconsistencies in code style in the pseudo code and a few suggestions for different wording in several cases.