Support manually selecting inputs consuming their entire value by wpaulino · Pull Request #4575 · lightningdevkit/rust-lightning

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino · 2026-04-23T17:08:48Z

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain.

To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

ldk-reviews-bot · 2026-04-23T17:08:52Z

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

ldk-claude-review-bot · 2026-04-23T17:16:47Z

 );

- if !self.inputs.is_empty() {
+ if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) {


Bug: Backwards compatibility issue with persisted FundingContribution objects.
FundingContribution is persisted in PendingFunding.contributions (channel.rs:2917). When deserializing contributions created before this PR, input_mode will be None (it's a new option TLV field). Old coin-selected contributions with inputs will have input_mode == None, causing this condition to be false even though they should take the coin-selected branch.
This causes two problems for old persisted coin-selected contributions:
Wrong fee buffer calculation: Uses holder_balance + net_value_without_fee instead of estimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly.
Change output silently dropped: compute_feerate_adjustment returns None for change, and at_feerate sets change_output = None, losing the change value.
This is reachable via for_acceptor_at_feerate / for_initiator_at_feerate called on contributions loaded from pending_splice.contributions (channel.rs lines 12504, 12944, 13127, 13145).
Fix: use self.input_mode != Some(FundingInputMode::Manual) instead of self.input_mode == Some(FundingInputMode::CoinSelected) to preserve old behavior for contributions where input_mode is None:
Suggested change
if !self.inputs.is_empty() && self.input_mode==Some(FundingInputMode::CoinSelected){
if !self.inputs.is_empty() && self.input_mode!=Some(FundingInputMode::Manual){

There is no backwards compatibility concern because the serialized object has not been included in a release yet.

ldk-claude-review-bot · 2026-04-23T17:17:33Z

+ if let Some(PriorContribution { contribution: prior_contribution, .. }) =
+ self.prior_contribution.as_ref()
+ {
+ if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected)


Minor: Same backwards-compat pattern as line 880. Old persisted coin-selected contributions will have input_mode == None, so this guard won't fire for them. In practice this is mostly mitigated by the check at line 1309 (value_added > 0 && manually_selected_inputs non-empty), but it could miss edge cases where the old prior had value_added() == 0 (inputs exactly covered outputs + fees).
Consider using != Some(FundingInputMode::Manual) combined with a non-empty inputs check:
Suggested change
if prior_contribution.input_mode==Some(FundingInputMode::CoinSelected)
if prior_contribution.input_mode!=Some(FundingInputMode::Manual)
&& !prior_contribution.inputs.is_empty()

ldk-claude-review-bot · 2026-04-23T17:18:44Z

I've completed a thorough review of the entire PR diff and verified the current code state. All previously flagged issues have been checked, and I've searched for any new issues.

Review Summary

Previously Flagged Issues — Status

FIXED — lightning/src/ln/funding.rs — .take() destroying funding_inputs on coin selection fallback path is now .clone() at line 1229.
Still open — lightning/src/ln/funding.rs:933 — Backwards-compat: old persisted coin-selected contributions have input_mode == None, causing them to take the manual-inputs fee buffer branch instead of the coin-selected branch in compute_feerate_adjustment. This drops the change output and miscalculates the fee buffer.
Still open — lightning/src/ln/funding.rs:1401-1412 — Backwards-compat: old coin-selected priors with input_mode == None get funding_inputs = None in builder init, silently discarding their inputs when the request doesn't match and amend_without_coin_selection is called.
Still open — lightning/src/ln/funding.rs:1375-1381 — Duplicate-output validation (by script_pubkey) shadows MAX_MONEY sum tests since funding_output_sats() always produces the same script_pubkey.

No New Issues Found

The new code is otherwise correct. Specifically verified:

FundingInputs/FundingInputMode enums and their serialization
Builder add_input/add_inputs/remove_input methods correctly enforce mode exclusivity
splice_in_inputs convenience method correctly appends to prior manual inputs
ManuallySelectedInputsInsufficient error correctly does NOT fall through to coin selection in both sync and async builders
Fee buffer calculation for manual inputs in compute_feerate_adjustment is correct for positive and negative net_value_without_fee
validate_inputs duplicate detection covers the combined set (prior + new) of manually selected inputs
validate_contribution_parameters runs before build_from_prior_contribution, catching duplicates and MAX_MONEY violations
request_matches_prior correctly compares outpoints for manual inputs and value for coin-selected inputs
net_value_without_fee and net_value_at_feerate arithmetic is sound for manual inputs
Spliceable balance check in try_build_without_coin_selection correctly gates net-negative manual contributions

codecov · 2026-04-23T18:30:17Z

Codecov Report

❌ Patch coverage is 96.14874% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (f408b17) to head (dcca939).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	96.07%	15 Missing and 14 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4575 +/- ##
==========================================
+ Coverage 86.15% 86.58% +0.43% 
==========================================
Files 157 159 +2 Lines 109096 110419 +1323 Branches 109096 110419 +1323 ==========================================
+ Hits 93989 95611 +1622 + Misses 12485 12275 -210 + Partials 2622 2533 -89

Flag	Coverage Δ
fuzzing-fake-hashes	`6.57% <0.00%> (?)`
fuzzing-real-hashes	`23.17% <3.67%> (?)`
tests	`86.23% <96.14%> (+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.

Alkamal01 · 2026-04-23T18:41:13Z

The root problem seems to be relying on == Some(CoinSelected) for persisted data. For legacy contributions, None effectively behaved like coin selection when inputs were present, so switching to != Some(Manual) (with an inputs non-empty check where needed) would preserve the old behavior.

One additional edge case in request_matches_prior: (Some(CoinSelected), None) currently falls into _ => false, so an RBF with the same value on a legacy prior won't be treated as a match and skips the feerate-adjustment path. Not strictly a correctness issue, but it becomes inconsistent if the other branches get fixed.

So overall, it'd be good to make the handling of None consistent across all three paths.

TheBlueMatt

All LGTM. One question.

ldk-reviews-bot · 2026-04-27T00:00:09Z

🔔 1st Reminder

Hey @jkczyz! 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-29T00:01:00Z

🔔 2nd Reminder

Hey @jkczyz! 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

thanks

elnosh

Not that familiar with splicing-related code in the codebase but looking at this for learning purposes. After much staring, changes make sense although agree with @jkczyz comment on making an enum since those fields (value_added and manually_selected_inputs) are mutually exclusive in the ways inputs can be provided.

elnosh · 2026-04-30T22:25:07Z

+/// used if the request cannot be satisfied by reusing a prior contribution, by using only
+/// manually selected inputs, or by building a pure splice-out directly.


I'm not sure I follow this comment. If we have a builder with either sync or async CoinSelectionSource then manually added inputs are not allowed.

Manually selected inputs are still allowed as long as the added_value is 0.

TheBlueMatt · 2026-05-07T21:31:01Z

Needs rebase again :/

ldk-claude-review-bot · 2026-05-13T18:47:37Z

+ for (idx, output) in self.outputs.iter().enumerate() {
+ if self.outputs[..idx]
+ .iter()
+ .any(|existing_output| existing_output.script_pubkey == output.script_pubkey)
+ {
+ return Err(FundingContributionError::InvalidSpliceValue);


Nit / test gap: this new duplicate-output check fires before the value_removed > MAX_MONEY accumulation below. Since funding_output_sats() always returns the same script_pubkey (new_p2wpkh(&WPubkeyHash::all_zeros())), the existing tests test_build_funding_contribution_validates_max_money ("splice_out with multiple outputs summing > MAX_MONEY", lines 2723-2734) and test_funding_builder_validates_mixed_request_max_money ("outputs summing > MAX_MONEY", lines 2755-2769) now hit this duplicate check first. They still pass because both paths yield InvalidSpliceValue, but the MAX_MONEY sum-of-outputs path has lost test coverage.
Consider using distinct script_pubkey values for each output in those tests.

TheBlueMatt · 2026-05-17T23:02:22Z

+ let value_added =
+ inner.funding_inputs.as_ref().map_or(Amount::ZERO, FundingInputs::value_added);
+
 match inner.build_without_coin_selection() {


Its quite awkward that we call this here and it takes several fields from inner internally but then we reuse inner a few lines down as well as later. It probably makes sense to pay the penalty of either calling prepare_coin_selection_request upfront so we can make build_without_coin_selection take mut self without ref or cloneing internally to avoid ripping out parts of inner before we reuse it. Seems like too much opportunity for stuff to go wrong later.

Alright just decided to clone internally

Sorry, this wasn't the only place, we also take the prior_contribution in try_build_without_coin_selection and mutate inner that way too.

This commit introduces an alternative way of splicing in funds without coin selection by requiring the full UTXO to be provided. Each UTXO's entire value (minus fees) is allocated towards the channel, which provides unified balance wallets a more intuitive API when splicing funds into the channel, as they don't particularly care about maintaining a portion of their balance onchain. To simplify the implementation, we require that contributions are not allowed to mix coin-selected inputs with manually-selected ones. Users will need to start a fresh contribution if they want to change the funding input mode.

There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino added this to the 0.3 milestone Apr 23, 2026

wpaulino requested review from TheBlueMatt and jkczyz April 23, 2026 17:08

wpaulino self-assigned this Apr 23, 2026

wpaulino marked this pull request as ready for review April 23, 2026 17:08

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

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch from 12e5994 to 412ec3dCompare April 23, 2026 17:19

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch from 412ec3d to 228cae8Compare April 28, 2026 22:00

wpaulino requested a review from TheBlueMatt April 29, 2026 16:58

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

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

elnosh reviewed Apr 30, 2026
View reviewed changes

wpaulino dismissed TheBlueMatt’s stale review via 17c864bMay 5, 2026 23:36

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch 3 times, most recently from c3da7fe to 6e16493Compare May 6, 2026 19:27

ldk-claude-review-bot reviewed May 6, 2026
View reviewed changes

Comment threadlightning/src/ln/funding.rs Outdated

wpaulino mentioned this pull request May 8, 2026
Drop QuiescentError::with_negotiation_failure_reason#4608
Merged

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch from 6e16493 to c240f1bCompare May 13, 2026 18:40

ldk-claude-review-bot reviewed May 13, 2026
View reviewed changes

TheBlueMatt previously approved these changes May 17, 2026
View reviewed changes

wpaulino dismissed TheBlueMatt’s stale review via ed757b2May 18, 2026 20:13

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch from c240f1b to ed757b2Compare May 18, 2026 20:13

wpaulino requested review from TheBlueMatt and jkczyz May 18, 2026 20:14

TheBlueMatt removed their request for review May 18, 2026 20:37

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch from ed757b2 to a9fa610Compare May 18, 2026 22:04

wpaulino added 3 commits May 19, 2026 10:58

Include spliceable balance in every FundingTemplate
740ebb1
There's no reason not to do so, and it allows us to fail earlier when the user's net contribution exceeds their spliceable balance.

Detect duplicate inputs and outputs upon building FundingBuilder
dcca939
While this is already enforced when we get to the interactive negotiation phase, we choose to fail early anyway.

wpaulino force-pushed the funding-contribution-builder-manual-inputs branch from a9fa610 to dcca939Compare May 19, 2026 18:00

wpaulino requested a review from TheBlueMatt May 19, 2026 18:00

TheBlueMatt approved these changes May 19, 2026
View reviewed changes

jkczyz approved these changes May 19, 2026
View reviewed changes

jkczyz merged commit e47a231 into lightningdevkit:mainMay 19, 2026
24 checks passed

wpaulino deleted the funding-contribution-builder-manual-inputs branch May 19, 2026 20:08