Support manually selecting inputs consuming their entire value#4575
Support manually selecting inputs consuming their entire value#4575wpaulino wants to merge 3 commits into
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| ); | ||
|
|
||
| if !self.inputs.is_empty() { | ||
| if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) { |
There was a problem hiding this comment.
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_feeinstead ofestimated_fee + change_value, potentially allowing or rejecting feerate adjustments incorrectly. - Change output silently dropped:
compute_feerate_adjustmentreturnsNonefor change, andat_feeratesetschange_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:
| if !self.inputs.is_empty() && self.input_mode == Some(FundingInputMode::CoinSelected) { | |
| if !self.inputs.is_empty() && self.input_mode != Some(FundingInputMode::Manual) { |
There was a problem hiding this comment.
There is no backwards compatibility concern because the serialized object has not been included in a release yet.
| if let Some(PriorContribution { contribution: prior_contribution, .. }) = | ||
| self.prior_contribution.as_ref() | ||
| { | ||
| if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected) |
There was a problem hiding this comment.
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:
| if prior_contribution.input_mode == Some(FundingInputMode::CoinSelected) | |
| if prior_contribution.input_mode != Some(FundingInputMode::Manual) | |
| && !prior_contribution.inputs.is_empty() |
Review SummaryNew Issue
Previously Flagged Issues (still open)All three issues from the prior review are still present and unresolved:
Inline Comment Posted
|
wpaulino
force-pushed
the
funding-contribution-builder-manual-inputs
branch
from
12e5994 to
412ec3d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4575 +/- ##
==========================================
+ Coverage 86.12% 86.18% +0.05%
==========================================
Files 157 157
Lines 108922 109538 +616
Branches 108922 109538 +616
==========================================
+ Hits 93812 94403 +591
- Misses 12495 12501 +6
- Partials 2615 2634 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
All LGTM. One question.
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
wpaulino
force-pushed
the
funding-contribution-builder-manual-inputs
branch
from
412ec3d to
228cae8
Compare
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
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.
| /// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Manually selected inputs are still allowed as long as the added_value is 0.
wpaulino
force-pushed
the
funding-contribution-builder-manual-inputs
branch
3 times, most recently
from
c3da7fe to
6e16493
Compare
|
Needs rebase again :/ |
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
force-pushed
the
funding-contribution-builder-manual-inputs
branch
from
6e16493 to
c240f1b
Compare
| 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); |
There was a problem hiding this comment.
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.
6 participants
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.