Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
option_simple_close
(features 60/61) #1205base: master
Are you sure you want to change the base?
option_simple_close
(features 60/61) #1205Changes from all commits
4f946a6
ee0c6f6
a5388ab
1a00338
0527294
30d36cb
959a5ae
6e0db70
47afb0e
8cfc619
a8fd1ab
1a3e5d8
aabde33
70c943f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add rationale here for why both of the new messages carry the closing scripts for both parties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that isn't immediately clear is: why does
closing_singed
need to replicate these fields? As a peer you MUST remember the last address you sent to make sure they don't trick you into sending all your funds away. Therefore, isn't it redundant to send the scripts again for the response message?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a rationale in 70c943f
This is generally very helpful for debugging (I'm thinking about the debug session we had when doing lnd <-> eclair cross-compat and has a signature mismatch): when you receive
closing_complete
orclosing_sig
, the message contains everything that's needed to build exactly the same transaction as the signer. It never hurts to include those and be explicit rather than implicit.It also makes it easier for nodes to accept a
closing_complete
message that uses their previous script, which is one way that can resolve the race condition described in the rationale. It's optional, and that's not what I implemented in eclair (I chose to simply disconnect when that race condition happens), but other implementations may choose to keep signing transactions that use their previous script(s) to avoid a disconnection and be free from race conditions.I hope that makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A diagram can help make that clearer: if Alice is ok with still using her old script (because a previous closing tx may confirm that uses it anyway), the follow flow may happen:
If the scripts were not included in
closing_sig
, Bob wouldn't know if Alice is using her new or old script.