Skip to content

[Splicing] Partial, handle splice_init & splice_ack messages - #3407

Merged
jkczyz merged 1 commit into
lightningdevkit:mainfrom
optout21:splice-init
Mar 7, 2025
Merged

[Splicing] Partial, handle splice_init & splice_ack messages#3407
jkczyz merged 1 commit into
lightningdevkit:mainfrom
optout21:splice-init

Conversation

@optout21

@optout21optout21 commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

Handle splice_init & splice_ack messages, without performing splicing. splice_init does not error, but splice_ack returns an error (and neither side continues with splicing).
One next piece for splicing #3298 .
This can come before #3137 .

@optout21
optout21 requested a review from jkczyzNovember 13, 2024 14:52
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@codecov

codecovBot commented Nov 18, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.25%. Comparing base (0216d7d) to head (cd3c40b).

Additional details and impacted files
@@ Coverage Diff @@## main #3407 +/- ##
==========================================
+ Coverage 89.19% 89.25% +0.05% 
==========================================
Files 152 152 Lines 118830 118841 +11 Branches 118830 118841 +11 ==========================================
+ Hits 105993 106066 +73 + Misses 10252 10201 -51 + Partials 2585 2574 -11 

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

@optout21optout21 changed the title [Draft, Splicing] Partly handle splice_init & splice_ack messages[Splicing] Partly handle splice_init & splice_ack messagesNov 18, 2024
@optout21
optout21force-pushed the splice-init branch 2 times, most recently from 19f8ab8 to 4ee3735CompareNovember 18, 2024 11:02
@optout21
optout21 marked this pull request as ready for review November 18, 2024 11:03
@optout21
optout21 requested a review from jkczyzNovember 18, 2024 11:44
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
@optout21

Copy link
Copy Markdown
ContributorAuthor

I have reorganized code, moved most single-channel checks and handling logic into Channel struct from ChannelManager.

Comment threadlightning/src/ln/channel.rs Outdated
@optout21

Copy link
Copy Markdown
ContributorAuthor

Rebased (post #3137) and squashed

@optout21
optout21 requested a review from jkczyzNovember 21, 2024 10:34
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@optout21optout21 changed the title [Splicing] Partly handle splice_init & splice_ack messages[Splicing] Partlial, handle splice_init & splice_ack messagesDec 5, 2024
@optout21

optout21 commented Dec 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Rebased and squashed
TODO: Fix linting issue

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
@optout21

Copy link
Copy Markdown
ContributorAuthor

Review comments from @wpaulino addressed.

@optout21
optout21 requested a review from wpaulinoMarch 4, 2025 21:08

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly minor comments and nits, but note that one introduced test is broken.

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/functional_tests_splice.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs
@optout21
optout21force-pushed the splice-init branch 2 times, most recently from 9e9045c to b7c2d45CompareMarch 4, 2025 22:54
@optout21
optout21 requested a review from jkczyzMarch 4, 2025 22:58

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @wpaulino Good to squash the fixups?

@optout21

Copy link
Copy Markdown
ContributorAuthor

Squashed into a single commit, prepared for merging.

wpaulino
wpaulino previously approved these changes Mar 5, 2025
jkczyz
jkczyz previously approved these changes Mar 5, 2025

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still waiting on CI to start... :(

@optout21

optout21 commented Mar 6, 2025

Copy link
Copy Markdown
ContributorAuthor

Somehow CI got mixed up, showing failures for earlier commit... I'm not sure how to re-trigger.

Commit ID is different. I know the issue being reported (conflict caused by #3016) but fixed previously.

@optout21

Copy link
Copy Markdown
ContributorAuthor

Did a commit amend & force push, let's see CI's response...

@optout21

Copy link
Copy Markdown
ContributorAuthor

For some reason (which I don't understand), when in the CI this PR branch is merged onto current main, a bunch of previous rebase merges get reverted, and compilation fails. Even though the PR branch was rebased to recent and has no conflicts. I have recreated the commit completely anew, hopefully this helps.

@dunxen
dunxen self-requested a review March 6, 2025 15:17
@jkczyz

Copy link
Copy Markdown
Contributor

For some reason (which I don't understand), when in the CI this PR branch is merged onto current main, a bunch of previous rebase merges get reverted, and compilation fails. Even though the PR branch was rebased to recent and has no conflicts. I have recreated the commit completely anew, hopefully this helps.

Github shows that your branch is 33 commits behind. If I rebase your PR on upstream main, I can reproduce the CI failure with RUSTFLAGS="--cfg=splicing" cargo +1.63.0 check -p lightning. Seems you need to account for #3594.

Handle the initial splice negotiation, without continuing
to transaction negotiation:
- `splice_channel()` for initiating splicing
- handling of `splice_init` and `splice_ack` messages, with checks
- `splice_ack` always fails (use case still imcomplete)
- A test to go through the use case (`test_v1_splice_in`)
@jkczyz
jkczyz dismissed stale reviews from wpaulino and themself via dc3ab7bMarch 6, 2025 19:00
@jkczyz
jkczyz requested a review from wpaulinoMarch 6, 2025 19:02
@jkczyz

Copy link
Copy Markdown
Contributor

Github shows that your branch is 33 commits behind. If I rebase your PR on upstream main, I can reproduce the CI failure with RUSTFLAGS="--cfg=splicing" cargo +1.63.0 check -p lightning. Seems you need to account for #3594.

Rebased and fixed the compilation error locally. CI running now.

@jkczyz
jkczyz merged commit 4c43a5b into lightningdevkit:mainMar 7, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Splicingweekly goalSomeone wants to land this this week

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@optout21@jkczyz@wpaulino@TheBlueMatt