Add splice details to `TransactionType::Splice` by jkczyz · Pull Request #4570 · lightningdevkit/rust-lightning

jkczyz · 2026-04-17T21:14:13Z

Surface splice broadcast metadata downstream, so consumers (notably LDK Node) can reconcile PaymentDetails / PendingPaymentDetails directly from the broadcaster callback.

TransactionType::Splice now carries contribution: Option<FundingContribution> — our local contribution for this round, and replaced_txid: Option<Txid> — the prior negotiated candidate being replaced on RBF. The alternative (reacting to SplicePending) is worse: that event races with BDK wallet sync and doesn't carry the replaced txid.
FundingContribution gains public getters for estimated_fee, inputs, and max_feerate; feerate is elevated from pub(super) to pub. Combined with existing value_added, outputs, change_output, this exposes everything a downstream consumer needs without reaching into the raw transaction.

The Hash derive on TransactionType is dropped (unused in the workspace; FundingContribution transitively contains ConfirmedUtxo which lacks Hash).

ldk-reviews-bot · 2026-04-17T21:14:16Z

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

ldk-claude-review-bot · 2026-04-17T21:21:28Z

No issues found.

I've thoroughly re-reviewed every file and hunk in this diff:

chaininterface.rs: The InteractiveFunding(Vec<FundingCandidate>) variant and supporting types (FundingCandidate, ChannelFunding, FundingPurpose) are well-designed. Serialization impls use correct TLV field IDs and appropriate modes (required_vec for channels ensures it's always present). Hash derive removal is safe and necessary.
channel.rs (lines 9398–9422): Contribution alignment via contrib_offset is correct. The saturating_sub / checked_sub / filter_map chain handles all edge cases (initiator-only contributions, acceptor starting mid-sequence, get_funding_txid() returning None). The invariant contributions.len() <= negotiated_candidates.len() is maintained by all push/pop paths (try_enter_stfu, reset_pending_splice_state).
channelmanager.rs (line 11139): Pattern match update from Splice { .. } to InteractiveFunding(..) is correct and no other match sites were missed (confirmed via codebase search).
funding.rs: New public getters (estimated_fee, inputs, feerate, max_feerate) are straightforward delegations. All return types are already public. The feerate() visibility widening from pub(super) to pub doesn't affect existing callers (all within ln).
splicing_tests.rs: All 25+ call-site updates pass correct expected_replaced_txid: None for first splice, Some(prior_tx.compute_txid()) for RBF rounds, None after tiebreak (fresh splice, not an RBF). The loop-based RBF tests (test_splice_rbf_rejects_low_feerate_after_several_attempts, test_splice_rbf_rejects_own_low_feerate_after_several_attempts) correctly chain prev_splice_tx tracking through iterations.

The pre-existing concern about 0-conf splice promotion rebroadcast using TransactionType::Funding (flagged in my prior review) remains valid but is not introduced by this PR.

ldk-reviews-bot · 2026-04-20T00:00:55Z

🔔 1st Reminder

Hey @TheBlueMatt @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 · 2026-04-20T00:00:57Z

🔔 1st Reminder

Hey @TheBlueMatt @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 · 2026-04-22T00:01:41Z

🔔 2nd Reminder

Hey @TheBlueMatt @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 · 2026-04-22T00:01:43Z

🔔 2nd Reminder

Hey @TheBlueMatt @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-claude-review-bot · 2026-04-22T18:06:12Z

+		if !self.context.is_manual_broadcast
+			&& !waiting_for_batch
+			&& self.funding.get_funding_tx_confirmation_height().is_none()
+		{
+			if let Some(tx) = &self.funding.funding_transaction {
+				return Some((
+					tx.clone(),
+					TransactionType::Funding {
+						channels: vec![(
+							self.context.counterparty_node_id,
+							self.context.channel_id,
+						)],
+					},
+				));
+			}
+		}


Bug: After 0-conf splice promotion, self.pending_splice is None (cleared at maybe_promote_splice_funding line 11680) and self.funding holds the splice's FundingScope with funding_tx_confirmation_height == 0 and funding_transaction = Some(splice_tx). This initial-funding branch will match:

is_manual_broadcast = false (normal channel)

waiting_for_batch = false (channel is ChannelReady)

get_funding_tx_confirmation_height().is_none() = true (height is 0)

funding_transaction = Some(splice_tx)

...and return TransactionType::Funding wrapping the splice transaction. The splice branch below is unreachable because self.pending_splice is None.

The test test_splice_rebroadcast_uses_splice_type_after_0conf_promotion (added in this PR) expects TransactionType::Splice with the original contribution preserved, but this code would produce TransactionType::Funding. That test will fail.

A possible fix: check self.funding.channel_transaction_parameters.splice_parent_funding_txid — it is Some(...) for promoted splices and None for initial funding. You'd also need to persist the contribution and replaced_txid metadata somewhere that survives promotion (e.g., on FundingScope or ChannelContext), since pending_splice is cleared.

Discussed offline. Dropped the commit to retry for now. We may do this later in ChannelMonitor where we'll store the FundingContribution.

TheBlueMatt

Needs rebase, also ci is very sad.

`TransactionType::Splice` now carries the local contribution to the splice and, for RBF, the txid of the prior negotiated candidate being replaced. This lets LDK Node update its `PaymentDetails` and `PendingPaymentDetails` from the broadcast callback without re-deriving the contribution from the raw transaction or tracking RBF chains itself. Driving these updates from the `SplicePending` event instead is more complicated because that event races with BDK wallet syncing and doesn't carry the replaced txid; the broadcast callback is a cleaner integration point. The `Hash` derive on `TransactionType` is dropped since it isn't used anywhere in the workspace and `FundingContribution` (now embedded) transitively contains `ConfirmedUtxo`, which doesn't derive `Hash`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jkczyz · 2026-04-23T22:17:32Z

Rebased. Compilation failures were from new tests in main.

codecov · 2026-04-23T23:10:49Z

Codecov Report

❌ Patch coverage is 52.63158% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.08%. Comparing base (2313bd5) to head (dacf092).

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	25.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4570      +/-   ##
==========================================
- Coverage   87.08%   87.08%   -0.01%     
==========================================
  Files         161      161              
  Lines      109255   109271      +16     
  Branches   109255   109271      +16     
==========================================
+ Hits        95147    95154       +7     
- Misses      11627    11632       +5     
- Partials     2481     2485       +4

Flag	Coverage Δ
fuzzing-fake-hashes	`30.88% <36.84%> (-0.10%)`	⬇️
fuzzing-real-hashes	`22.66% <0.00%> (+0.01%)`	⬆️
tests	`86.16% <52.63%> (-0.01%)`	⬇️

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.

ldk-reviews-bot · 2026-04-25T00:00:43Z

🔔 3rd Reminder

Hey @TheBlueMatt @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 · 2026-04-27T00:00:15Z

🔔 1st Reminder

Hey @TheBlueMatt @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 · 2026-04-27T00:01:48Z

🔔 4th Reminder

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

tnull · 2026-04-27T07:38:56Z

+		contribution: Option<FundingContribution>,
+		/// For an RBF replacement, the txid of the prior negotiated splice candidate being
+		/// replaced. `None` for the first splice attempt.
+		replaced_txid: Option<Txid>,


Hmm, what happens when we make multiple subsequent RBFs? Could/should we make this a Vec and track all prior conflicting Txids to make sure we're not losing any intermediary step when tracking these events?

Also, when integrating BDK's RBF events in LDK Node we found that it would be nice if the API clearly defined an ordering for the replaced_txids, i.e., so we can always identify the 'original' txid (under which we started tracking a certain payment). That is, if we make this a Vec it would be great if the API could guarantee that replaced_txids[0] is always the 'original'.

That seems reasonable, though things get hairy if we have multiple splice-RBFs that conflict across channels. ISTM LDK Node might need to track that somehow, but maybe we can just avoid ever building such things?

Ok, discussed a bit offline. Ideally, we'll want to use a format in LDK Node that is compatible with batching. I've pushed a change that refactors TransactionType::Splice to TransactionType::InteractiveFunding, which supports batches of both v2 channel establishment and splices in the future. It also contains all previous attempts (including contributions) along with the corresponding txid instead of replaced_txid. Including simplifies the downstream handling. Then in 0.4 we can add re-broadcasts.

See how it's used in LDK Node here: lightningdevkit/ldk-node#888. LMK what you think.

TheBlueMatt

Changes LGTM, no strong opinion on the API, if it works for LDK Node alright, though I agree exposing all the previous RBFs might be nice.

TheBlueMatt · 2026-04-28T14:23:00Z

+		contribution: Option<FundingContribution>,
+		/// For an RBF replacement, the txid of the prior negotiated splice candidate being
+		/// replaced. `None` for the first splice attempt.
+		replaced_txid: Option<Txid>,


That seems reasonable, though things get hairy if we have multiple splice-RBFs that conflict across channels. ISTM LDK Node might need to track that somehow, but maybe we can just avoid ever building such things?

ldk-reviews-bot · 2026-04-29T00:02:18Z

🔔 5th 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.

Rename TransactionType::Splice to TransactionType::InteractiveFunding and reshape it as Vec<FundingCandidate> (original + RBF attempts) where each candidate carries one or more ChannelFunding entries (forward- compatible with batched splices and V2 channel establishment, neither of which is implemented yet). Per-candidate contribution is tail-aligned with negotiated_candidates so leading rounds where we didn't contribute yield None. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Implement Writeable/Readable for FundingCandidate, ChannelFunding, and FundingPurpose so downstream consumers (e.g., LDK Node) can persist these types directly alongside their own state without having to mirror them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Add public getters for `estimated_fee`, `inputs`, and `max_feerate`, and elevate `feerate` from `pub(super)` to `pub`. Together with the existing `value_added`, `outputs`, and `change_output`, this gives downstream consumers of `TransactionType::Splice` (notably LDK Node, which updates `PaymentDetails` from the broadcast callback) the data they need without reaching into the raw transaction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jkczyz requested review from TheBlueMatt and wpaulino April 17, 2026 21:14

ldk-claude-review-bot reviewed Apr 22, 2026

View reviewed changes

jkczyz force-pushed the 2026-04-splice-transaction-type branch from dbaf6d9 to 86dcede Compare April 22, 2026 18:36

TheBlueMatt reviewed Apr 23, 2026

View reviewed changes

jkczyz self-assigned this Apr 23, 2026

jkczyz added this to Weekly Goals Apr 23, 2026

jkczyz force-pushed the 2026-04-splice-transaction-type branch from 86dcede to dacf092 Compare April 23, 2026 21:42

jkczyz requested a review from TheBlueMatt April 23, 2026 22:17

tnull reviewed Apr 27, 2026

View reviewed changes

TheBlueMatt previously approved these changes Apr 28, 2026

View reviewed changes

jkczyz dismissed TheBlueMatt’s stale review via 925b58d April 29, 2026 20:42

jkczyz force-pushed the 2026-04-splice-transaction-type branch from dacf092 to 925b58d Compare April 29, 2026 20:42

jkczyz and others added 2 commits April 29, 2026 16:11

jkczyz force-pushed the 2026-04-splice-transaction-type branch from 925b58d to 4b82770 Compare April 29, 2026 21:13

jkczyz requested review from TheBlueMatt and tnull April 29, 2026 22:35

Conversation

jkczyz commented Apr 17, 2026

Uh oh!

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

Uh oh!

Uh oh!

ldk-claude-review-bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 20, 2026

Uh oh!

ldk-reviews-bot commented Apr 20, 2026

Uh oh!

ldk-reviews-bot commented Apr 22, 2026

Uh oh!

ldk-reviews-bot commented Apr 22, 2026

Uh oh!

ldk-claude-review-bot Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Apr 23, 2026

Uh oh!

codecov Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Apr 25, 2026

Uh oh!

ldk-reviews-bot commented Apr 27, 2026

Uh oh!

ldk-reviews-bot commented Apr 27, 2026

Uh oh!

tnull Apr 27, 2026 • 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!

TheBlueMatt Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Apr 17, 2026 •

edited

Loading

ldk-claude-review-bot commented Apr 17, 2026 •

edited

Loading

codecov Bot commented Apr 23, 2026 •

edited

Loading

tnull Apr 27, 2026 •

edited

Loading