Skip to content
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.

Sign up for GitHub

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

Clarify channel_reestablish requirements #1049

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Clarify channel_reestablish requirements #1049

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions 02-peer-protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1445,10 +1445,10 @@ A node:
- if `next_commitment_number` is not 1 greater than the
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

commitment number of the last `commitment_signed` message the receiving
node has sent:
- SHOULD send an `error` and fail the channel.
- SHOULD send an `error`.
- if it has not sent `commitment_signed`, AND `next_commitment_number`
is not equal to 1:
- SHOULD send an `error` and fail the channel.
- SHOULD send an `error`.
- if `next_revocation_number` is equal to the commitment number of
the last `revoke_and_ack` the receiving node sent, AND the receiving node
hasn't already received a `closing_signed`:
Expand All @@ -1460,10 +1460,10 @@ A node:
- otherwise:
- if `next_revocation_number` is not equal to 1 greater than the
commitment number of the last `revoke_and_ack` the receiving node has sent:
- SHOULD send an `error` and fail the channel.
- SHOULD send an `error`.
- if it has not sent `revoke_and_ack`, AND `next_revocation_number`
is not equal to 0:
- SHOULD send an `error` and fail the channel.
- SHOULD send an `error`.

A receiving node:
- if `option_static_remotekey` applies to the commitment transaction:
Expand All @@ -1472,9 +1472,10 @@ A node:
`next_revocation_number` minus 1:
- MUST NOT broadcast its commitment transaction.
- SHOULD send an `error` to request the peer to 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:
- if `your_last_per_commitment_secret` does not match the expected values:
- SHOULD send an `error` and fail the channel.
- SHOULD send an `error`.
- otherwise, if it supports `option_data_loss_protect`:
- if `next_revocation_number` is greater than expected above, AND
`your_last_per_commitment_secret` is correct for that
Expand All @@ -1483,9 +1484,10 @@ A node:
- SHOULD send an `error` to request the peer to fail the channel.
- SHOULD store `my_current_per_commitment_point` to retrieve funds
should the sending node broadcast its commitment transaction on-chain.
- otherwise (`your_last_per_commitment_secret` or `my_current_per_commitment_point`
do not match the expected values):
- 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 error and 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!).

- SHOULD send an `error`.

A node:
- MUST NOT assume that previously-transmitted messages were lost,
Expand Down