Rework ChannelManager::funding_transaction_signed #4336

wpaulino · 2026-01-22T17:53:58Z

Alternative version to #4257

ldk-reviews-bot · 2026-01-22T17:54:02Z

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

codecov · 2026-01-22T19:19:08Z

Codecov Report

❌ Patch coverage is 72.00957% with 117 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.07%. Comparing base (8cdc86a) to head (be67c67).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	72.72%	54 Missing and 6 partials ⚠️
lightning/src/ln/channelmanager.rs	71.42%	46 Missing and 10 partials ⚠️
lightning/src/ln/interactivetxs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4336      +/-   ##
==========================================
- Coverage   86.53%   86.07%   -0.47%     
==========================================
  Files         158      156       -2     
  Lines      103190   102804     -386     
  Branches   103190   102804     -386     
==========================================
- Hits        89300    88490     -810     
- Misses      11469    11803     +334     
- Partials     2421     2511      +90

Flag	Coverage Δ
fuzzing	`?`
tests	`86.07% <72.00%> (+0.24%)`	⬆️

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.

lightning/src/ln/channel.rs

jkczyz · 2026-01-23T18:27:43Z

lightning/src/ln/channelmanager.rs

+						// We should never be sending a `commitment_signed` in response to their
+						// `tx_signatures`.
+						debug_assert!(commitment_signed.is_none());
+
 						if let Some(tx_signatures) = tx_signatures {
 							peer_state.pending_msg_events.push(MessageSendEvent::SendTxSignatures {
 								node_id: *counterparty_node_id,
 								msg: tx_signatures,
 							});
 						}


I'm a little confused by this. Why would we send our tx_signatures if we aren't sending our commitment_signed? Didn't we want to send both at once?

We've already sent it by this point, we're sending our tx_signatures here in response to theirs

jkczyz · 2026-01-23T18:49:11Z

lightning/src/ln/channelmanager.rs

+		// We may need to regenerate [`Event::FundingTransactionReadyForSigning`] for channels that
+		// still need their holder `tx_signatures`.


Does it make sense to start persisting this?

Matt pointed out we shouldn't #4257 (comment)

@TheBlueMatt Is the goal eventually to not write events that (a) can be re-constructed from ChannelMonitor / FundedChannel state or (b) aren't applicable across a restart? Presumable this falls into that category.

Seems there will at least be some events that aren't tied to a particular channel, so we may still need to persist some events?

ldk-reviews-bot · 2026-01-26T00:00:19Z

🔔 1st Reminder

Hey @TheBlueMatt! 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.

TheBlueMatt

Hmm, seems like the last commit should really get have test coverage? I guess just our dual-funding coverage isn't that great?

Also, presumably this needs documentation for how to cancel a splice instead of signing and a test that does so?

lightning/src/ln/channelmanager.rs

wpaulino · 2026-01-26T19:56:01Z

Hmm, seems like the last commit should really get have test coverage? I guess just our dual-funding coverage isn't that great?

Yeah things aren't really hooked up yet to do proper testing.

Also, presumably this needs documentation for how to cancel a splice instead of signing and a test that does so?

Planned for a separate PR, no need for it to happen here.

TheBlueMatt

Question somewhat orthogonal to this PR - do we need a monitor update before sending our tx-sigs? Once we send it, its possible for our counterparty to respond with a commitment sigs and then we can't cancel. If we restart without chanman persistence and the user tries to cancel at that point we'll end up getting force-closed on.

I continue to be somewhat annoyed at how many expects there are in the splicing logic, and would feel a lot better if we had a full_stack_target seed that spliced so that at least the fuzzer could explore it a tiny bit (though of course a larger refactor to reduce them would be nice too).

lightning/src/ln/channel.rs

ldk-reviews-bot · 2026-01-29T00:35:13Z

🔔 1st Reminder

Hey @jkczyz! 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 · 2026-01-29T16:41:52Z

lightning/src/ln/channelmanager.rs

+		// We may need to regenerate [`Event::FundingTransactionReadyForSigning`] for channels that
+		// still need their holder `tx_signatures`.


@TheBlueMatt Is the goal eventually to not write events that (a) can be re-constructed from ChannelMonitor / FundedChannel state or (b) aren't applicable across a restart? Presumable this falls into that category.

Seems there will at least be some events that aren't tied to a particular channel, so we may still need to persist some events?

jkczyz · 2026-01-29T16:52:56Z

lightning/src/ln/channelmanager.rs

+										);
+									}
 								}
 								return NotifyOption::DoPersist;


Is this independent of any events being generated? (i.e., some stated in the channel was made, but is_connected was false?

We still want to persist the funding signatures regardless of channel connectivity

jkczyz · 2026-01-29T16:58:09Z

lightning/src/ln/channelmanager.rs

+
+						// We should never be sending a `commitment_signed` in response to their
+						// `tx_signatures`.
+						debug_assert!(commitment_signed.is_none());
+


Would it make sense to return an error here?

If we actually do, they'll force close on us, so we get the same result.

jkczyz · 2026-01-29T17:13:14Z

lightning/src/ln/interactivetxs.rs

 		let funding_tx_opt = self.maybe_finalize_funding_tx();
-		let holder_tx_signatures = (self.holder_sends_tx_signatures_first
-			|| self.has_received_tx_signatures())
+		let holder_tx_signatures = (self.has_received_commitment_signed


Is has_received_commitment_signed implied from has_received_tx_signatures?

Yes, we force close when receiving tx_signatures without the commitment_signed first. I'd prefer keeping the code as is for clarity though.

lightning/src/ln/channel.rs

Previously, we'd emit a FundingTransactionReadyForSigning event once the initial commitment_signed is exchanged for a splicing/dual-funding attempt and require users to call back with their signed inputs using ChannelManager::funding_transaction_signed. While this approach worked in practice, it prevents us from abandoning a splice if we cannot or no longer wish to sign as the splice has already been committed to by this point. This commit reworks the API such that this is now possible. After exchanging tx_complete, we will no longer immediately send our initial commitment_signed. We will now emit the FundingTransactionReadyForSigning event and wait for the user to call back before releasing both our initial commitment_signed and our tx_signatures. As a result, the event is now persisted, as there is only one possible path in which it is generated. Note that we continue to only emit the event if a local contribution to negotiated transaction was made. Future work will expose a cancellation API such that we can abandon splice attempts safely (we can just force close the channel with dual-funding).

This is crucial to enable the splice cancellation use case. When we process the initial commitment signed from our counterparty, we queue a monitor update that cannot be undone. To give the user a chance to abort the splice negotiation before it's committed to, we buffer the message until a successful call to `Channel::funding_transaction_signed` and process it then. Note that this is currently only done for splice and RBF attempts, as if we want to abort a dual funding negotiation, we can just force close the channel as it hasn't been funded yet.

Now that we require users to first call `ChannelManager::funding_transaction_signed` before releasing any signatures, it's possible that it is called before we receive the initial commitment signed from our counterparty, which would transition the channel to funded. Because of this, we need to support the API call while the channel is still in the unfunded phase. Note that this commit is mostly a code move of `FundedChannel::funding_transaction_signed` to `Channel::funding_transaction_signed` that doesn't alter the signing logic.

wpaulino added this to the 0.3 milestone Jan 22, 2026

wpaulino requested review from TheBlueMatt and jkczyz January 22, 2026 17:53

wpaulino self-assigned this Jan 22, 2026

wpaulino force-pushed the funding-transaction-signed-rework branch from c0193fe to 0fc923c Compare January 22, 2026 19:59

jkczyz reviewed Jan 23, 2026

View reviewed changes

TheBlueMatt reviewed Jan 26, 2026

View reviewed changes

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

wpaulino force-pushed the funding-transaction-signed-rework branch 2 times, most recently from 734ad69 to 04a7560 Compare January 27, 2026 00:34

wpaulino requested review from TheBlueMatt and jkczyz January 27, 2026 00:34

wpaulino mentioned this pull request Jan 28, 2026

Support async signing of interactive-tx initial commitment signatures #4355

Open

TheBlueMatt previously approved these changes Jan 28, 2026

View reviewed changes

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

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

wpaulino mentioned this pull request Jan 28, 2026

Consider adding an extra monitor update upon ChannelManager::funding_transaction_signed #4358

Open

wpaulino dismissed TheBlueMatt’s stale review via f2dd6f2 January 28, 2026 23:31

wpaulino force-pushed the funding-transaction-signed-rework branch from 04a7560 to f2dd6f2 Compare January 28, 2026 23:31

jkczyz reviewed Jan 29, 2026

View reviewed changes

wpaulino added 3 commits January 29, 2026 10:01

wpaulino force-pushed the funding-transaction-signed-rework branch from f2dd6f2 to be67c67 Compare January 29, 2026 18:03

jkczyz approved these changes Jan 29, 2026

View reviewed changes

TheBlueMatt approved these changes Jan 30, 2026

View reviewed changes

TheBlueMatt merged commit fcc3d33 into lightningdevkit:main Jan 30, 2026
21 checks passed

wpaulino deleted the funding-transaction-signed-rework branch January 30, 2026 02:08

		// We may need to regenerate [`Event::FundingTransactionReadyForSigning`] for channels that
		// still need their holder `tx_signatures`.

Rework ChannelManager::funding_transaction_signed #4336

Rework ChannelManager::funding_transaction_signed #4336

Uh oh!

Conversation

wpaulino commented Jan 22, 2026

Uh oh!

ldk-reviews-bot commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

ldk-reviews-bot commented Jan 26, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wpaulino commented Jan 26, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jan 29, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Jan 22, 2026 •

edited

Loading

codecov bot commented Jan 22, 2026 •

edited

Loading