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 @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 (de4ccfb).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4516      +/-   ##
==========================================
+ Coverage   86.99%   87.08%   +0.08%     
==========================================
  Files         163      163              
  Lines      108635   108936     +301     
  Branches   108635   108936     +301     
==========================================
+ Hits        94511    94869     +358     
+ Misses      11647    11587      -60     
- Partials     2477     2480       +3

Flag	Coverage Δ
fuzzing	`40.50% <73.91%> (+0.19%)`	⬆️
tests	`86.16% <100.00%> (+0.07%)`	⬆️

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

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 — PR #4516: Introduce FundingContributionBuilder API

Correction from prior review

My prior comment #2 ("with_inputs_and_outputs uses wrong fee estimate when checking sufficiency after dropping change") was a false positive. On re-examination, the fallthrough _ => arm at line 656 correctly uses estimated_fee_no_change, not estimated_fee. The code handles the change-dropped case properly.

New issues (this pass)

lightning/src/ln/funding.rs:1153 — validate_contribution_parameters rejects value_added == 0 && outputs.is_empty() before build_from_prior_contribution can reuse a fee-bump-only prior contribution, creating an asymmetry where a previously valid contribution can't be reused with an identical request.
lightning/src/ln/funding.rs:516 — FundingInputs single-variant enum adds unnecessary indirection.

Previously flagged issues (still present)

lightning/src/ln/funding.rs:546-554 — Breaking serialization: TLV tags renumbered for a persisted type (FundingContribution is stored in PendingFunding.contributions).
lightning/src/ln/funding.rs:777 — splice_in/splice_in_sync additive semantics double value_added during RBF when the template carries a prior contribution.
lightning/src/ln/funding.rs:584 — saturating_add/saturating_sub for bitcoin amounts silently produces nonsensical values.
lightning/src/ln/funding.rs:655 — add_value/remove_value/add_output/remove_outputs methods duplicated across AsyncFundingBuilder and SyncFundingBuilder.
lightning/src/ln/funding.rs:682,740 — Unnecessary return keyword in final expression position.
lightning/src/ln/funding.rs:856 — rbf_prior_contribution now requires a prior contribution, removing old fee-bump-only capability.
lightning/src/ln/funding.rs:418 — Missed amend_without_coin_selection opportunity for input-backed prior contributions with holder_balance: None.

Cross-cutting concerns

The validate() method was removed from FundingContribution and replaced with validate_inputs() (called only after coin selection in the builder). Contributions reused from prior state or deserialized from disk no longer undergo input validation at the channel level. This relies on the assumption that previously validated inputs remain valid, which should hold in practice but reduces defense-in-depth.
The PR description says "still needs to be cleaned up a good bit" — the serialization break (try! instead of ? #1 above) and the additive semantics footgun (Fix typos #2) should be addressed before merge.

lightning/src/ln/funding.rs

TheBlueMatt

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

lightning/src/ln/funding.rs

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

Had to rebase due to a small import conflict.

lightning/src/ln/funding.rs

TheBlueMatt

needs rebase again tho

jkczyz

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

lightning/src/ln/funding.rs

ldk-claude-review-bot · 2026-04-08T00:27:04Z

lightning/src/ln/funding.rs

 impl_writeable_tlv_based!(FundingContribution, {
-	(1, value_added, required),
-	(3, estimated_fee, required),
-	(5, inputs, optional_vec),
-	(7, outputs, optional_vec),
-	(9, change_output, option),
-	(11, feerate, required),
-	(13, max_feerate, required),
-	(15, is_splice, required),
+	(1, estimated_fee, required),
+	(3, inputs, optional_vec),
+	(5, outputs, optional_vec),
+	(7, change_output, option),
+	(9, feerate, required),
+	(11, max_feerate, required),
+	(13, is_splice, required),
 });


Breaking serialization: TLV tags renumbered for a persisted type

FundingContribution is serialized as part of PendingFunding.contributions (channel.rs:2926), which is persisted in FundedChannel (channel.rs:15814, tag 64). Renumbering the TLV tags means any previously serialized FundingContribution will fail to deserialize:

Old tag 1 = value_added (Amount), new tag 1 = estimated_fee (Amount) — same type, wrong semantics

Old tag 3 = estimated_fee (Amount), new tag 3 = inputs (Vec) — different types, will fail

If a node with a pending splice upgrades, it will be unable to read its channel data.

Options:

Keep the old tag numbering and make value_added optional (defaulting to derived) for backward compatibility

Use new, higher tag numbers (e.g., start at 17+) so old fields are simply ignored by the new code

If splicing serialization is considered unstable/unreleased, document this as a known breaking change

This serialization has not been included in a release so it should be safe to break.

lightning/src/ln/funding.rs

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

linter, fuzz-sanity, and check_commits are failing.

jkczyz · 2026-04-08T15:06:57Z

lightning/src/ln/funding.rs

+				holder_balance.ok_or(FeeRateAdjustmentError::FeeBufferInsufficient {
+					source: "channel balance",
+					available: Amount::ZERO,
+					required: target_fee,
+				})?;


Could you add a test for this error path?

Hmm... this comment was made on the second commit, but it shows up in a weird place on the third commit.

Added a unit test since a functional test would be pretty hard to discern. It's also quite the edge case, you shouldn't have a net-negative contribution in the first place if you didn't have a balance, so either the contribution is stale/invalid or the balance is.

jkczyz · 2026-04-08T15:12:44Z

lightning/src/ln/funding.rs

+	/// The value that will be added to the channel after fees. See [`Self::net_value`] for the net
+	/// value contribution to the channel.
+	fn value_added(&self) -> Amount {
+		let total_input_value = self.inputs.iter().map(|i| i.utxo.output.value).sum::<Amount>();


Should we check if this overflows?

We check it when we select the inputs.

Where is that? I don't see it checked after calling select_confirmed_utxo. Maybe we shouldn't care if the user's CoinSelectionSource is broken?

Ah somehow this part didn't make it into this commit and was in my follow-up for manual input support. Should be good now.

jkczyz · 2026-04-08T15:31:32Z

lightning/src/ln/funding.rs

 				return Err(FeeRateAdjustmentError::FeeBufferInsufficient {
-					source: "channel balance - withdrawal outputs",
-					available: holder_balance.checked_sub(value_removed).unwrap_or(Amount::ZERO),
+					source: "channel balance",


Isn't the previous wording correct?

Only when there aren't any inputs because there may be some that are partially contributing to the net-negative contribution.

lightning/src/ln/funding.rs

jkczyz · 2026-04-08T15:38:39Z

lightning/src/ln/funding.rs

 			.to_signed()
-			.expect("value_added is validated to not exceed Amount::MAX_MONEY");
-		let value_removed = self
+			.expect("total_input_value is validated to not exceed Amount::MAX_MONEY");


I believe the validation was removed?

Everything should be in validate_contribution_parameters, inputs are validated separately when we do coin selection.

I meant can't the sum overflow? We avoided the problem earlier with the explicit value_added. But maybe it was already a problem elsewhere?

jkczyz · 2026-04-08T15:39:04Z

lightning/src/ln/funding.rs

+		let estimated_fee = self
+			.estimated_fee
+			.to_signed()
+			.expect("total_input_value is validated to not exceed Amount::MAX_MONEY");


lightning/src/ln/funding.rs

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

This simplifies the callsites such that we no longer need to check for presense of the variable, and will also become useful when we want to adjust prior contributions regardless of whether we had a non-zero holder balance.

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. Doing so also highlighted that there was an incorrect assumption in how feerates are computed/adjusted. Ultimately, we don't care whether a contribution has inputs or not. Instead, we look at whether it's contributing a positive or negative amount to the channel, and enforce different constraints for each.

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-08T17:59:31Z

lightning/src/ln/funding.rs

+			}
+		}
+
+		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

lightning/src/ln/funding.rs

+	/// 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

lightning/src/ln/funding.rs

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

TheBlueMatt · 2026-04-08T18:02:44Z

lightning/src/ln/funding.rs

 	/// Returns `Err` if the contribution cannot accommodate the target feerate.
 	fn compute_feerate_adjustment(
-		&self, target_feerate: FeeRate, holder_balance: Amount, is_initiator: bool,
+		&self, target_feerate: FeeRate, holder_balance: Option<Amount>, is_initiator: bool,


Huh? We always have a holder_balance, there's just cases where we failed to compute one but that is a "this mostly shouldn't happen" case and should just return an error anyway because our channel is kinda borked. We shouldn't convert everything to an option.

Happy to drop the commit, but then we probably should not allow splice_channel to return a FundingTemplate when we can't compute the balance so that PriorContribution::holder_balance is not an option.

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

lightning/src/ln/funding.rs

-					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?

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

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

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

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

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

View reviewed changes

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

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

View reviewed changes

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

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

View reviewed changes

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

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

View reviewed changes

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

jkczyz reviewed Mar 31, 2026

View reviewed changes

TheBlueMatt reviewed Mar 31, 2026

View reviewed changes

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

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

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

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

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

View reviewed changes

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

TheBlueMatt previously approved these changes Apr 1, 2026

View reviewed changes

jkczyz reviewed Apr 2, 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

wpaulino dismissed TheBlueMatt’s stale review via c11af28 April 8, 2026 00:21

wpaulino force-pushed the funding-contribution-builder branch from 19b230b to c11af28 Compare April 8, 2026 00:21

wpaulino requested review from TheBlueMatt and jkczyz April 8, 2026 00:22

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

View reviewed changes

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

jkczyz reviewed Apr 8, 2026

View reviewed changes

wpaulino added 5 commits April 8, 2026 10:50

Remove wallet argument from FundingTemplate::splice_out

b4b3bfb

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

Introduce FundingBuilder for splice requests

231d555

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

de4ccfb

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

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

Conversation

wpaulino commented Mar 27, 2026

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

Correction from prior review

New issues (this pass)

Previously flagged issues (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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Apr 8, 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!

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