Introduce FundingContributionBuilder API by wpaulino · Pull Request #4516 · lightningdevkit/rust-lightning

wpaulino · 2026-03-27T00:05:20Z

This PR refactors splice contribution construction around a new FundingBuilder API and tightens the semantics of how splice value is funded. It replaces the older FundingTemplate contribution helpers with a builder flow that can reuse, amend, or replace prior contributions for fresh splices and RBF attempts.

ldk-reviews-bot · 2026-03-27T00:05:22Z

👋 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 · 2026-03-27T00:56:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.08%. Comparing base (5704e8e) to head (9f9fe58).
⚠️ Report is 62 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4516 +/- ##
==========================================
+ Coverage 86.99% 87.08% +0.08% 
==========================================
Files 163 161 -2 Lines 108635 109248 +613 Branches 108635 109248 +613 ==========================================
+ Hits 94511 95138 +627 + Misses 11647 11629 -18 - Partials 2477 2481 +4

Flag	Coverage Δ
fuzzing	`39.65% <34.21%> (-0.65%)`	⬇️
tests	`86.17% <84.21%> (+0.08%)`	⬆️

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.

TheBlueMatt

basically lgtm

wpaulino · 2026-03-30T18:54:59Z

Leaving the explicit input support for a follow-up as this PR is large enough already.

ldk-claude-review-bot · 2026-03-30T19:01:30Z

+ .await
+ .map_err(|_| FundingContributionError::CoinSelectionFailed)?;
+
+ return Ok(FundingContribution::new(


Nit: unnecessary return in final expression position. Same on line 740 for the sync variant.
Suggested change
returnOk(FundingContribution::new(
Ok(FundingContribution::new(

ldk-claude-review-bot · 2026-03-30T19:02:04Z

Review Summary — PR #4516: Introduce FundingContributionBuilder API

New issue posted this pass

fuzz/src/chanmon_consistency.rs:1526 — splice_out additive output semantics doubles the withdrawal amount on RBF calls in the fuzz test. The old splice_out_sync discarded the prior contribution; the new splice_out seeds from it and appends outputs.

Previously flagged issues (all still present)

Critical:

funding.rs:570-578 — Breaking TLV tag renumbering for persisted FundingContribution.
funding.rs:348 — splice_in/splice_in_sync additive semantics doubles value_added during RBF.

Correctness:
3. funding.rs:647 — Wrong fee estimate used in sufficiency check after dropping change output.
4. funding.rs:700-714 — No sufficiency check in the no-change fallthrough of amend_without_coin_selection.
5. funding.rs:636-644 — amend_without_coin_selection always fails for amended splice-out priors.
6. funding.rs:1103-1104 — build_from_prior_contribution failure blocks fresh splice-out fallthrough.
7. funding.rs:1076-1080 — FundingInputs::None silently discards prior wallet inputs when value_added == 0.
8. funding.rs:431 — rbf_prior_contribution now requires a prior contribution, removing old fee-bump-only capability.
9. funding.rs:1172 — validate_contribution_parameters blocks reuse of fee-bump-only prior contributions.
10. channel.rs:12332-12341 — splice_channel now fails with APIError where it previously silently degraded.

Diagnostic / DX:
11. funding.rs:1067-1073 — All feerate adjustment failures mapped to MissingCoinSelectionSource.
12. funding.rs:1292-1300 — saturating_add/saturating_sub for bitcoin amounts can silently truncate/zero.

Code quality:
13. funding.rs:1365,1465 — add_value/remove_value duplicated across async/sync builder impls.
14. funding.rs:1420,1519 — Unnecessary return keyword in final expression.

Cross-cutting concerns

The additive semantics of splice_in and splice_out are surprising for convenience methods. Both fuzz tests (chanmon_consistency.rs splice_in and splice_out closures) and the do_initiate_rbf_splice_in test helper are affected.
The MissingCoinSelectionSource error is overloaded across multiple failure modes.
The wallet-backed contribution design change (wallet inputs fund withdrawals) is a significant semantic shift from the old splice_in_and_out behavior.

TheBlueMatt

I didn't look too deeply at the tests but basically LGTM.

wpaulino · 2026-03-31T20:28:56Z

Had to rebase due to a small import conflict.

TheBlueMatt

needs rebase again tho

jkczyz

Code looks good, but I think merging some impl blocks will help the diff.

The `holder_balance` is computed by `FundedChannel::get_holder_counterparty_balances_floor_incl_fee`, which may unexpectedly fail due to the balance either being too high or too low. These cases are highly unlikely to happen given we have validation to ensure we never enter such a state to begin with. If they were to happen, something has gone wrong with the channel and it doesn't make sense to allow splicing anyway. Therefore, we opt to make `PriorContribution::holder_balance` non-optional and return an error that the channel cannot be spliced at the moment.

This commit removes `FundingContribution::value_added` as tracking it is unnecessary -- it can just be derived from the total amount in minus total amount out minus fees.

jkczyz

PR description could probably be completed now.

jkczyz

Mostly LGTM.

jkczyz · 2026-04-16T23:16:03Z

+ let total_output_value = self
 .outputs
 .iter()
+ .chain(self.change_output.iter())
 .map(|txout| txout.value)
 .sum::<Amount>()
 .to_signed()
- .expect("value_removed is validated to not exceed Amount::MAX_MONEY");
-
- let contribution_amount = value_added - value_removed;
- contribution_amount
- .checked_sub(unpaid_fees)
+ .expect("total_output_value is validated to not exceed Amount::MAX_MONEY");


I see that the total input value is checked in validate_inputs, but I don't see where we validate the outputs.

It's in validate_contribution_parameters

ldk-reviews-bot · 2026-04-18T16:50:43Z

🔔 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

Some nits on docs but I'm happy with the logic now. Will finish a full review after lunch.

TheBlueMatt · 2026-04-17T14:32:31Z

 	pub fn value_added(&self) -> Amount {
- self.value_added
+ let total_input_value = self.inputs.iter().map(|i| i.utxo.output.value).sum::<Amount>();
+ let total_output_value = self.outputs.iter().map(|output| output.value).sum::<Amount>();


I'm confused how this differs from net_value now - they appear to be identical, do we still need both?

Mostly just used to preserve the value_added when resuming from a prior contribution. If we had a net-negative contribution, then this returns 0 rather than the negative value itself.

When a user requests to add value via coin-selected inputs, we should strive to fulfill their request. Allowing them to remove value from the channel is undesired as it goes against their request. While we still allow adding outputs to enabled mixed contributions, their funds must now always come from the set of coin-selected inputs, and must never draw from the channel balance resulting in a smaller added value.

This lets callers easily amend a prior contribution in place and only re-run coin selection when the new request cannot be satisfied with the existing inputs.

This results in a slight change of behavior: now these methods reuse and amend the prior contribution, as opposed to always starting from a fresh contribution, which would be the desired expected behavior by users.

ldk-claude-review-bot · 2026-04-20T23:42:29Z

- FeeRate::MAX,
- &WalletSync::new(wallet, logger.clone()),
- )
+ funding_template.splice_out(outputs, feerate, FeeRate::MAX)


Bug: splice_out now has additive output semantics when a prior contribution exists (via with_prior_contribution). On RBF calls, the builder is seeded with the prior's outputs, then add_outputs(outputs) appends the same withdrawal again — doubling the withdrawal amount.
Trace for the RBF case:
Prior contribution has outputs = [TxOut { value: 546, ... }], no inputs (splice-out)
Builder seeds from prior: outputs = [withdrawal]
splice_out calls add_outputs([withdrawal]) → outputs = [withdrawal, withdrawal]
build() → amend_without_coin_selection constructs a splice-out with doubled outputs
compute_feerate_adjustment checks fee + 1092 <= holder_balance — which passes (capacity > 20K sat)
Result: the RBF splice-out silently withdraws 2 × 546 = 1092 sat instead of 546 sat
The old splice_out_sync destructured the template with .. which discarded the prior contribution, so outputs were always treated as absolute. The new splice_out is additive.
Fix: use without_prior_contribution in the closure when a prior exists, or use rbf_prior_contribution_sync for the RBF case. For example:
let has_prior = funding_template.prior_contribution().is_some();if has_prior { funding_template.rbf_prior_contribution_sync(Some(feerate),FeeRate::MAX, wallet)}else{ funding_template.splice_out(outputs, feerate,FeeRate::MAX)}
(The splice_in closure has the analogous issue — already flagged in a prior review pass.)

@wpaulino Should we address this given the earlier comment?
// We conditionally splice out `MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS` only when the node// has double the balance required to send a payment upon a `0xff` byte. We do this to// ensure there's always liquidity available for a payment to succeed then.

Discussed offline. We'll wait for #4550 to land.

TheBlueMatt · 2026-04-21T20:53:27Z

+ }
+ }
+ }
+


Shouldn't we also be rejecting here if we don't have enough to meet the previously-requested target value?

TheBlueMatt

One comment, otherwise post-merge ACK

wpaulino added this to the 0.3 milestone Mar 27, 2026

wpaulino requested review from TheBlueMatt and jkczyz March 27, 2026 00:05

wpaulino self-assigned this Mar 27, 2026

TheBlueMatt reviewed Mar 27, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated

jkczyz reviewed Mar 27, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated

wpaulino force-pushed the funding-contribution-builder branch from 0968836 to 2da45efCompare March 30, 2026 18:54

wpaulino marked this pull request as ready for review March 30, 2026 18:54

wpaulino requested review from TheBlueMatt and jkczyz March 30, 2026 18:54

ldk-claude-review-bot reviewed Mar 30, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

ldk-claude-review-bot reviewed Mar 30, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs

ldk-claude-review-bot reviewed Mar 30, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

ldk-claude-review-bot reviewed Mar 30, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs

ldk-claude-review-bot reviewed Mar 30, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

jkczyz reviewed Mar 31, 2026
View reviewed changes

TheBlueMatt reviewed Mar 31, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs

wpaulino force-pushed the funding-contribution-builder branch from 2da45ef to 19b230bCompare March 31, 2026 20:00

wpaulino requested review from TheBlueMatt and jkczyz March 31, 2026 20:29

ldk-claude-review-bot reviewed Mar 31, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

ldk-claude-review-bot reviewed Mar 31, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

TheBlueMatt previously approved these changes Apr 1, 2026
View reviewed changes

jkczyz reviewed Apr 2, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated

jkczyz reviewed Apr 9, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs
Comment threadlightning/src/ln/splicing_tests.rs

wpaulino force-pushed the funding-contribution-builder branch from de4ccfb to 97eb0fcCompare April 9, 2026 22:17

ldk-claude-review-bot reviewed Apr 9, 2026
View reviewed changes

Comment threadlightning/src/ln/channel.rs

ldk-claude-review-bot reviewed Apr 9, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

ldk-claude-review-bot reviewed Apr 9, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

ldk-claude-review-bot reviewed Apr 9, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs

wpaulino force-pushed the funding-contribution-builder branch from 97eb0fc to fefea5cCompare April 10, 2026 00:05

ldk-claude-review-bot reviewed Apr 10, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs

ldk-claude-review-bot reviewed Apr 10, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs

ldk-claude-review-bot reviewed Apr 10, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs

TheBlueMatt reviewed Apr 13, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs
Comment threadlightning/src/ln/funding.rs Outdated
Comment threadlightning/src/ln/funding.rs

wpaulino added 2 commits April 14, 2026 19:51

Derive FundingContribution::net_value implicitly
11610af
This commit removes `FundingContribution::value_added` as tracking it is unnecessary -- it can just be derived from the total amount in minus total amount out minus fees.

wpaulino force-pushed the funding-contribution-builder branch from fefea5c to 33f510cCompare April 15, 2026 02:51

jkczyz reviewed Apr 15, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

wpaulino force-pushed the funding-contribution-builder branch from 33f510c to fa10899Compare April 16, 2026 16:49

wpaulino requested a review from TheBlueMatt April 16, 2026 16:50

jkczyz reviewed Apr 16, 2026
View reviewed changes

TheBlueMatt reviewed Apr 20, 2026
View reviewed changes

wpaulino added 3 commits April 20, 2026 16:34

Introduce FundingBuilder for splice requests
b1c3e29
This lets callers easily amend a prior contribution in place and only re-run coin selection when the new request cannot be satisfied with the existing inputs.

Replace FundingTemplate contribution methods with FundingBuilder
9f9fe58
This results in a slight change of behavior: now these methods reuse and amend the prior contribution, as opposed to always starting from a fresh contribution, which would be the desired expected behavior by users.

wpaulino force-pushed the funding-contribution-builder branch from fa10899 to 9f9fe58Compare April 20, 2026 23:34

ldk-claude-review-bot reviewed Apr 20, 2026
View reviewed changes

jkczyz approved these changes Apr 21, 2026
View reviewed changes

jkczyz merged commit 3278ece into lightningdevkit:mainApr 21, 2026
24 of 25 checks passed

TheBlueMatt reviewed Apr 21, 2026
View reviewed changes

wpaulino deleted the funding-contribution-builder branch April 21, 2026 22:34

	returnOk(FundingContribution::new(
	Ok(FundingContribution::new(

Uh oh!

Conversation

wpaulino commented Mar 27, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

codecovBot commented Mar 27, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Review Summary — PR #4516: Introduce FundingContributionBuilder API

New issue posted this pass

Previously flagged issues (all still present)

Cross-cutting concerns

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Mar 31, 2026

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

wpaulino commented Mar 27, 2026•
edited
Loading

ldk-reviews-bot commented Mar 27, 2026•
edited
Loading

codecovBot commented Mar 27, 2026•
edited
Loading

ldk-claude-review-bot commented Mar 30, 2026•
edited
Loading