BOLT#02: clarify coop close requirements by Crypt-iQ · Pull Request #970 · lightning/bolts

Crypt-iQ · 2022-03-16T18:09:03Z

Closes #964

TheBlueMatt · 2022-03-17T05:44:12Z

02-peer-protocol.md

    - MUST NOT send a `shutdown`.
  - MUST NOT send an `update_add_htlc` after a `shutdown`.
-  - if no HTLCs remain in either commitment transaction:
+  - if neither side has a dangling commitment and neither commitment contains any HTLCs (including dust HTLCs):


I don't think "dangling" is super well-defined here, and I'm not convinced we want to make that change anyway.

Allowing closing_signed while a dangling commitment exists can only happen in this transition:

---fail--> (last htlc getting cancelled) ---sig---> <--rev---- <--sig----

The previous wording said "either" which means it wasn't accounting for the possibility of the 3rd. Also the wording in the receiver section of commit_sig says MUST reply with revoke_and_ack

Hmm, I'd argue there is still "an HTLC in (one of the) counterparty commitment transactions" at that point - it just so happens there are two counterparty commitment transactions until you get the RAA. Still, in either case, this should use more concrete language IMO.

TheBlueMatt · 2022-03-17T05:45:26Z

02-peer-protocol.md


 As shutdown implies a desire to terminate, it implies that no new
-HTLCs will be added or accepted.  Once any HTLCs are cleared, the peer
+HTLCs will be added or accepted.  Once any HTLCs are cleared, there are no dangling commitments and all updates are included on both commitment transactions, the peer


Instead of using the word "dangling" can we specify what we mean clearly - something about commitments for which no revoke_and_ack has been sent?

Also, nit: please wrap lines at 80 chars, or so.

vincenzopalazzo · 2022-03-28T09:36:07Z

02-peer-protocol.md

 ### Closing Negotiation: `closing_signed`

-Once shutdown is complete and the channel is empty of HTLCs, the final
+Once shutdown is complete, the channel is empty of HTLCs, there are no dangling commitments and all updates are included on both commitments, the final


Suggested change

Once shutdown is complete, the channel is empty of HTLCs, there are no dangling commitments and all updates are included on both commitments, the final

Once shutdown is complete, the channel is empty of HTLCs, there are no dangling

commitments and all updates are included on both commitments, the final

The same comment of Matt here, be more specific it is good here because we avoid inconsistency for future updates or ambiguity.

vincenzopalazzo · 2022-03-28T09:36:53Z

02-peer-protocol.md

 may immediately begin closing negotiation, so we ban further updates
 to the commitment transaction (in particular, `update_fee` would be
-possible otherwise).
+possible otherwise). However, while there are HTLCs on the commitment transaction, the initiator may find it desirable to increase the feerate as there may be pending HTLCs on the commitment which could timeout.


Suggested change

possible otherwise). However, while there are HTLCs on the commitment transaction, the initiator may find it desirable to increase the feerate as there may be pending HTLCs on the commitment which could timeout.

possible otherwise). However, while there are HTLCs on the commitment

transaction, the initiator may find it desirable to increase

the feerate as there may be pending HTLCs on the commitment which could timeout.

Crypt-iQ · 2022-03-28T17:07:43Z

The current wording of this PR allows:

A                            B

			<---fail---
			<---sig----
			<---fee---- (allowed)

<---fail---
<---sig----
----rev---> (immediately sent upon sig)
----sig---> (lnd does this)
<---fee----

			----rev--->
			----sig---> (dont revoke)
			<---sig---- (signing for fee)

<---sig----
----rev--->
			
			----rev--->
			<---fee----
			<---sig----

<---fee----
<---sig----

	*repeat the fee transition*

So B can continually send update_fee since B never sent a revocation for the signature. I don't think this state transition should be allowed - there needs to be some point after which update_fee is disallowed. I know that both lnd and eclair will fail eventually with these state transitions if B does not send a revoke_and_ack after some time, but it doesn't seem to be a spec requirement.

TheBlueMatt · 2022-04-01T20:25:58Z

02-peer-protocol.md

    - MUST NOT send a `shutdown`
  - MAY send a `shutdown` before a `funding_locked`, i.e. before the funding transaction has reached `minimum_depth`.
-  - if there are updates pending on the receiving node's commitment transaction:
+  - if there are uncommitted updates pending on the receiving node's commitment transaction:


@vincenzopalazzo started looking at this in our code and I kinda realized....why do we have this requirement at all? Like, who really cares what's pending when a shutdown goes out? after shutdown goes out you shouldn't add anything new, and certainly sending a shutdown out while an update_add is pending is kinda dumb (the peer should reject at that point), but its not like it should break anything, only pre-closing_signed is a real concern here.

I assume a part of the reason for this is so that the receiving end can enforce the "no new update_adds" requirement when receiving commitment_signed and not later? Does anyone do this (maybe c-lightning @rustyrussell ?)

OK, this was good chatting, and I'm sorry if this came out of my head after the meeting, but I just drafted the lnprototest PR rustyrussell/lnprototest#37 after the meeting and I was able to make some consideration after writing the code.

So, my idea that I shared with @TheBlueMatt is why we care about what we don't receive, and we can be more precise in the fact of what we must not receive.

This will make the spec also less ambiguous, for instance, some things like that

Suggested change

- if there are uncommitted updates pending on the receiving node's commitment transaction:

- if `add_htlc` message is received from the received after shutdown, the sender must fail the htlc

P.S: this make also the lnprototest integration test easy with a single assert MustNodeReceive('add_htlc') :)

The only reason to have a "black list" is that during the time this black list can blow up?

Roasbeef · 2022-05-09T20:35:00Z

Spec comment: need to define what a "dangling" commitment is explicitly (and also if we want to use that term or something else). We can add a section here if we like the wording: https://github.com/lightning/bolts/blob/master/00-introduction.md#glossary-and-terminology-guide

Crypt-iQ · 2022-06-10T17:22:56Z

I've changed the wording so "no dangling commitments" is now roughly "no commitments for which a revocation is owed"

vincenzopalazzo

LGTM, just some format changes

vincenzopalazzo · 2022-06-15T10:15:10Z

02-peer-protocol.md

-may immediately begin closing negotiation, so we ban further updates
-to the commitment transaction (in particular, `update_fee` would be
-possible otherwise).
+HTLCs will be added or accepted.  Once any HTLCs are cleared, there are no commitments


Suggested change

HTLCs will be added or accepted. Once any HTLCs are cleared, there are no commitments

HTLCs will be added or accepted. Once any HTLCs are cleared, there are no commitments

vincenzopalazzo · 2022-06-15T10:18:12Z

02-peer-protocol.md

+  - if neither side has a commitment for which a revocation is owed and neither
+  commitment contains any HTLCs (including dust HTLCs):


Suggested change

- if neither side has a commitment for which a revocation is owed and neither

commitment contains any HTLCs (including dust HTLCs):

- if neither side has a commitment for which a revocation is owed and neither

commitment contains any HTLCs (including dust HTLCs):

t-bast

Looks mostly good to me, this is more clear now and I think this is the behavior we want.

02-peer-protocol.md

This commit ensures closing_signed can only begin if there are no dangling commitments. It also clarifies update_fee requirements if it is sent after shutdown.

t-bast

ACK baae0e9

TheBlueMatt · 2022-07-02T18:07:43Z

This closes #964, correct?

Crypt-iQ force-pushed the coop_close branch from de4ddbe to 240cdec Compare March 16, 2022 18:13

TheBlueMatt reviewed Mar 17, 2022

View reviewed changes

vincenzopalazzo mentioned this pull request Mar 18, 2022

coop-close channel integration testing rustyrussell/lnprototest#37

Closed

vincenzopalazzo suggested changes Mar 28, 2022

View reviewed changes

TheBlueMatt reviewed Apr 1, 2022

View reviewed changes

vincenzopalazzo mentioned this pull request Apr 6, 2022

BOLT 2: forget the check about update_* messages, and check what must not happens during shutdown #972

Closed

vincenzopalazzo mentioned this pull request Apr 29, 2022

bolt2: shutdown adventure #985

Closed

t-bast mentioned this pull request May 18, 2022

Lightning Specification Meeting 2022/05/23 #992

Closed

23 tasks

Crypt-iQ mentioned this pull request May 31, 2022

CLN interoperability: cooperative close attempt resulted in unilateral close lightningnetwork/lnd#6599

Closed

Crypt-iQ force-pushed the coop_close branch from 240cdec to 5a0d554 Compare June 10, 2022 17:20

Crypt-iQ mentioned this pull request Jun 10, 2022

extension-bolt: simple taproot channels (feature 80/81) #995

Open

vincenzopalazzo suggested changes Jun 15, 2022

View reviewed changes

t-bast mentioned this pull request Jun 16, 2022

Lightning Specification Meeting 2022/06/20 #1002

Closed

24 tasks

t-bast reviewed Jun 20, 2022

View reviewed changes

02-peer-protocol.md Outdated Show resolved Hide resolved

BOLT#02: clarify coop close requirements

baae0e9

This commit ensures closing_signed can only begin if there are no dangling commitments. It also clarifies update_fee requirements if it is sent after shutdown.

Crypt-iQ force-pushed the coop_close branch from 5a0d554 to baae0e9 Compare June 28, 2022 14:58

t-bast approved these changes Jun 28, 2022

View reviewed changes

t-bast mentioned this pull request Jun 29, 2022

Lightning Specification Meeting 2022/07/04 #1007

Closed

24 tasks

TheBlueMatt mentioned this pull request Jul 2, 2022

Allow update_fee sending during shutdown negotiation lightningdevkit/rust-lightning#1590

Open

TheBlueMatt approved these changes Jul 2, 2022

View reviewed changes

Crypt-iQ mentioned this pull request Jul 7, 2022

Splice draft (feature 62/63) #863

Closed

vincenzopalazzo approved these changes Jul 18, 2022

View reviewed changes

niftynei merged commit 341ec84 into lightning:master Jul 18, 2022

Crypt-iQ deleted the coop_close branch July 18, 2022 20:54

Crypt-iQ mentioned this pull request Jul 22, 2022

multi: implement spec-compliant coop close lightningnetwork/lnd#6760

Closed

vincenzopalazzo mentioned this pull request Sep 19, 2022

bolt2: add coop close test rustyrussell/lnprototest#62

Merged

	Once shutdown is complete, the channel is empty of HTLCs, there are no dangling commitments and all updates are included on both commitments, the final
	Once shutdown is complete, the channel is empty of HTLCs, there are no dangling
	commitments and all updates are included on both commitments, the final

	- if there are uncommitted updates pending on the receiving node's commitment transaction:
	- if `add_htlc` message is received from the received after shutdown, the sender must fail the htlc

	HTLCs will be added or accepted. Once any HTLCs are cleared, there are no commitments
	HTLCs will be added or accepted. Once any HTLCs are cleared, there are no commitments

		- if neither side has a commitment for which a revocation is owed and neither
		commitment contains any HTLCs (including dust HTLCs):

Conversation

Crypt-iQ commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ commented Mar 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Crypt-iQ commented Jun 10, 2022

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Crypt-iQ commented Mar 16, 2022 •

edited

Loading

Roasbeef commented May 9, 2022 •

edited

Loading