Fix initial `commitment_signed` for splicing #4014

jkczyz · 2025-08-15T00:49:24Z

When sending or handling the initial commitment_signed message for splicing, the previous commitment point and number must be used. Track this and fix the current behavior which was using the next commitment point and number by mistake. Also, refactor HolderCommitmentPoint as the use of "current" is confusing when prefixing holder_commitment_point fields with next_ and previous_.

Found when reviewing #3886 (comment).

The only difference between the two variants is the next point, which can be stored using an Option for simplicity. The naming of the Available variant is also confusing as it refers to the next commitment point. But HolderCommitmentPoint is typically used to represent the next point, which is actually stored in the current field. Drop the "current" nomenclature to avoid confusion.

The previous commitment point will be tracked in an upcoming commit. Rename existing HolderCommitmentPoint fields to avoid ambiguity.

ldk-reviews-bot · 2025-08-15T00:49:27Z

👋 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.

codecov · 2025-08-15T00:56:35Z

Codecov Report

❌ Patch coverage is 83.43949% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.85%. Comparing base (3b939c0) to head (96a7870).
⚠️ Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	83.33%	17 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4014      +/-   ##
==========================================
- Coverage   88.91%   88.85%   -0.06%     
==========================================
  Files         174      175       +1     
  Lines      125066   127727    +2661     
  Branches   125066   127727    +2661     
==========================================
+ Hits       111197   113488    +2291     
- Misses      11358    11675     +317     
- Partials     2511     2564      +53

Flag	Coverage Δ
fuzzing	`21.85% <48.07%> (-0.24%)`	⬇️
tests	`88.68% <83.43%> (-0.06%)`	⬇️

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-15T01:01:52Z

lightning/src/ln/channel.rs

@@ -6065,7 +6065,10 @@ where
 	///
 	/// This field is cleared once our counterparty sends a `channel_ready`.
 	pub interactive_tx_signing_session: Option<InteractiveTxSigningSession>,
-	holder_commitment_point: HolderCommitmentPoint,
+
+	/// The commitment point used for the next commitment transaction.


Suggested change

/// The commitment point used for the next commitment transaction.

/// The commitment point used for the next holder commitment transaction.

wpaulino · 2025-08-15T01:07:39Z

lightning/src/ln/channel.rs

@@ -6066,6 +6068,9 @@ where
 	/// This field is cleared once our counterparty sends a `channel_ready`.
 	pub interactive_tx_signing_session: Option<InteractiveTxSigningSession>,

+	/// The commitment point used for the previous commitment transaction.
+	previous_holder_commitment_point: Option<HolderCommitmentPoint>,


This should be current (aka the one most recently negotiated but not revoked) not previous?

Ah, yeah, that would be better. May have been just overzealous with ridding us of "current".

When splicing a channel, sending and receiving the initial commitment_signed message should use the current commitment point rather then the next one. Store this in FundedChannel whenever the commitment point is advanced and re-derive it using the signer upon restart.

jkczyz · 2025-08-15T14:51:01Z

lightning/src/ln/channel.rs

+				next_point,
+			},
+			(_, _) => {
+				let point = holder_signer.get_per_commitment_point(next_holder_commitment_transaction_number, &secp_ctx)
 						.expect("Must be able to derive the current commitment point upon channel restoration");


I suppose this should be "next"...

jkczyz · 2025-08-15T14:56:44Z

lightning/src/ln/channel.rs

+						&secp_ctx,
+					)
+					.expect(
+						"Must be able to derive the next commitment point upon channel restoration",


Which means this is next-next... 😛

Why do we attempt to get this here when next_holder_commitment_point_opt is None but don't bother above when it is Some? In the latter case, next_holder_commitment_point_next_advance_opt may be either.

jkczyz · 2025-08-15T14:59:08Z

lightning/src/ln/channel.rs

@@ -6066,6 +6068,9 @@ where
 	/// This field is cleared once our counterparty sends a `channel_ready`.
 	pub interactive_tx_signing_session: Option<InteractiveTxSigningSession>,

+	/// The commitment point used for the previous commitment transaction.
+	previous_holder_commitment_point: Option<HolderCommitmentPoint>,


Ah, yeah, that would be better. May have been just overzealous with ridding us of "current".

lightning/src/ln/channel.rs

TheBlueMatt · 2025-08-15T17:59:37Z

lightning/src/ln/channel.rs

@@ -14005,6 +14025,7 @@ where
 				is_holder_quiescence_initiator: None,
 			},
 			interactive_tx_signing_session,
+			previous_holder_commitment_point,


It seems really weird to store a { transaction_number, point, next_point } in FundedChannel when two of the three fields are redundant with next_holder_commitment_point. Why can't we just store the one extra key in HolderCommitmentPoint?

There are places in the code that take a HolderCommitmentPoint, so it wouldn't be clear which point is needed. Currently, next_point is simply used as a means to determine if we can advance. It's never actually accessed outside serialization.

Also, the code wouldn't be very readable as next_holder_commitment_point.previous_point() to get the current point. And if we keep the field named holder_commitment_point, it's "current" is the "next", and it's previous is the "current". The primary motivation if this approach is to avoid confusion.

That said, we can get away with only persisting the new point, even there is some in-memory duplication. We could probably clear the "next" point form current, too, since that shouldn't be needed after advancing the point.

There are places in the code that take a HolderCommitmentPoint, so it wouldn't be clear which point is needed.

Its not really clear which point is needed already? We need two different points depending on if we're validating a commitment_signed for a splice or during normal operation. I don't really see how passing a u64 and a PublicKey makes those places any less readable.

Also, the code wouldn't be very readable as next_holder_commitment_point.previous_point() to get the current point. And if we keep the field named holder_commitment_point, it's "current" is the "next", and it's previous is the "current". The primary motivation if this approach is to avoid confusion.

Seems like one more rename of the method would do it? next and current on a holder_commitment_point would be correct, and as you note we don't even access the after-next, so it would read fine?

TheBlueMatt · 2025-08-15T18:01:42Z

lightning/src/ln/channel.rs

+				next_holder_commitment_point.transaction_number() + 1;
+			let previous_point = holder_signer
+				.get_per_commitment_point(previous_holder_commitment_transaction_number, &secp_ctx)
+				.expect(


We can't always rely on this on startup, we need to persist the new point. The logic we had relied on was "you have to do a sync-signer load once, then you can switch to async signing", but now we require you to have a sync signer always on startup, which isn't acceptable.

I'm also kinda not a fan of requiring a sync signer again once after we told people they can do async signing after loading once with a previous version. Given this is only used in splicing is it that much work to make the new key Optional and just fail splices until its filled in (which should happen any time we step the state machine forward)?

Sure, that should be simple enough to do. Though, we'd advertise to our peer that channel supports splicing but may fail on the first attempt.

I think that's fine. "If you're using async signing then splicing may fail until you do a channel update" seems like a rare enough thing that we can just document it and live with it. Not many of our users do async signing anyway.

When splicing a channel, the initial commitment_signed received should use the same commitment number and point previously received prior to splicing the channel. Account for this by checking the commitment_signed against that one instead, which is now stored separately in FundedChannel.

When splicing a channel, the initial commitment_signed should use the same commitment number and point previously sent. Account for this by adjusting these to use the previous commitment number and point, since the next expected one is stored.

TheBlueMatt

I'm a bit uncertain about the design for the access to the previous point - #4014 (comment)

jkczyz added 3 commits August 14, 2025 19:34

Rename UnfundedChannelContext::holder_commitment_point

40a55f4

The previous commitment point will be tracked in an upcoming commit. Rename existing HolderCommitmentPoint fields to avoid ambiguity.

Rename FundedChannel::holder_commitment_point

0160e54

The previous commitment point will be tracked in an upcoming commit. Rename existing HolderCommitmentPoint fields to avoid ambiguity.

jkczyz self-assigned this Aug 15, 2025

jkczyz requested review from TheBlueMatt and wpaulino August 15, 2025 00:49

jkczyz mentioned this pull request Aug 15, 2025

Update channel_reestablish for splicing #3886

Open

wpaulino reviewed Aug 15, 2025

View reviewed changes

jkczyz force-pushed the 2025-08-splice-initial-commitment-signed branch from 57ece8a to cb521b4 Compare August 15, 2025 01:25

jkczyz added 3 commits August 15, 2025 10:19

f - fix docs

ed68abe

f - s/previous/current

661214e

jkczyz force-pushed the 2025-08-splice-initial-commitment-signed branch from cb521b4 to e2ed042 Compare August 15, 2025 15:30

jkczyz commented Aug 15, 2025

View reviewed changes

jkczyz requested a review from wpaulino August 15, 2025 15:32

TheBlueMatt reviewed Aug 15, 2025

View reviewed changes

jkczyz added 4 commits August 15, 2025 14:46

f - persist current_holder_commitment_point

50de598

Drop outdated async signing comment

96a7870

jkczyz force-pushed the 2025-08-splice-initial-commitment-signed branch from e2ed042 to 96a7870 Compare August 15, 2025 19:56

jkczyz requested a review from TheBlueMatt August 15, 2025 21:49

TheBlueMatt reviewed Aug 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix initial `commitment_signed` for splicing #4014

Fix initial `commitment_signed` for splicing #4014

jkczyz commented Aug 15, 2025

Uh oh!

ldk-reviews-bot commented Aug 15, 2025 •

edited

Loading

Uh oh!

codecov bot commented Aug 15, 2025 •

edited

Loading

Uh oh!

wpaulino Aug 15, 2025

Uh oh!

wpaulino Aug 15, 2025

Uh oh!

jkczyz Aug 15, 2025

Uh oh!

jkczyz Aug 15, 2025

Uh oh!

jkczyz Aug 15, 2025

Uh oh!

jkczyz Aug 15, 2025

Uh oh!

Uh oh!

TheBlueMatt Aug 15, 2025

Uh oh!

jkczyz Aug 15, 2025

Uh oh!

TheBlueMatt Aug 17, 2025

Uh oh!

TheBlueMatt Aug 15, 2025

Uh oh!

jkczyz Aug 15, 2025

Uh oh!

TheBlueMatt Aug 17, 2025

Uh oh!

TheBlueMatt left a comment

Uh oh!

Uh oh!

	/// The commitment point used for the next commitment transaction.
	/// The commitment point used for the next holder commitment transaction.

Fix initial commitment_signed for splicing #4014

Are you sure you want to change the base?

Fix initial commitment_signed for splicing #4014

Conversation

jkczyz commented Aug 15, 2025

Uh oh!

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

Uh oh!

Uh oh!

codecov bot commented Aug 15, 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!

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!

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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix initial `commitment_signed` for splicing #4014

Fix initial `commitment_signed` for splicing #4014

ldk-reviews-bot commented Aug 15, 2025 •

edited

Loading

codecov bot commented Aug 15, 2025 •

edited

Loading