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.07%. Comparing base (5704e8e) to head (fa10899).
⚠️ Report is 52 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4516      +/-   ##
==========================================
+ Coverage   86.99%   87.07%   +0.07%     
==========================================
  Files         163      161       -2     
  Lines      108635   109248     +613     
  Branches   108635   109248     +613     
==========================================
+ Hits        94511    95130     +619     
+ Misses      11647    11636      -11     
- Partials     2477     2482       +5

Flag	Coverage Δ
fuzzing	`39.60% <40.00%> (-0.70%)`	⬇️
tests	`86.17% <80.00%> (+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

return Ok(FundingContribution::new(

Ok(FundingContribution::new(

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

Review Summary — PR #4516: Introduce FundingContributionBuilder API (Final Pass)

No new inline comments this pass. The prior review passes were comprehensive and covered all significant issues found in this diff.

Previously flagged issues (all still present)

Critical:

funding.rs:570-578 — Breaking TLV tag renumbering for persisted FundingContribution. Upgrading nodes with pending splices will fail to deserialize channel data.
funding.rs:348 — splice_in/splice_in_sync additive semantics doubles value_added during RBF when a prior contribution is present.

Correctness:
3. funding.rs:647 — Wrong fee estimate used in sufficiency check after dropping change output in amend_without_coin_selection. Uses WITH-change fee instead of no-change fee, making the check too conservative.
4. funding.rs:700-714 — No sufficiency check in the no-change fallthrough of amend_without_coin_selection. Can silently return a contribution with less value_added() than requested.
5. funding.rs:636-644 — amend_without_coin_selection always fails for amended splice-out priors (no inputs → fee check fails).
6. funding.rs:1103-1104 — build_from_prior_contribution failure blocks fresh splice-out fallthrough in try_build_without_coin_selection.
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 to re-coin-selection.

Diagnostic / DX:
11. funding.rs:1067-1073 — All feerate adjustment failures mapped to MissingCoinSelectionSource, losing diagnostic information.
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 MissingCoinSelectionSource error remains overloaded: it covers "need wallet inputs", "feerate can't be accommodated", and "prior amendment failed." The wallet-backed builder silently discards prior inputs on any of these by falling through to fresh coin selection.
The design shift where wallet-backed contributions fund withdrawal outputs from wallet inputs (rather than channel balance) is intentional per the docs, but represents a significant semantic change 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.

TheBlueMatt · 2026-04-08T11:11:46Z

linter, fuzz-sanity, and check_commits are failing.

It does not require coin selection, so the wallet argument is not necessary.

ldk-claude-review-bot · 2026-04-08T17:59:31Z

+			}
+		}
+
+		if self.value_added == Amount::ZERO && self.outputs.is_empty() {


Correctness concern: validate_contribution_parameters blocks reuse of fee-bump-only prior contributions

When a prior contribution has value_added() == 0 and empty outputs (a fee-bump-only contribution that only selected wallet inputs to pay fees), initializing a builder from it yields self.value_added = 0 and self.outputs = []. The validate_contribution_parameters check here then rejects the request with InvalidSpliceValue before build_from_prior_contribution gets a chance to reuse the prior.

This can happen when coin selection returned inputs where inputs - fee - change = 0 exactly. The old rbf code handled this case (via force_coin_selection = true); the new builder path rejects it at validation.

This may be intentional given the removal of the fee-bump-only flow, but it creates an asymmetry: a contribution that was successfully built and stored as a prior can fail to be reused with an identical request, which would be surprising to callers using rbf_prior_contribution or with_prior_contribution.

Fee-bump-only contributions would have the prior contribution's value_added and outputs so this is nonsense.

ldk-claude-review-bot · 2026-04-08T17:59:41Z

+	/// Reuses the contribution's existing inputs while targeting at least `value_added` added to
+	/// the channel after fees. If dropping the change output leaves surplus value, it remains in
+	/// the channel contribution.
+	CoinSelected { value_added: Amount },


Nit: FundingInputs is a single-variant enum, adding indirection without benefit

FundingInputs has only one variant (CoinSelected), yet with_inputs_and_outputs and amend_without_coin_selection match on it. The target_value_added extracted from the match is Option<Amount> but is always Some(...) in practice. This makes the control flow harder to follow without adding extensibility — if new variants are expected soon, a comment noting that would help; otherwise this could be simplified to a plain struct or direct parameter.

We plan to add another variant in a follow-up for manual input support.

TheBlueMatt · 2026-04-08T13:16:26Z

+		let new_fee = new_estimated_fee
+			.to_signed()
+			.expect("total input amount cannot exceed Amount::MAX_MONEY");
+		let new_change = new_change


Does this need to consider dust?

new_change is returned from compute_feerate_adjustment so it should have already been considered there.

I don't think that's quite sufficient? This is also called from net_value_for_acceptor_at_feerate in channel.rs's resolve_queued_contribution. The resulting our_funding_contribution is sent to our peer. Maybe its fine to be a bit off there but it does leave a good chunk of code all the way into channel.rs kinda confusing.

TheBlueMatt · 2026-04-08T19:09:57Z

-					target_feerate,
-				);
+		let net_value_without_fee = self.net_value_without_fee();
+		if net_value_without_fee.is_positive() {


Its weird to split on whether the net contribution was negative or not - if I have a splice where I added 1 BTC+1 sat to the channel but sent 1BTC out without change output, I'm probably more than happy to RBF by using the value of some of my inputs to change the channel's balance.

Good point, but then we're not really abiding by the value_added they provided when we initially coin-selected the inputs. With manual input selection, this will work out of the box since there's no explicit value_added there, we're just always adding whatever is left after fees. Should we treat the coin-selected no-change case the same way?

With manual input selection, this will work out of the box since there's no explicit value_added there, we're just always adding whatever is left after fees.

Hmm? There's no explicit value_added anywhere now? So I suppose you could argue this code is correct for the coin selection case but wrong for the all-input case? Do we need to track the input selection style here?

It's still explicit when splicing in funds via FundingBuilder::add_value. The manual input selection does have a special case in this path to allow spending up to half of the UTXO value (as long as the feerate is still within the max of course).

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.

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.

jkczyz

PR description could probably be completed now.

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.

jkczyz

Mostly LGTM.

jkczyz · 2026-04-16T22:32:37Z

+	/// - **input-backed contributions**: when wallet inputs are selected, those inputs fund both
+	///   the requested value added to the channel and any explicit withdrawal outputs


Hope this isn't too pedantic, bit using "fund" here may be confusing for the withdrawal case. I think of "fund" more as funding a channel rather than how the contribution fees are paid for. It seems a little overloaded here when talking about withdrawal outputs.

Perhaps we can re-word similar to the second bullet where paying for fees and withdrawal outputs is mentioned?

jkczyz · 2026-04-16T22:37:59Z

+	/// them as needed. The withdrawal `outputs` are funded by the selected wallet inputs and do
+	/// not reduce the requested `value_added` to the channel.


Similarly here with "funded".

jkczyz · 2026-04-16T22:38:39Z

+	/// The outputs to include in the funding transaction.
+	///
+	/// When no wallet inputs are contributed, these outputs are paid from the channel balance.
+	/// Otherwise, they are funded by the contributed inputs.


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.

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.

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 thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

jkczyz reviewed Mar 27, 2026

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

wpaulino force-pushed the funding-contribution-builder branch from 0968836 to 2da45ef Compare 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 thread lightning/src/ln/funding.rs Outdated

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

jkczyz reviewed Mar 31, 2026

View reviewed changes

TheBlueMatt reviewed Mar 31, 2026

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs

wpaulino force-pushed the funding-contribution-builder branch from 2da45ef to 19b230b Compare 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 thread lightning/src/ln/funding.rs Outdated

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

View reviewed changes

Comment thread lightning/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 thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

jkczyz reviewed Apr 8, 2026

View reviewed changes

Remove wallet argument from FundingTemplate::splice_out

b4b3bfb

It does not require coin selection, so the wallet argument is not necessary.

wpaulino force-pushed the funding-contribution-builder branch from c11af28 to de4ccfb Compare April 8, 2026 17:51

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

View reviewed changes

TheBlueMatt reviewed Apr 8, 2026

View reviewed changes

jkczyz reviewed Apr 9, 2026

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs

Comment thread lightning/src/ln/splicing_tests.rs

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

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

View reviewed changes

Comment thread lightning/src/ln/channel.rs

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs Outdated

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs

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

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs

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

View reviewed changes

Comment thread lightning/src/ln/funding.rs

TheBlueMatt reviewed Apr 13, 2026

View reviewed changes

Comment thread lightning/src/ln/funding.rs

Comment thread lightning/src/ln/funding.rs Outdated

Comment thread lightning/src/ln/funding.rs

wpaulino added 3 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 33f510c Compare April 15, 2026 02:51

jkczyz reviewed Apr 15, 2026

View reviewed changes

Comment thread lightning/src/ln/channelmanager.rs

wpaulino added 2 commits April 16, 2026 09:49

Introduce FundingBuilder for splice requests

12970b9

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

fa10899

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 33f510c to fa10899 Compare April 16, 2026 16:49

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

jkczyz reviewed Apr 16, 2026

View reviewed changes

	return Ok(FundingContribution::new(
	Ok(FundingContribution::new(

		/// - input-backed contributions: when wallet inputs are selected, those inputs fund both
		/// the requested value added to the channel and any explicit withdrawal outputs

		/// them as needed. The withdrawal `outputs` are funded by the selected wallet inputs and do
		/// not reduce the requested `value_added` to the channel.

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!

codecov bot 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 (Final 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!

TheBlueMatt commented Apr 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

wpaulino commented Mar 27, 2026 •

edited

Loading

ldk-reviews-bot commented Mar 27, 2026 •

edited

Loading

codecov bot commented Mar 27, 2026 •

edited

Loading

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

edited

Loading