Integrate Splicing with Quiescence #4019

TheBlueMatt · 2025-08-18T13:58:38Z

~~Based on #4007~~ this does the actual integration. Will probably go after #3979 so drafting for now.

codecov · 2025-08-18T14:06:30Z

Codecov Report

❌ Patch coverage is 76.47059% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.71%. Comparing base (a18f362) to head (9200308).
⚠️ Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	77.35%	12 Missing ⚠️
lightning/src/util/ser.rs	0.00%	7 Missing ⚠️
lightning/src/ln/channelmanager.rs	81.25%	2 Missing and 1 partial ⚠️
lightning/src/ln/quiescence_tests.rs	92.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4019      +/-   ##
==========================================
- Coverage   88.76%   88.71%   -0.05%     
==========================================
  Files         176      176              
  Lines      128139   128891     +752     
  Branches   128139   128891     +752     
==========================================
+ Hits       113743   114348     +605     
- Misses      11822    11947     +125     
- Partials     2574     2596      +22

Flag	Coverage Δ
fuzzing	`21.92% <15.27%> (+<0.01%)`	⬆️
tests	`88.54% <76.47%> (-0.05%)`	⬇️

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 · 2025-08-20T17:36:50Z

lightning/src/ln/channel.rs

-			let tx16 = TransactionU16LenLimited::new(tx)
-				.map_err(|_e| APIError::APIMisuseError { err: format!("Too large transaction") })?;
+
+		// TODO(splicing): Check that transactions aren't too big for the splice_init message here.


We probably just want to do all input validation here before we even attempt quiescence so that we can fail early and fast. No point in going through the whole stfu dance just to immediately fail back to the user with "insufficient fees" or something similar.

Yep, that's definitely the goal. Kinda leaving this until the parameters are clear and I'll clean it up then.

ldk-reviews-bot · 2025-08-21T15:17:26Z

👋 Thanks for assigning @jkczyz 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.

When doing an outbound splice, we check some of the instructions first in utility methods, then convert errors to `APIError`s. These utility methods should thus either return an `APIError` or more generic (string or `()`) error type, but they currently return a `ChannelError`, which is only approprite when the calling code will do what the `ChannelError` instructs (including closing the channel). Here we fix that by returning `String`s instead.

lightning/src/ln/channel.rs

lightning/src/ln/quiescence_tests.rs

lightning/src/ln/channel.rs

jkczyz · 2025-08-27T17:19:20Z

lightning/src/ln/channel.rs

 	// Assumes we are either awaiting quiescence or our counterparty has requested quiescence.
 	#[rustfmt::skip]
-	pub fn send_stfu<L: Deref>(&mut self, logger: &L) -> Result<msgs::Stfu, ChannelError>
+	pub fn send_stfu<L: Deref>(&mut self, logger: &L) -> Result<msgs::Stfu, &'static str>


May as well return String if we'll just need to call to_owned.

I think we'd end up with strictly more to_owneds, and we'll already have the map at the callsites to convert to a ChannelError, plus rustfmt is much more likely to blow up an Err(long string.to_owned()) than a map(|e| e.to_string()). Seems simpler to just keep the &'static str?

lightning/src/ln/channel.rs

jkczyz · 2025-08-27T17:23:56Z

lightning/src/ln/channel.rs

+		// TODO(splicing): if post_quiescence_action is set, integrate what the user wants to do
+		// into the counterparty-initiated splice. For always-on nodes this probably isn't a useful
+		// optimization, but for often-offline nodes it may be, as we may connect and immediately
+		// go into splicing from both sides.


Would this happen when both counterparties initiated quiescence simultaneously? Ties should go to the channel opener. Would we lose the user-initiated quiescent_action in that case? Or rather would we possibly be stuck when the user tries to initiate the splice again since quiescent_action is Some?

If we lose the tie, we don't seem to consume the quiescent_action, but we also don't seem to attempt quiescence again once we exit so that we can perform ours. We only seem to retry quiescence with a pending quescient_action after a reconnection.

We wouldn't lose it immediately, we'd wait until the counterparty's splice completes, then try to initialize our own splice and then fail at that point (because another splice is now in progress).

What I mean here is that we don't retry quiescence after we've exited if we still have a quiescent_action lingering. It's generally applicable once we have other quiescence protocols, but also relevant for zero conf splices since it'll be immediately locked in and we can attempt another splice.

We should retry quiescence after we've exited, it'll just fail. We don't yet have any exiting-quiescence logic yet, however, so there's nowhere to put it yet...

Should we address the TODO now (i.e., contribute using splice_ack) instead of adding retry or SpliceFailed event? Only issue would be if there were another quiescence protocol that we understood. Then we'd again have a stuck action, IIUC.

We'll ultimately need a splice failed event for the general case even if, eg a peer simply nak's a splice, no? I'm definitely happy to address the TODO now, however, is doing it trivial? Its not quite clear to me it is.

I spent some time looking at addressing the TODO and I think it really needs to come with a SpliceInstructions refactor - we need to add a max-fee and min-fee field, with some default (ie push things through a constructor rather than a public enum) and then want to move the fee-calculation logic so that we can recalculate fees if we have to. Would rather wait on this.

ldk-reviews-bot · 2025-08-27T17:24:04Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

lightning/src/ln/channel.rs

wpaulino · 2025-08-27T19:27:50Z

lightning/src/ln/channel.rs

+		if self.funding.get_funding_txo() != Some(original_funding_txo) {
+			// This should be unreachable once we opportunistically merge splices if the
+			// counterparty initializes a splice.
+			return Err("Funding changed out from under us".to_owned());


So this is preventing a user-initiated splice from happening if the counterparty happens to splice first. Why do we want this? Presumably, if they spliced, they still want to splice-in/out funds from their side of the channel and that hasn't happened yet.

We can't splice in that case anyway until the first splice locks, so I'm not really sure its worth holding the pending splice around in that case? Also the current code would break cause we need to recalculate if the splice as proposed is possible, and what the fees should be. But I didn't bother fixing it cause I'm not sure we should wait 6 blocks then do it.

We could initiate an RBF to do our splice but that's not supported yet either. Without that or being able to contribute on an incoming splice_init, then it seems we should queue a SpliceFailed event to the user?

Yep, seems like we should. We don't yet have a SpliceFailed event upstream yet, tho.

lightning/src/ln/channel.rs

wpaulino · 2025-08-27T19:36:48Z

lightning/src/ln/channel.rs

+		// TODO(splicing): if post_quiescence_action is set, integrate what the user wants to do
+		// into the counterparty-initiated splice. For always-on nodes this probably isn't a useful
+		// optimization, but for often-offline nodes it may be, as we may connect and immediately
+		// go into splicing from both sides.


If we lose the tie, we don't seem to consume the quiescent_action, but we also don't seem to attempt quiescence again once we exit so that we can perform ours. We only seem to retry quiescence with a pending quescient_action after a reconnection.

wpaulino

Feel free to squash

lightning/src/ln/channel.rs

Now that we have a `QuiescentAction` to track what we intend to do once we reach quiescence, we need to use it to initiate splices. Here we do so, adding a new `SpliceInstructions` to track the arguments that are currently passed to `splice_channel`. While these may not be exactly the right arguments to track in the end, a lot of the splice logic is still in flight, so we can worry about it later.

While we have a test to disconnect a peer if we're waiting on an `stfu` message, we also disconnect if we've reached quiescence but we're waiting on a peer to do "something fundamental" and they take too long to do so. We test that behavior here.

TheBlueMatt · 2025-08-28T21:56:20Z

Squashed and addressed @wpaulino's feeedback, plus a rustfmt thing

$ git diff-tree -U3 b37b5e4e7 9200308e8
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0ee9319df8..12d3b53905 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -10799,7 +10799,7 @@ where

 		// Check if a splice has been initiated already.
 		// Note: only a single outstanding splice is supported (per spec)
-		if self.pending_splice.is_some() {
+		if self.pending_splice.is_some() || self.quiescent_action.is_some() {
 			return Err(APIError::APIMisuseError {
 				err: format!(
 					"Channel {} cannot be spliced, as it has already a splice pending",
@@ -10913,9 +10913,7 @@ where

 		let original_funding_txo = self.funding.get_funding_txo().ok_or_else(|| {
 			debug_assert!(false);
-			APIError::APIMisuseError {
-				err: "Channel isn't yet fully funded".to_owned(),
-			}
+			APIError::APIMisuseError { err: "Channel isn't yet fully funded".to_owned() }
 		})?;

 		let (our_funding_inputs, our_funding_outputs, change_script) = contribution.into_tx_parts();
@@ -10947,12 +10945,6 @@ where
 			original_funding_txo,
 		} = instructions;

-		if self.funding.get_funding_txo() != Some(original_funding_txo) {
-			// This should be unreachable once we opportunistically merge splices if the
-			// counterparty initializes a splice.
-			return Err("Funding changed out from under us".to_owned());
-		}
-
 		// Check if a splice has been initiated already.
 		// Note: only a single outstanding splice is supported (per spec)
 		if self.pending_splice.is_some() {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 81c507b7d2..89992dd831 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4495,8 +4495,12 @@ where
 				let locktime = locktime.unwrap_or_else(|| self.current_best_block().height);
 				if let Some(chan) = chan_phase_entry.get_mut().as_funded_mut() {
 					let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-					let msg_opt = chan
-						.splice_channel(contribution, funding_feerate_per_kw, locktime, &&logger)?;
+					let msg_opt = chan.splice_channel(
+						contribution,
+						funding_feerate_per_kw,
+						locktime,
+						&&logger,
+					)?;
 					if let Some(msg) = msg_opt {
 						peer_state.pending_msg_events.push(MessageSendEvent::SendStfu {
 							node_id: *counterparty_node_id,

wpaulino · 2025-08-28T22:38:24Z

lightning/src/ln/channel.rs

+	change_script: Option<ScriptBuf>,
+	funding_feerate_per_kw: u32,
+	locktime: u32,
+	original_funding_txo: OutPoint,


This can be dropped now

Will do in a followup, thanks.

TheBlueMatt force-pushed the 2025-08-splice-quiescent-real branch from be3b93a to ddb3b37 Compare August 20, 2025 17:25

wpaulino reviewed Aug 20, 2025

View reviewed changes

jkczyz mentioned this pull request Aug 21, 2025

Dual-funded channels and Splicing Project Tracking #1621

Closed

7 tasks

TheBlueMatt force-pushed the 2025-08-splice-quiescent-real branch from ddb3b37 to d9a278c Compare August 26, 2025 16:36

TheBlueMatt marked this pull request as ready for review August 26, 2025 16:36

ldk-reviews-bot requested a review from valentinewallace August 26, 2025 16:37

wpaulino requested review from jkczyz and removed request for valentinewallace August 26, 2025 16:47

jkczyz reviewed Aug 27, 2025

View reviewed changes

wpaulino reviewed Aug 27, 2025

View reviewed changes

TheBlueMatt force-pushed the 2025-08-splice-quiescent-real branch from d9a278c to b37b5e4 Compare August 27, 2025 20:45

TheBlueMatt requested review from jkczyz and wpaulino August 27, 2025 20:54

wpaulino reviewed Aug 28, 2025

View reviewed changes

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

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

TheBlueMatt added 4 commits August 28, 2025 21:55

Reject splice_inits when we aren't quiescent

73a6d97

Add a quick TODO about merging cp- and locally-initiated splices

9200308

TheBlueMatt force-pushed the 2025-08-splice-quiescent-real branch from b37b5e4 to 9200308 Compare August 28, 2025 21:55

wpaulino approved these changes Aug 28, 2025

View reviewed changes

jkczyz approved these changes Aug 28, 2025

View reviewed changes

TheBlueMatt merged commit 2150f24 into lightningdevkit:main Aug 29, 2025
25 checks passed

TheBlueMatt mentioned this pull request Aug 29, 2025

Drop unused variable #4035

Merged

Integrate Splicing with Quiescence #4019

Integrate Splicing with Quiescence #4019

Uh oh!

Conversation

TheBlueMatt commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 27, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TheBlueMatt commented Aug 18, 2025 •

edited

Loading

codecov bot commented Aug 18, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 21, 2025 •

edited

Loading

TheBlueMatt Aug 27, 2025 •

edited

Loading