Skip to content

Implement option_simple_close #2967

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

Jump to bottom
Merged
t-bast merged 4 commits into from
Feb 13, 2025
Merged

Implement option_simple_close #2967

t-bast merged 4 commits into from
Feb 13, 2025

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Dec 17, 2024 •
edited
Loading

Implement option_simple_close as defined in the official spec feature in lightning/bolts#1205

Hopefully this is the last time we change the mutual close protocol! And at some point that will let us entirely remove all the code supporting the two previous mutual close protocols (this is why I kept the code as separate as possible instead of trying to fit into the existing NEGOTIATING state).

Note that this is a prerequisite for taproot channels: this protocol allows nodes to safely exchange nonces in shutdown, closing_complete and closing_sig to spend a musig2 channel output.

I have verified that we have cross-compat with lnd so this is ready to integrate!

@t-bast t-bast mentioned this pull request Dec 17, 2024
Merged
@t-bast t-bast mentioned this pull request Jan 15, 2025
Closed
@t-bast t-bast changed the title Implement option_simple_close (simpler variant) Implement option_simple_close Jan 17, 2025
@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from 67aa29f to 420298c Compare January 21, 2025 09:52
@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2025 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.89831% with 38 lines in your changes missing coverage. Please review.

Project coverage is 85.94%. Comparing base (10edb42) to head (73bff0e).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 76.66% 21 Missing ⚠️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 88.46% 6 Missing ⚠️
...la/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala 40.00% 6 Missing ⚠️
...la/fr/acinq/eclair/transactions/Transactions.scala 94.87% 2 Missing ⚠️
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala 0.00% 1 Missing ⚠️
...la/fr/acinq/eclair/io/OpenChannelInterceptor.scala 0.00% 1 Missing ⚠️
...n/scala/fr/acinq/eclair/io/PeerReadyNotifier.scala 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2967      +/-   ##
==========================================
- Coverage   86.02%   85.94%   -0.09%     
==========================================
  Files         228      228              
  Lines       20452    20719     +267     
  Branches      846      860      +14     
==========================================
+ Hits        17594    17807     +213     
- Misses       2858     2912      +54
Files with missing lines Coverage Δ
...core/src/main/scala/fr/acinq/eclair/Features.scala 100.00% <100.00%> (ø)
...in/scala/fr/acinq/eclair/channel/ChannelData.scala 100.00% <100.00%> (ø)
...in/scala/fr/acinq/eclair/channel/Commitments.scala 96.30% <100.00%> (+0.01%) ⬆️
...a/fr/acinq/eclair/channel/fsm/CommonHandlers.scala 88.88% <100.00%> (+3.59%) ⬆️
...inq/eclair/channel/fund/InteractiveTxBuilder.scala 93.03% <100.00%> (ø)
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala 96.00% <ø> (ø)
...ire/internal/channel/version4/ChannelCodecs4.scala 98.55% <100.00%> (+0.02%) ⬆️
...ala/fr/acinq/eclair/wire/protocol/ChannelTlv.scala 100.00% <100.00%> (ø)
.../eclair/wire/protocol/LightningMessageCodecs.scala 99.49% <100.00%> (+0.01%) ⬆️
...q/eclair/wire/protocol/LightningMessageTypes.scala 99.18% <100.00%> (+0.04%) ⬆️
... and 7 more

... and 4 files with indirect coverage changes

@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from 420298c to d57c3a7 Compare January 21, 2025 16:28
@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from d57c3a7 to d5c6021 Compare January 30, 2025 15:44
@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from d5c6021 to f738b99 Compare February 7, 2025 09:20
@t-bast t-bast marked this pull request as ready for review February 7, 2025 10:21
@t-bast t-bast requested review from pm47 and sstone February 7, 2025 10:22
sstone
sstone reviewed Feb 10, 2025
View reviewed changes
sstone
sstone reviewed Feb 11, 2025
View reviewed changes
Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

LGTM! just a few nits

@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from bade5a6 to 73bff0e Compare February 11, 2025 15:47
@t-bast
Copy link
Member Author

t-bast commented Feb 11, 2025

Rebased to fix the trivial conflict in CheckBalance.scala.

sstone
sstone previously approved these changes Feb 11, 2025
View reviewed changes
@t-bast
Add option_simple_close feature bit
9e22637
@t-bast
Add codecs for option_simple_close
7559535 
This feature adds two new messages:

- `closing_complete` sent after exchanging `shutdown`
- `closing_sig` sent in response to `closing_complete`
@t-bast
Add support for OP_RETURN closing scripts
55e9375 
The spec allows the closer to use an OP_RETURN output if their amount is
too low when using `option_simple_close`.
@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from 73bff0e to a01cde4 Compare February 13, 2025 08:04
@t-bast
Copy link
Member Author

t-bast commented Feb 13, 2025

Rebased to fix trivial conflict in Features.scala.

sstone
sstone previously approved these changes Feb 13, 2025
View reviewed changes
pm47
pm47 previously approved these changes Feb 13, 2025
View reviewed changes
@t-bast
Implement the option_simple_close protocol
f7b4212 
We introduce a new `NEGOTIATING_SIMPLE` state where we exchange the
`closing_complete` and `closing_sig` messages, and allow RBF-ing previous
transactions and updating our closing script.

We stay in that state until one of the transactions confirms, or a force
close is detected. This is important to ensure we're able to correctly
reconnect and negotiate RBF candidates.

We keep this separate from the previous `NEGOTIATING` state to make it
easier to remove support for the older mutual close protocols once we're
confident the network has been upgraded.
@t-bast t-bast dismissed stale reviews from pm47 and sstone via f7b4212 February 13, 2025 12:16
@t-bast t-bast force-pushed the option-simple-close-no-state-machine branch from a01cde4 to f7b4212 Compare February 13, 2025 12:16
@t-bast
Copy link
Member Author

t-bast commented Feb 13, 2025

Rebased to squash fixup commits.

@t-bast t-bast requested a review from sstone February 13, 2025 13:15
@t-bast t-bast merged commit 3aac8da into master Feb 13, 2025
1 check passed
@t-bast t-bast deleted the option-simple-close-no-state-machine branch February 13, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pm47 pm47 pm47 approved these changes

@sstone sstone Awaiting requested review from sstone

Assignees

No one assigned

Labels

Optech Make Me Famous!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@t-bast @codecov-commenter @sstone @pm47