Clarify channel_reestablish requirements
#1049
Conversation
There are conflicting requirements after applying lightning#942. The only case where a node should fail the channel when receiving an unexpected `channel_reestablish` is when the remote peer is provably lying by sending an invalid `your_last_per_commitment_secret`.
| @@ -1445,10 +1445,10 @@ A node: | |||
| - if `next_commitment_number` is not 1 greater than the | |||
There was a problem hiding this comment.
I started with the smallest changes possible, but I find it weird that we have those requirements in the A node section and not in the A receiving node section, they only make sense once you've received the remote channel_reestablish, don't they? Should I move those in the receiving node section below?
There was a problem hiding this comment.
Yeah this also tripped me up as well when re-reading this section recently (had been a looong time since I had to dive into this section). I recall that during the past spec meeting in Oakland, @rustyrussell had a sort of mini session near the end explaining how to properly write out these types of requirements to minimize ambiguity....don't think any one took notes though
| - SHOULD send an `error` and fail the channel. | ||
| - if `your_last_per_commitment_secret` does not match the expected values: | ||
| - SHOULD send an `error` and fail the channel (the sending node is lying). | ||
| - otherwise: |
There was a problem hiding this comment.
I think this extra clause is out of place? Or is it meant to be a final else clause which was created above with "if option_static_remotekey"?
There was a problem hiding this comment.
It is currently under the "otherwise, if it supports option_data_loss_protect", which now contains three separate cases:
- your peer is honest and proves to you that you're late: send an
errorand wait for them to force-close to recover your funds - your peer tried to tell you you were late, but they lied: force-close on them, they're fishy
- otherwise, just send an error
But this is wrong, because that otherwise also includes the case where both nodes are correctly up-to-date, so we shouldn't tell peers to send an error in that case...
More generally, I'm thinking that I should instead more heavily rework the requirements to be almost pseudo-code, based on what eclair currently does. This way it should be obvious to transform it to code in any language, and we'd be sure we have exactly the same behavior. I'll try that approach in a separate PR (it won't be trivial to review!).
#942 clarified some of the requirements, but it created conflicts with other existing requirements that can be confusing.
IMHO the only case where a node should fail the channel when receiving an unexpected
channel_reestablishis when the remote peer is provably lying by sending an invalidyour_last_per_commitment_secret. In all other cases, one of the two peers may have lost data, but you cannot always be 100% sure, so the best you can do is notify your peer that one of you isn't up-to-date by sending anerrorand wait for them to either close the channel (if you're the one that isn't up-to-date) or send you anerror(if they're the one that isn't up-to-date).This should close #934 once and for all 🎉