Contribute to splice as acceptor#4416
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
fc3e1da to
758ab0a
Compare
b6cec12 to
5a26025
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4416 +/- ##
==========================================
+ Coverage 85.94% 86.02% +0.07%
==========================================
Files 159 159
Lines 104607 104758 +151
Branches 104607 104758 +151
==========================================
+ Hits 89901 90113 +212
+ Misses 12194 12139 -55
+ Partials 2512 2506 -6
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:
|
5a26025 to
658f332
Compare
Now that the Splice variant (containing non-serializable FundingContribution) is the only variant produced, and the previous commit consumes the acceptor's quiescent_action in splice_init(), there is no longer a need to persist it. This allows removing LegacySplice, SpliceInstructions, ChangeStrategy, and related code paths including calculate_change_output, calculate_change_output_value, and the legacy send_splice_init method. With ChangeStrategy removed, the only remaining path in calculate_change_output was FromCoinSelection which always returned Ok(None), making it dead code. The into_interactive_tx_constructor method is simplified accordingly, and the signer_provider parameter is removed from it and from splice_init/splice_ack since it was only needed for the removed change output calculation. On deserialization, quiescent_action (TLV 65) is still read for backwards compatibility but discarded, and the awaiting_quiescence channel state flag is cleared since it cannot be acted upon without a quiescent_action. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
658f332 to
4274f75
Compare
a1fe138 to
e9c5791
Compare
lightning/src/ln/channel.rs
Outdated
| {1, LegacySplice} => (), | ||
| ); | ||
| #[cfg(not(any(test, fuzzing, feature = "_test_utils")))] | ||
| impl_writeable_tlv_based_enum_upgradable!(QuiescentAction, |
There was a problem hiding this comment.
Shouldn't this be removed?
There was a problem hiding this comment.
Yeah, no longer needed since we don't need to read an odd TLV to ignore.
lightning/src/ln/channel.rs
Outdated
| )) | ||
| })?; | ||
| ); | ||
| let tx_msg_opt = interactive_tx_constructor.take_initiator_first_message(); |
There was a problem hiding this comment.
Let's return this as part of the constructor now and drop take_initiator_first_message?
There was a problem hiding this comment.
Done though note that InteractiveTxConstructorArgs::new_for_outbound still needs to return an Option for it since we have a debug_assert there.
| let splice_funding = self.validate_splice_init(msg, our_funding_contribution)?; | ||
| let feerate = FeeRate::from_sat_per_kwu(msg.funding_feerate_per_kw as u64); | ||
| let our_funding_contribution = self.queued_funding_contribution().and_then(|c| { | ||
| c.net_value_for_acceptor_at_feerate(feerate) |
There was a problem hiding this comment.
We probably shouldn't blindly accept the counterparty's feerate, but asking the user to approve it isn't ideal either.
There was a problem hiding this comment.
Yeah, I wonder if we should have the user pass a max fee rate when building / passing the FundingContribution?
There was a problem hiding this comment.
FundingTemplate now has both min_feerate and max_feerate, the former used for coin selection and the latter carried over to FundingContribution and checked in net_value_for_acceptor_at_feerate.
| /// - `(fee, Some(change))` — inputs with change: both should be updated | ||
| /// - `(fee, None)` — inputs without change (or change removed), or splice-out: fee updated | ||
| /// only | ||
| fn compute_feerate_adjustment( |
There was a problem hiding this comment.
Docs should probably mention this is only intended to be used in the non-initiator contribution case
| is_splice, | ||
| target_feerate, | ||
| ); | ||
| if budget < fair_fee { |
There was a problem hiding this comment.
If we have a mixed splice where we fully consume an input and cover the remaining outstanding amount with the channel balance, then budget should probably also reflect the channel balance available to spend?
There was a problem hiding this comment.
As of now, we've only allowed drawing from the channel balance in the splice-out case. Mixed mode will only use inputs.
Outsourcing this idea to Claude, it would be possible but has some implications. We can discuss but I'd rather leave this to a follow-up as it would change the current behavior.
❯ What are the implications of allowing the fees to be absorbed by the
channel balance in this case? Is it possible to do? What code changes
would be involved?
⏺ Let me trace how net_value flows through the protocol to understand the
implications.
⏺ Searched for 2 patterns (ctrl+o to expand)
⏺ Now let me look at how our_funding_contribution is used in
validate_splice_contributions and the interactive TX setup.
⏺ Searched for 1 pattern (ctrl+o to expand)
⏺ Now let me see how our_funding_contribution flows into
FundingScope::for_splice and how it affects the shared output value.
⏺ Searched for 1 pattern (ctrl+o to expand)
⏺ This is the key piece. our_funding_contribution (which is net_value())
directly determines:
1. post_channel_value — the new total channel capacity
2. post_value_to_self_msat — our balance in the new channel (line 2794-2796:
value_to_self_msat + our_funding_contribution * 1000)
Now let me check how the acceptor path uses net_value_for_acceptor_at_feerate
vs net_value to understand the current split.
⏺ Read 1 file (ctrl+o to expand)
⏺ Here's what I've found:
How net_value is used: our_funding_contribution (from net_value() or
net_value_for_acceptor_at_feerate()) flows into FundingScope::for_splice where
it directly determines:
- post_channel_value — the new channel capacity
- value_to_self_msat — our balance (line 2796: prev_value_to_self +
our_funding_contribution * 1000)
Current behavior for with-inputs case: net_value_with_fee returns value_added
- value_removed regardless of estimated_fee, because unpaid_fees = 0 when
inputs are present. So our_funding_contribution is fixed — feerate changes
don't affect it.
Current behavior for no-inputs case (pure splice-out): net_value_with_fee
returns -value_removed - estimated_fee. The fee IS deducted from channel
balance. And compute_feerate_adjustment in the no-inputs branch (line 504-521)
freely allows the surplus to go back to channel balance — there's no strict
budget constraint, just a check that estimated_fee >= fair_fee.
The question for mixed mode: Could we let the channel balance absorb fee
increases when inputs can't cover them?
Yes, it's possible, but here's what it would mean:
1. net_value would become feerate-dependent even with inputs. Instead of
value_added - value_removed, it would be something like value_added -
value_removed - unpaid_fee_delta. This changes our_funding_contribution, which
changes both the channel capacity and our balance.
2. The user's intended contribution silently changes. If someone asks for
splice_in_and_out(10k, [80k output]), they expect net = -70k. If we absorb 500
sats of extra fee from channel balance, net becomes -70,500. The user's
channel balance decreases by an extra 500 sats beyond what they requested.
3. Code changes required:
- compute_feerate_adjustment would need a new branch (or extension of the
inputs branch) that, when the inputs budget is insufficient, computes how much
extra fee the channel balance can absorb
- net_value_with_fee would need to change: instead of unpaid_fees = 0 when
inputs are present, it would need to compute the portion of fees not covered
by inputs
- for_acceptor_at_feerate would need to propagate this adjusted net value
- net_value_for_acceptor_at_feerate already uses
net_value_with_fee(new_estimated_fee) — this would need to account for the
split between input-paid and balance-paid fees
- Tests asserting net_value is stable across feerate changes for splice-in
would need updating
4. Protocol implication: The splice_ack.funding_contribution_satoshis message
communicates our net contribution. If our contribution changes due to the
initiator's feerate, the value we send in splice_ack must reflect the adjusted
amount. Currently splice_init computes the adjusted our_funding_contribution
via net_value_for_acceptor_at_feerate before sending splice_ack, so the
plumbing exists — it just returns the same value today for the with-inputs
case.
Bottom line: It's technically feasible but it changes the semantics of the
user's contribution. The current behavior (fail if inputs can't cover the fee
increase) is arguably more correct — it tells the user "your contribution
can't accommodate this feerate" rather than silently taking more from their
channel balance. The pure splice-out case is different because there, fees
from channel balance are the only fee mechanism, so flexibility is expected.
Does this answer your question, or are you considering making a change here?
✻ Baked for 49s
lightning/src/ln/channel.rs
Outdated
|
|
||
| let mut pending_splice: Option<PendingFunding> = None; | ||
| let mut quiescent_action = None; | ||
| let mut _quiescent_action: Option<QuiescentAction> = None; |
There was a problem hiding this comment.
You can just remove this entirely and not even mention it as its an odd TLV.
| .ok_or("Budget plus change value overflow".to_string())?; | ||
|
|
||
| match available.checked_sub(fair_fee) { | ||
| Some(new_change_value) if new_change_value >= dust_limit => { |
There was a problem hiding this comment.
In both the no-input and no-change case we fail if we'd pay a fee higher than our initially-estimated fee, but here we'll happily continue even if we'd end up reducing change or even removing it. ISTM we should maybe fail instead, as we'd maybe prefer to do our own splice with our own feerate on top of the counterparty's splice later or even splice on another channel instead instead of paying more in fees. Its complicated though, for someone with only a single channel who wants to pay immediately they'd prefer it to work, but in that setup there's a chance that the counterparty just beats them to the punch anyway, so maybe its "something they have to deal with either way" and its fine?
There was a problem hiding this comment.
Right, related to my response to one of @wpaulino's comments, perhaps the user needs to indicate a max fee rate for these scenarios? Otherwise, it seems we'd never contribute as an acceptor.
There was a problem hiding this comment.
Also, note that if the user-specified min/max fee rate is tight, we could reject if the counterparty gives a fee rate slightly above the max even though we may pay less fees since we wouldn't be paying for common fields and shared input / output. So that would need to be considered.
Replace the single public InteractiveTxConstructor::new() with separate new_for_outbound() and new_for_inbound() constructors. This moves the initiator's first message preparation out of the core constructor, making it infallible and removing is_initiator from the args struct. Callers no longer need to handle constructor errors, which avoids having to generate SpliceFailed/DiscardFunding events after the QuiescentAction has already been consumed during splice_init/splice_ack handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When constructing a FundingContribution, it's always assumed the estimated_fee is for when used as the initiator, who pays for the common fields and shared inputs / outputs. However, when the contribution is used as the acceptor, we'd be overpaying fees. Provide a method on FundingContribution that adjusts the fees and the change output, if possible.
e9c5791 to
f878905
Compare
Add FeeRateAdjustmentError enum with TooLow, TooHigh, and BudgetInsufficient variants to distinguish different feerate incompatibility scenarios. Add max_feerate field to FundingTemplate and FundingContribution. Update compute_feerate_adjustment to check feerate bounds: - Target below min_feerate (coin selection rate) -> TooLow - Target above max_feerate with fair_fee > estimated_fee -> TooHigh - Target above max_feerate but fair_fee <= estimated_fee -> allowed (change output not consumed, acceptor pays less than as initiator) - Budget insufficient within acceptable range -> BudgetInsufficient Change splice_channel and rbf_channel signatures from feerate -> (min_feerate, max_feerate). Coin selection uses min_feerate; as initiator, min_feerate is proposed in splice_init. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a `change_output: Option<&TxOut>` parameter to `estimate_transaction_fee` so the initial fee estimate accounts for the change output's weight. Previously, the change output weight was omitted from `estimated_fee` in `FundingContribution`, causing the estimate to be slightly too low when a change output was present. This also eliminates an unnecessary `Vec<TxOut>` allocation in `compute_feerate_adjustment`, which previously cloned outputs into a temporary Vec just to include the change output for the fee estimate. A mock `TightBudgetWallet` is added to `splicing_tests` to demonstrate that `validate()` correctly rejects contributions where the input value is sufficient without the change output weight but insufficient with it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When both nodes want to splice simultaneously, the quiescence tie-breaker designates one as the initiator. Previously, the losing node responded with zero contribution, requiring a second full splice session after the first splice locked. This is wasteful, especially for often-offline nodes that may connect and immediately want to splice. Instead, the losing node contributes to the winner's splice as the acceptor, merging both contributions into a single splice transaction. Since the FundingContribution was originally built with initiator fees (which include common fields and shared input/output weight), the fee is adjusted to the acceptor rate before contributing, with the surplus returned to the change output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add FeeRateAdjustmentError enum with TooLow, TooHigh, and BudgetInsufficient variants to distinguish different feerate incompatibility scenarios. Add max_feerate field to FundingTemplate and FundingContribution. Update compute_feerate_adjustment to check feerate bounds: - Target below min_feerate (coin selection rate) -> TooLow - Target above max_feerate with fair_fee > estimated_fee -> TooHigh - Target above max_feerate but fair_fee <= estimated_fee -> allowed (change output not consumed, acceptor pays less than as initiator) - Budget insufficient within acceptable range -> BudgetInsufficient Change splice_channel and rbf_channel signatures from feerate -> (min_feerate, max_feerate). Coin selection uses min_feerate; as initiator, min_feerate is proposed in splice_init. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use a single get_and_clear_pending_msg_events() + match pattern for the initiator's turn, matching the existing acceptor code path. Also add assertions that all expected initiator inputs and outputs were sent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f878905 to
6d75f99
Compare
When both nodes want to splice simultaneously, the quiescence tie-breaker designates one as the initiator. Previously, the losing node responded with zero contribution, requiring a second full splice session after the first splice locked. This is wasteful, especially for often-offline nodes that may connect and immediately want to splice.
Instead, the losing node contributes to the winner's splice as the acceptor, merging both contributions into a single splice transaction. Since the
FundingContributionwas originally built with initiator fees (which include common fields and shared input/output weight), the fee is adjusted to the acceptor rate before contributing, with the surplusreturned to the change output.