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

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

Looking for initial feedback on the design, still needs to be cleaned up a good bit.

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

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

Codecov Report

❌ Patch coverage is 90.15152% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.26%. Comparing base (688544d) to head (0968836).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	90.15%	65 Missing and 13 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4516      +/-   ##
==========================================
+ Coverage   86.14%   86.26%   +0.11%     
==========================================
  Files         160      160              
  Lines      108046   109045     +999     
  Branches   108046   109045     +999     
==========================================
+ Hits        93080    94071     +991     
+ Misses      12346    12332      -14     
- Partials     2620     2642      +22

Flag	Coverage Δ
tests	`86.26% <90.15%> (+0.11%)`	⬆️

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

lightning/src/ln/funding.rs

jkczyz · 2026-03-27T15:13:14Z

lightning/src/ln/funding.rs

+			return Ok(contribution);
+		}
+
+		Err(FundingContributionError::MissingCoinSelectionSource)


Could we have the compiler enforce this by requiring a call to with_coin_selection_source yielding a type with compatible methods? I'd include the convenience methods like splice_in assuming we reuse FundingTemplate instead of adding another type.

Or maybe we just drop the convenience methods?

Discussed offline: we enforce this at compile-time already for add/remove_value, but we cannot do it elsewhere as it's possible that an RBF builder without an attached wallet may require coin selection if the new feerate is too large for the prior contribution's input set.

lightning/src/ln/funding.rs

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

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

lightning/src/ln/funding.rs

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

lightning/src/ln/funding.rs

+			.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(

lightning/src/ln/funding.rs

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

Review Summary

New Issues Found (this pass)

lightning/src/ln/funding.rs:418 — When request matches prior exactly but holder_balance is None, the early return skips amend_without_coin_selection, which could succeed for input-backed contributions without needing holder_balance. This forces unnecessary coin selection.
lightning/src/ln/funding.rs:508 — InvalidSpliceValue error message is unhelpful when a user creates a builder from a fresh template and forgets to call add_value or add_output.

Previously Flagged Issues (still present)

lightning/src/ln/funding.rs:836-841 — splice_in/splice_in_sync additive semantics double value_added during RBF when the template carries a prior contribution. The do_initiate_rbf_splice_in test helper (splicing_tests.rs:252) calls splice_in_sync(value_added, ...) on RBF templates, resulting in prior.value_added + value_added.
lightning/src/ln/funding.rs:618-627 — saturating_add/saturating_sub for bitcoin amounts silently produces nonsensical values on overflow/underflow. checked_add/checked_sub with error propagation would be safer.
lightning/src/ln/funding.rs:661-777 — add_value/remove_value/add_output/remove_outputs methods duplicated across AsyncFundingBuilder and SyncFundingBuilder impl blocks with identical docs and implementations. Could be deduplicated.
lightning/src/ln/funding.rs:727,804 — Unnecessary return keyword in final expression position.
lightning/src/ln/funding.rs:908-920 — rbf_prior_contribution now requires a prior contribution, removing the old fee-bump-only capability without directing users to the builder alternative.

Inline Comments Posted

lightning/src/ln/funding.rs:418 — Missed amend_without_coin_selection opportunity for input-backed prior contributions with holder_balance: None
lightning/src/ln/funding.rs:508 — Unhelpful error message for empty builder requests

lightning/src/ln/funding.rs

jkczyz · 2026-03-30T23:30:11Z

lightning/src/ln/funding.rs

-		let FundingTemplate { shared_input, min_rbf_feerate, .. } = self;
-		build_funding_contribution!(value_added, vec![], shared_input, min_rbf_feerate, min_feerate, max_feerate, false, wallet, await)
+
+		Err(FundingContributionError::MissingCoinSelectionSource)


Can we get this when it should be InvalidSpliceValue? For example, with zero added value and no outputs?

That should have already been checked in validate_contribution_parameters above.

jkczyz · 2026-03-30T23:43:38Z

lightning/src/ln/funding.rs

+		for output in outputs {
+			builder = builder.add_output(output);
 		}


Could we make a private add_outputs that simply moves the input Vec?

jkczyz · 2026-03-30T23:47:35Z

lightning/src/ln/funding.rs


-	/// Creates a [`FundingContribution`] for both adding and removing funds from a channel using
-	/// `wallet` to perform coin selection.
+impl FundingTemplate {


Hmm... I think the diff may be saner if these methods were kept in the original impl block.

The methods haven't moved, rather the diffing logic isn't happy with FundingBuilder being added above FundingTemplate.

jkczyz · 2026-03-31T00:02:45Z

lightning/src/ln/funding.rs

+struct FundingBuilderContents {
+	shared_input: Option<Input>,
+	min_rbf_feerate: Option<FeeRate>,
+	prior_contribution: Option<PriorContribution>,
+	value_added: Amount,
+	outputs: Vec<TxOut>,
+	feerate: FeeRate,
+	max_feerate: FeeRate,
+}


Is this struct necessary? Seems having all this on FundingBuilder could simplify things. Plus, you may be able to simplify error handling if value_added was only for builder types having a CoinSelectionSource. Then value_added could be a field on the parameter type.

Removed FundingBuilderContents. value_added will need to remain here as we plan to add explicit input support.

TheBlueMatt

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

TheBlueMatt · 2026-03-31T14:32:36Z

lightning/src/ln/funding.rs

+}
+
+#[doc(hidden)]
+pub mod builder_states {


Rather than using a generic, it seems like the code would be basically just as clean, but bindings would be happier, if we kept the internal struct and just exposed three separate builder structs.

I think what I pushed should work, let me know otherwise.

lightning/src/ln/funding.rs

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

This commit introduces a `FundingBuilder`, following the builder pattern, to iteratively construct a `FundingContribution`. It supports adding/removing value (which may trigger a coin selection attempt), adding/remove outputs, and amending prior contributions (which was previously not possible). A `FundingBuilder` may be obtained in two ways: via `FundingTemplate::contribution_builder` (which resumes from the prior contribution to amend if available) or `FundingTemplate::without_prior_contribution_builder` to start from scratch and fully replace the prior contribution. When `FundingBuilder::build` is called, we attempt to build the contribution iteratively such that coin selection is our last resort. The splice-related convenience methods on `FundingTemplate` now map to the `FundingBuilder` under the hood.

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

Had to rebase due to a small import conflict.

ldk-claude-review-bot · 2026-03-31T21:20:54Z

lightning/src/ln/funding.rs

+					return Some(adjusted);
+				}
+			}
+			return None;


Bug: When the request matches the prior exactly but holder_balance is None, this early return at line 418 skips amend_without_coin_selection entirely. For input-backed contributions, amend_without_coin_selection does not need holder_balance (lines 1157-1211 don't use it), so it could succeed. This means rbf_prior_contribution_sync(None, max_feerate, wallet) unnecessarily re-runs coin selection for input-backed prior contributions when holder_balance is None — potentially yielding a different input set than the prior had.

This is covered by the test at line 2890 (test_rbf_sync_holder_balance_none_falls_through_to_coin_selection), which expects coin selection, so this appears intentional. But worth noting that the amend_without_coin_selection path could handle this case without requiring a wallet.

ldk-claude-review-bot · 2026-03-31T21:21:27Z

lightning/src/ln/funding.rs

 		}

+		if self.value_added == Amount::ZERO && self.outputs.is_empty() {
+			return Err(FundingContributionError::InvalidSpliceValue);


The validation here catches value_added == 0 && outputs.is_empty(), which blocks the RBF "same request" path for splice-out prior contributions that had value_added == 0 and outputs == []. While this shouldn't arise in normal use (a prior contribution with both zero is meaningless), there's a subtle interaction: if someone calls with_prior_contribution(...) on a fresh template (no prior) without calling any mutation methods, they'd hit this error. The error message "Invalid splice value" doesn't hint that they need to call add_value or add_output first. Consider a more descriptive variant like EmptyContribution or adding a hint to the InvalidSpliceValue display message.

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

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

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

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

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

jkczyz reviewed Mar 27, 2026

View reviewed changes

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