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

Add Splicing (and Quiescence) wire message definitions #2544

Merged
merged 1 commit into from Nov 13, 2023

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Aug 31, 2023

Add Splicing and Quiescence wire messages.

The messages are defined in the LN spec PR 863 (draft). Note that the spec PR is currently a draft, it needs a rebase, and does not reflect the latest state of discussions.

The following messages are added:

  • stfu (2)
  • splice (75* -- see notes below)
  • splice_ack (76)
  • splice_locked (77)

Fixes #2542 .

Note that current spec PR specifies type 74 for splice message, but that value is already used (by tx_abort), so value 75 is used as of now in this PR.

TODO:

  • msg definitions
  • msg encoding
  • send&handle infrastructure
  • Finalize type for splice msg
  • fuzzing

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Attention: 129 lines in your changes are missing coverage. Please review.

Comparison is base (96e7d7a) 88.85% compared to head (649129d) 88.68%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2544      +/-   ##
==========================================
- Coverage   88.85%   88.68%   -0.17%     
==========================================
  Files         113      113              
  Lines       89170    89344     +174     
  Branches    89170    89344     +174     
==========================================
+ Hits        79228    79238      +10     
- Misses       7699     7859     +160     
- Partials     2243     2247       +4     
Files Coverage Δ
lightning/src/events/mod.rs 33.11% <ø> (ø)
lightning-net-tokio/src/lib.rs 73.94% <0.00%> (-1.69%) ⬇️
lightning/src/ln/functional_test_utils.rs 90.71% <0.00%> (-0.35%) ⬇️
lightning/src/ln/wire.rs 55.81% <0.00%> (-2.02%) ⬇️
lightning/src/util/test_utils.rs 76.18% <0.00%> (-1.02%) ⬇️
lightning/src/ln/channelmanager.rs 78.46% <0.00%> (-0.29%) ⬇️
lightning/src/ln/msgs.rs 76.83% <64.28%> (-0.40%) ⬇️
lightning/src/ln/peer_handler.rs 57.28% <0.00%> (-1.52%) ⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dunxen
Copy link
Contributor

dunxen commented Sep 1, 2023

Thanks! Yeah, due to the drafty nature of the spec, there's no rush to merge this, but then again it can always be reworked as they're also not yet used in release.

Note that current spec PR specifies type 74 for splice message, but that value is already used (by tx_abort), so value 84 is used as of now in this PR.

Oh, really? Spec PR probably hasn't been updated in while. Thanks for tracking it. Then we'll definitely keep this draft until there's a clear type.

@optout21
Copy link
Contributor Author

optout21 commented Sep 4, 2023

Did some changes to be in sync with CLN, as opposed to the outdated version of the spec (but this will need confirmation):

  • type of splice msg is 75 (74 is taken by tx_abort; the 3 splicing msgs are thus 75, 76, 77)
  • funding parameter is now relative, signed value, relative_satoshis
  • reorder first two parameters channel_id and chain_hash

Note: definition in CLN https://github.com/ElementsProject/lightning/blob/master/wire/peer_wire.csv#L209

@optout21 optout21 force-pushed the splicing-msgs0 branch 2 times, most recently from e1e2870 to 1e9eece Compare September 4, 2023 09:01
@optout21
Copy link
Contributor Author

This is on hold, due to the spec not being updated. Above changes are in sync with the CLN implementation, but not the spec. Specifically, using 75 for splice message.
The spec PR has not been updated, but the changes are clearly mentioned as TODO in a comment by @ddustin.

Therefore I suggest to take this as a confirmation, and do not necessarily wait for spec update with this PR.

I will wrap up with adding fuzzing tests and propose for review.

@moneyball
Copy link
Contributor

Out of curiosity what do Eclair/Phoenix do?

@optout21
Copy link
Contributor Author

Out of curiosity what do Eclair/Phoenix do?

They are currently using nonstandard 3700x numerical types for splice messages (which means that their implementation is not compatible with others).
(https://github.com/ACINQ/eclair/blob/master/eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/LightningMessageCodecs.scala#L483)

@optout21 optout21 changed the title [WIP] Add Splicing wire message definitions Add Splicing wire message definitions Nov 5, 2023
@optout21 optout21 marked this pull request as ready for review November 5, 2023 06:15
lightning/src/ln/wire.rs Show resolved Hide resolved
lightning/src/ln/wire.rs Show resolved Hide resolved
lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
@optout21 optout21 marked this pull request as draft November 6, 2023 13:17
@optout21 optout21 requested a review from dunxen November 6, 2023 13:21
@optout21 optout21 marked this pull request as ready for review November 6, 2023 16:45
fuzz/src/bin/gen_target.sh Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@optout21 optout21 changed the title Add Splicing wire message definitions Add Splicing (and Quiescence) wire message definitions Nov 7, 2023
@dunxen
Copy link
Contributor

dunxen commented Nov 7, 2023

Nice. I think we can squash the fix ups and formatting commits now :)

@optout21
Copy link
Contributor Author

optout21 commented Nov 7, 2023

Squashed and rebased

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

I'll trust @dunxen on the message encodings and contents.

@TheBlueMatt TheBlueMatt merged commit c852ce6 into lightningdevkit:main Nov 13, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Splicing messages
5 participants