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.

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

Removed requirement to broadcast an outdated commitment transaction #942

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

lightning-developer
Copy link
Contributor

Following the discussion at #932 and looking at the proposal of #834 I thought it might be good to clarify in BOLT 5 when a node MUST NOT (potentially SHOULD NOT) broadcast its own commitment transaction when failing the channel.

If a node has to fail a channel but knows that its latest commitment transaction is outdated it should not be required to send it but rather wait for the peer to unilaterally close the channel.

The proposed solution is not so clean because it might produce a deadlock in which two peers assume they have outdated state and send error back and forth without actually force closing. Maybe in such a scenario we could create a protocol that mutually closes with split balance?

Also replaced the word use with broadcast as it seems more accurate.

If a node has to fail a channel but knows that its latest commitment transaction is outdated it should not be required to send it but rather wait for the peer to unilaterally close the channel. 

The proposed solution is not so clean because it might produce a deadlock in which two peers assume they have outdated state and send `error` back and forth without actually force closing. Maybe in such a scenario we could create a protocol that mutually closes with split balance? 

Also replaced the word use with broadcast as it seems more accurate.
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK fdd791a

@t-bast t-bast added the clarification substantive change or addition around wording or meaning label Dec 15, 2021
05-onchain.md Outdated
- MUST use the *last commitment transaction*, for which it has a
- if the node knows or assumes its channel state is outdated it
- MUST NOT broadcast its *last commitment transaction*.
- SHOULD send an `error`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems related to #932. I think this is a weird place to add this requirement as BOLT 2 is where we define the p2p interaction, while BOLT 5 so far has been reserved primarily for on chain interaction. In other words, this seems to sort of "breach" layers.

This was discussed during the latest dev call, with some inclination to sort of merge this and #932, remove this line (as it's in #932) and keep the rest of the diff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lightning-developer I've prepared a commit on top of your branch that does this: t-bast@ae2d1e9

Can you cherry-pick it and update this PR? We can then include it and simply close #932.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as requested.

@rustyrussell
Copy link
Collaborator

This is already stated explicitly in reestablish, but probably worth mentioning here too, indeeed.

 A receiving node:
  - if `option_static_remotekey` or `option_anchors` applies to the commitment
    transaction:
    - if `next_revocation_number` is greater than expected above, AND
    `your_last_per_commitment_secret` is correct for that
    `next_revocation_number` minus 1:
      - MUST NOT broadcast its commitment transaction.
      - SHOULD fail the channel.

t-bast added a commit to t-bast/bolts that referenced this pull request Jan 4, 2022
It was decided in the spec meeting to combine lightning#932 and lightning#942 as they are
closely related. It was also decided to add a rationale for lightning#932 to clearly
state why nodes must wait for an error before force-closing instead of
eagerly force-closing when detecting that their peer is behind.
It was decided in the spec meeting to combine lightning#932 and lightning#942 as they are
closely related. It was also decided to add a rationale for lightning#932 to clearly
state why nodes must wait for an error before force-closing instead of
eagerly force-closing when detecting that their peer is behind.
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK b60e2b2

@t-bast t-bast requested a review from Roasbeef January 17, 2022 19:07
@t-bast t-bast merged commit 29db92f into lightning:master Jan 17, 2022
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ack b60e2b2

t-bast added a commit to t-bast/bolts that referenced this pull request Jan 10, 2023
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification substantive change or addition around wording or meaning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants