Skip to content

Add Splicing (and Quiescence) wire message definitions#2544

Merged
TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
optout21:splicing-msgs0
Nov 13, 2023
Merged

Add Splicing (and Quiescence) wire message definitions#2544
TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
optout21:splicing-msgs0

Conversation

@optout21
Copy link
Copy Markdown
Contributor

@optout21 optout21 commented Aug 31, 2023 •
edited
Loading

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
Copy Markdown

codecov-commenter commented Sep 1, 2023 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

optout21 commented Oct 27, 2023

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
Copy Markdown
Contributor

moneyball commented Oct 27, 2023

Out of curiosity what do Eclair/Phoenix do?

@optout21
Copy link
Copy Markdown
Contributor Author

optout21 commented Oct 27, 2023

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
dunxen
dunxen reviewed Nov 6, 2023
View reviewed changes
@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
dunxen
dunxen reviewed Nov 7, 2023
View reviewed changes
@optout21 optout21 changed the title Add Splicing wire message definitions Add Splicing (and Quiescence) wire message definitions Nov 7, 2023
@dunxen
Copy link
Copy Markdown
Contributor

dunxen commented Nov 7, 2023

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

@optout21
Copy link
Copy Markdown
Contributor Author

optout21 commented Nov 7, 2023

Squashed and rebased

dunxen
dunxen approved these changes Nov 9, 2023
View reviewed changes
Copy link
Copy Markdown
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.

TheBlueMatt
TheBlueMatt approved these changes Nov 13, 2023
View reviewed changes
Copy link
Copy Markdown
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
@optout21 optout21 mentioned this pull request Sep 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@dunxen dunxen dunxen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Splicing messages

5 participants

@optout21 @codecov-commenter @dunxen @moneyball @TheBlueMatt