Refactor `PendingSplice` #3911

jkczyz · 2025-07-03T16:04:36Z

FundedChannel::pending_funding and FundedChannel::pending_splice were developed independently, but the former will only contain values when the latter is set. This PR moves the former into PendingSplice and renames it to negotiated_candidates. It also removes unnecessary checks for FundedChannel::pending_splice and renames PendingSplice to PendingFunding. This allows for using PendingFunding for V2 channel establishment in order to support RBF.

ldk-reviews-bot · 2025-07-03T16:04:40Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

dunxen · 2025-07-04T07:13:31Z

lightning/src/ln/channel.rs

@@ -12616,7 +12616,6 @@ where
 			(49, self.context.local_initiated_shutdown, option), // Added in 0.0.122
 			(51, is_manual_broadcast, option), // Added in 0.0.124
 			(53, funding_tx_broadcast_safe_event_emitted, option), // Added in 0.0.124
-			(54, self.pending_funding, optional_vec), // Added in 0.2


We still have the opportunity to reuse 54 for negotiated_candidates when we persist that, right?

I think the more important question is whether PendingSplice (or PendingFunding) should be persisted, because currently it isn't.

We'll persist PendingFunding entirely. Wasn't sure about doing it in this PR or waiting for the additions in #3736.

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-07-05T16:05:24Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-07-07T16:06:11Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2025-07-07T20:38:14Z

Rebased.

wpaulino

LGTM, CI is failing though

lightning/src/ln/channel.rs

wpaulino

Good to squash

lightning/src/ln/channel.rs

jkczyz · 2025-07-08T18:00:05Z

Squashed. Hopefully CI will be happy.

TheBlueMatt · 2025-08-06T23:33:02Z

Looks like this stalled long enough that it needs rebase :/

jkczyz · 2025-08-07T14:10:53Z

Looks like this stalled long enough that it needs rebase :/

Yeah, plan is to get it in after #3886 to avoid merge needing to rebase that for merge conflicts.

jkczyz · 2025-09-09T22:33:35Z

Attempted a rebase on main using Claude Code with mixed success. Looks like there's another conflict though. 😭

FundedChannel::pending_funding is to be moved to PendingSplice. As such, it will be persisted with PendingSplice once persistence is added for the latter.

FundedChannel::pending_funding and FundedChannel::pending_splice were developed independently, but the former will only contain values when the latter is set.

An upcoming commit will rename PendingSplice to PendingFunding. Thus, rename the similarly named field to something more meaningful. It includes FundingScopes that have been negotiated but have not reached enough confirmations by both parties to have exchanged splice_locked.

While PendingSplice is only used for splicing a FundedChannel, it will be useful when supporting RBF for V2 channel establishment.

Now that PendingFunding directly contains the negotiated candidates, some unnecessary checks can be removed.

codecov · 2025-09-10T17:40:02Z

Codecov Report

❌ Patch coverage is 61.06870% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.32%. Comparing base (867f084) to head (5350932).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	61.06%	47 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3911      +/-   ##
==========================================
- Coverage   88.39%   88.32%   -0.07%     
==========================================
  Files         177      177              
  Lines      131314   131495     +181     
  Branches   131314   131495     +181     
==========================================
+ Hits       116069   116146      +77     
- Misses      12596    12688      +92     
- Partials     2649     2661      +12

Flag	Coverage Δ
fuzzing	`21.63% <24.42%> (-0.38%)`	⬇️
tests	`88.16% <61.06%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wpaulino

Needs to add persistence to PendingFunding as discussed offline

lightning/src/ln/channel.rs

To use impl_writeable_tlv_based_enum_upgradable with unread_variants, currently tuple syntax can't be used enum variants. Update FundingNegotiation to use this syntax so that it can be used with that macro.

graphite-app · 2025-09-11T04:34:21Z

lightning/src/ln/channel.rs

+			let prev_funding_txid = self.funding.get_funding_txid();
+
+			if let Some(scid) = self.funding.short_channel_id {
+				self.context.historical_scids.push(scid);
+			}
+
+			core::mem::swap(&mut self.funding, funding);
+
+			// The swap above places the previous `FundingScope` into `pending_funding`.
+			pending_splice
+				.negotiated_candidates
+				.drain(..)
+				.filter(|funding| funding.get_funding_txid() != prev_funding_txid)
+				.map(|mut funding| {
+					funding
+						.funding_transaction
+						.take()
+						.map(|tx| FundingInfo::Tx { transaction: tx })
+						.unwrap_or_else(|| FundingInfo::OutPoint {
+							outpoint: funding
+								.get_funding_txo()
+								.expect("Negotiated splices must have a known funding outpoint"),
+						})
+				})
+				.collect::<Vec<_>>()


The code separates the funding swap logic from the cleanup operations that happen at lines 10742-10745. This creates a potential issue where if any code between these two sections throws an exception, the channel could be left in an inconsistent state.

The original macro ensured these operations happened atomically. Consider moving the cleanup operations (setting interactive_tx_signing_session, pending_splice, etc. to None) inside this block before returning the discarded funding information to maintain atomicity and prevent inconsistent state in error cases.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

There isn't any possibility of failure. Having it inside the same scope doesn't make any difference.

wpaulino · 2025-09-11T17:39:20Z

lightning/src/ln/channel.rs

@@ -2576,6 +2583,13 @@ enum FundingNegotiation {
 	},
 }

+impl_writeable_tlv_based_enum_upgradable!(FundingNegotiation,
+	(0, AwaitingSignatures) => {
+		(0, funding, required),


This can be odd, right?

Once a splice funding transaction has been constructed, the corresponding state must be persisted so that the process can be continued across restarts. This includes exchanging signatures, waiting for enough confirmations, and RBF'ing.

jkczyz self-assigned this Jul 3, 2025

jkczyz requested a review from wpaulino July 3, 2025 16:05

jkczyz added this to Weekly Goals Jul 3, 2025

jkczyz moved this to Goal: Merge in Weekly Goals Jul 3, 2025

dunxen reviewed Jul 4, 2025

View reviewed changes

jkczyz force-pushed the 2025-07-pending-funding branch from d8da168 to 12cc782 Compare July 7, 2025 20:38

wpaulino reviewed Jul 7, 2025

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

jkczyz force-pushed the 2025-07-pending-funding branch 2 times, most recently from 75850ff to db9149c Compare July 8, 2025 17:44

wpaulino reviewed Jul 8, 2025

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

jkczyz force-pushed the 2025-07-pending-funding branch from db9149c to 6dabb81 Compare July 8, 2025 17:59

wpaulino mentioned this pull request Jul 8, 2025

[Splicing] Tx negotiation during splicing #3736

Merged

wpaulino previously approved these changes Jul 8, 2025

View reviewed changes

jkczyz mentioned this pull request Jul 10, 2025

Dual-funded channels and Splicing Project Tracking #1621

Open

7 tasks

jkczyz dismissed wpaulino’s stale review via d5a7051 September 9, 2025 22:31

jkczyz force-pushed the 2025-07-pending-funding branch from 6dabb81 to d5a7051 Compare September 9, 2025 22:31

jkczyz added 5 commits September 10, 2025 10:53

Remove FundedChannel::pending_funding persistence

78347f0

FundedChannel::pending_funding is to be moved to PendingSplice. As such, it will be persisted with PendingSplice once persistence is added for the latter.

Move FundedChannel::pending_funding into PendingSplice

6349685

FundedChannel::pending_funding and FundedChannel::pending_splice were developed independently, but the former will only contain values when the latter is set.

Rename PendingSplice to PendingFunding

4268120

While PendingSplice is only used for splicing a FundedChannel, it will be useful when supporting RBF for V2 channel establishment.

Remove unnecessary FundedChannel::pending_splice checks

d0780f7

Now that PendingFunding directly contains the negotiated candidates, some unnecessary checks can be removed.

jkczyz force-pushed the 2025-07-pending-funding branch from d5a7051 to d0780f7 Compare September 10, 2025 16:36

wpaulino reviewed Sep 10, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

jkczyz added 2 commits September 10, 2025 14:34

Inline promote_splice_funding macro

4ba6452

Use struct syntax for FundingNegotiation variants

5fecde5

To use impl_writeable_tlv_based_enum_upgradable with unread_variants, currently tuple syntax can't be used enum variants. Update FundingNegotiation to use this syntax so that it can be used with that macro.

graphite-app bot reviewed Sep 11, 2025

View reviewed changes

jkczyz requested a review from wpaulino September 11, 2025 13:52

wpaulino reviewed Sep 11, 2025

View reviewed changes

wpaulino requested a review from TheBlueMatt September 11, 2025 17:41

jkczyz added 2 commits September 11, 2025 20:19

Persist FundedChannel::pending_splice

4f0abe9

Once a splice funding transaction has been constructed, the corresponding state must be persisted so that the process can be continued across restarts. This includes exchanging signatures, waiting for enough confirmations, and RBF'ing.

Clean up FundedChannel::pending_splice docs

5350932

jkczyz force-pushed the 2025-07-pending-funding branch from 1d7f24f to 5350932 Compare September 12, 2025 01:20

Refactor PendingSplice #3911

Are you sure you want to change the base?

Refactor PendingSplice #3911

Uh oh!

Conversation

jkczyz commented Jul 3, 2025

Uh oh!

ldk-reviews-bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunxen Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

optout21 Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

jkczyz Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 5, 2025

Uh oh!

ldk-reviews-bot commented Jul 7, 2025

Uh oh!

jkczyz commented Jul 7, 2025

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz commented Jul 8, 2025

Uh oh!

TheBlueMatt commented Aug 6, 2025

Uh oh!

jkczyz commented Aug 7, 2025

Uh oh!

jkczyz commented Sep 9, 2025

Uh oh!

codecov bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

graphite-app bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

jkczyz Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

wpaulino Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

jkczyz Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Refactor `PendingSplice` #3911

Refactor `PendingSplice` #3911

ldk-reviews-bot commented Jul 3, 2025 •

edited

Loading

codecov bot commented Sep 10, 2025 •

edited

Loading