RBF splice funding transactions#4427
Conversation
|
👋 I see @TheBlueMatt was un-assigned. |
1552b89 to
08c05f6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4427 +/- ##
==========================================
+ Coverage 85.97% 86.07% +0.10%
==========================================
Files 159 159
Lines 104722 105727 +1005
Branches 104722 105727 +1005
==========================================
+ Hits 90030 91009 +979
+ Misses 12191 12185 -6
- Partials 2501 2533 +32
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:
|
08c05f6 to
97858df
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
4aaa312 to
d40306c
Compare
When a splice funding transaction has been negotiated but not yet confirmed, either party may initiate RBF to bump the feerate. This enables the acceptor to handle such requests, allowing continued progress toward on-chain confirmation of splices in rising fee environments. Only the acceptor side is implemented; the acceptor does not contribute funds beyond the shared funding input. The initiator side (sending tx_init_rbf and handling tx_ack_rbf) is left for a follow-up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d40306c to
f76abb6
Compare
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
lightning/src/ln/channel.rs
Outdated
|
|
||
| /// Whether we (the holder) initiated the current quiescence session. | ||
| /// Set when quiescence is established, cleared when quiescence ends. | ||
| holder_is_quiescence_initiator: bool, |
There was a problem hiding this comment.
We avoided this for splicing (well, had it then removed it), and I think we can do the same here - in validate_splice_init we only check if there's a pending splice, as if we were the quiescence initiator we'd have set one immediately on entering quiescence. We can't just check for pending splice, ofc, but can we just check if there's a funding negotiation?
There was a problem hiding this comment.
Yeah, @wpaulino mentioned this to me offline last week, but I haven't had a moment to update yet. And looks like we are already checking, too.
| )); | ||
| } | ||
|
|
||
| if self.context.minimum_depth(&self.funding) == Some(0) { |
There was a problem hiding this comment.
Is this mandated by the spec? I mean its probably right for now but long-term it would be nice to separate "chanel was opened 0conf" from "channel does 0conf splicing"
There was a problem hiding this comment.
Yeah, it's in the spec, and IIRC we alway consider zero-conf channels to use zero-conf splices.
The receiving node:
- If `option_zeroconf` has been negotiated:
- MUST send a `warning` and close the connection or send an `error`
and fail the channel.
There was a problem hiding this comment.
Ugh, how annoying. Its something we should try to follow up on imo but certainly not in this PR.
| ))); | ||
| } | ||
|
|
||
| if pending_splice.received_funding_txid.is_some() { |
There was a problem hiding this comment.
Shouldn't we also check if we sent a splice_locked?
There was a problem hiding this comment.
Hmm... not according to the spec. I suppose the we could have not seen a re-org yet?
There was a problem hiding this comment.
Hmm, the inverse could be true too, though. They locked then saw a reorg and want to RBF but can't cause they locked. It seems like allowing this does more harm than good (implies we'll do a splice that will likely fail) and most likely its because of a bug not because of a weird reorg edge case?
There was a problem hiding this comment.
Added the checks and reached out to tbast about this.
| )); | ||
| } | ||
|
|
||
| if pending_splice.sent_funding_txid.is_some() { |
There was a problem hiding this comment.
Similarly here, shouldn't we fail if the counterparty sent a splice_locked?
There was a problem hiding this comment.
Perhaps a similar argument regarding re-orgs?
| }) | ||
| } | ||
|
|
||
| fn validate_tx_ack_rbf(&self, msg: &msgs::TxAckRbf) -> Result<FundingScope, ChannelError> { |
There was a problem hiding this comment.
It does feel like there's some room to DRY up the RBF and splice logic somewhat. They're not 1-to-1 identical and generally not super long methods, but if you wouldn't mind doing (or asking claude to do) a pass to see what can be DRY'd I'd appreciate it. Definitely for the ack handling they're really close.
There was a problem hiding this comment.
Done! tell me what you think.
There was a problem hiding this comment.
Thanks, LGTM. Is there a similar step we could do for the init handling as well?
There was a problem hiding this comment.
Done and worked the eariler refactor a little to match.
f76abb6 to
eb2ab08
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fixup! Accept tx_init_rbf for pending splice transactions Remove redundant `holder_is_quiescence_initiator` field from `FundedChannel`. Checking `pending_splice.funding_negotiation.is_some()` already covers this case since `send_splice_init` is called in the same `stfu()` handler that establishes quiescence, immediately setting `funding_negotiation = Some(AwaitingAck)`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fixup! Accept tx_init_rbf for pending splice transactions Remove unnecessary debug_assert!(false) in internal_tx_init_rbf; unreachable_no_such_peer already conveys the intent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract resolve_queued_contribution and FundingNegotiation::for_acceptor helpers to deduplicate the shared head and tail of the splice_init and tx_init_rbf acceptor code paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The channel monitor previously rejected any new pending funding when one already existed. This prevented adding RBF candidates for a pending splice since each candidate needs its own pending funding entry. Relax the check to only reject new pending funding when its splice parent differs from existing entries, allowing multiple RBF candidates that compete to confirm the same splice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expose ChannelManager::rbf_channel as the entry point for bumping the feerate of a pending splice funding transaction. Like splice_channel, it returns a FundingTemplate to be completed and passed to funding_contributed. Validates that a pending splice exists with at least one negotiated candidate, no active funding negotiation, and that the new feerate satisfies the 25/24 increase rule required by the spec. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the quiescence initiator has a pending splice and enters the stfu handler with a QuiescentAction::Splice, send tx_init_rbf to bump the existing splice's feerate rather than starting a new splice_init. This reuses the same QuiescentAction::Splice variant for both initial splices and RBF attempts -- the stfu handler distinguishes them by checking whether pending_splice already exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After sending tx_init_rbf, the initiator receives tx_ack_rbf from the acceptor. Implement the handler to validate the response and begin interactive transaction construction for the RBF funding transaction. Only clear the interactive signing session in `reset_pending_splice_state` when the current funding negotiation is in `AwaitingSignatures`. When an earlier round completed signing and a later RBF round is in `AwaitingAck` or `ConstructingTransaction`, the session belongs to the prior round and must be preserved. Otherwise, disconnecting mid-RBF would destroy the completed prior round's signing session and fire a false debug assertion. Update test_splice_rbf_acceptor_basic to exercise the full initiator flow: rbf_channel → funding_contributed → STFU exchange → tx_init_rbf → tx_ack_rbf → interactive TX → signing → mining → splice_locked. This replaces the previous test that manually constructed tx_init_rbf. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…itiator Add FundingNegotiation::for_initiator as a parallel to for_acceptor, and PendingFunding::take_awaiting_ack_context to extract the context from AwaitingAck at each call site. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove unnecessary debug_assert in internal_tx_ack_rbf since the unreachable_no_such_peer helper already handles the missing peer case consistently with other message handlers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously, the tx_init_rbf acceptor always contributed zero to the RBF transaction. This is incorrect when both parties try to RBF simultaneously and one loses the quiescence tie-breaker — the loser becomes the acceptor but still has a pending QuiescentAction::Splice with inputs/outputs that should be included in the RBF transaction. Consume the acceptor's QuiescentAction in the tx_init_rbf handler, just as is already done in the splice_init handler, and report the contribution in the TxAckRbf response.
When the counterparty initiates an RBF and we have no new contribution queued via QuiescentAction, we must re-use our prior contribution so that our splice is not lost. Track contributions in a new field on PendingFunding so the last entry can be re-used in this scenario. Each entry stores the feerate-adjusted version because that reflects what was actually negotiated and allows correct feerate re-adjustment on subsequent RBFs. Only explicitly provided contributions (from a QuiescentAction) append to the vec. Re-used contributions are replaced in-place with the version adjusted for the new feerate so they remain accurate for further RBF rounds, without growing the vec. Add test_splice_rbf_acceptor_recontributes to verify that when the counterparty initiates an RBF and we have no new QuiescentAction queued, our prior contribution is automatically re-used so the splice is preserved. Add test_splice_rbf_recontributes_feerate_too_high to verify that when the counterparty RBFs at a feerate too high for our prior contribution to cover, the RBF is rejected rather than proceeding without our contribution. Add test for sequential RBF splice attempts Add test_splice_rbf_sequential that exercises three consecutive RBF rounds on the same splice (initial → RBF lightningdevkit#1 → RBF lightningdevkit#2) to verify: - Each round requires the 25/24 feerate increase (253 → 264 → 275) - DiscardFunding events reference the correct funding txid from each replaced candidate - The final RBF splice can be mined and splice_locked successfully Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When funding_contributed is called while a splice negotiation is already in progress, unique contributions are computed to determine what to return via FailSplice or DiscardFunding. Without considering negotiated candidates stored in PendingFunding::contributions, UTXOs locked in earlier candidates could be incorrectly returned as reclaimable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SpliceFundingFailed events return contributed inputs and outputs to the user so they can unlock the associated UTXOs. When an RBF attempt is in progress, inputs/outputs already consumed by prior contributions must be excluded to avoid the user prematurely unlocking UTXOs that are still needed by the active funding negotiation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the generic error handling in splice_init with explicit matching on FeeRateAdjustmentError variants: - FeeRateTooLow: initiator's feerate is below our minimum. Proceed without contribution and preserve QuiescentAction for an RBF retry at our preferred feerate. - FeeRateTooHigh: initiator's feerate exceeds our maximum and would consume too much of our change output. Reject the splice with WarnAndDisconnect. - BudgetInsufficient: our fee budget can't cover the acceptor's fair fee at this feerate. Proceed without contribution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…play
Move behavioral suffixes ("proceeding without contribution") into the
Display impl for non-fatal FeeRateAdjustmentError variants, allowing
the match in splice_init to collapse from 3 Err arms to 2 with a
common log prefix.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Apply the same explicit FeeRateAdjustmentError handling to the tx_init_rbf acceptor path as was done for splice_init: - TooLow: proceed without queued contribution, preserve QuiescentAction for an RBF retry at our preferred feerate. - TooHigh: reject with WarnAndDisconnect. - BudgetInsufficient: proceed without queued contribution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
eb2ab08 to
9c91289
Compare
| match c.net_value_for_acceptor_at_feerate(feerate, holder_balance.unwrap()) { | ||
| Ok(net_value) => Some(net_value), | ||
| Err(e @ FeeRateAdjustmentError::FeeRateTooHigh { .. }) => { | ||
| return Err(ChannelError::WarnAndDisconnect(format!( |
There was a problem hiding this comment.
Oh I missed this. Why are we rejecting the entire splice if the feerate is too high? I guess it implies we won't be able to RBF to add our additions immediately (because the feerate will be too high for us) but it seems kinda antisocial to reject the splice entirely.
There was a problem hiding this comment.
Our contributions may have been from the last round (initial splice or RBF attempt) not because of a lost tie-breaker. So if our counterparty initiates RBF here with a fee rate we aren't comfortable with, we'd effectively drop our contribution if we didn't reject. So we'd either need to go back to the user and have them approve the higher fee rate / select new coins (not possible currently) or wait for the splice to lock before starting a new splice.
There was a problem hiding this comment.
Oh, duh, I think I had seen that and concluded the same. I'm tired today 🤦
|
|
||
| /// The feerate used in the last successfully negotiated funding transaction. | ||
| /// Used for validating the 25/24 feerate increase rule on RBF attempts. | ||
| last_funding_feerate_sat_per_1000_weight: Option<u32>, |
There was a problem hiding this comment.
Can't we just use the last one in candidates?
| ))); | ||
| } | ||
|
|
||
| let first_candidate = match pending_splice.negotiated_candidates.first() { |
There was a problem hiding this comment.
Shouldn't this be the last candidate?
| // Check the 25/24 feerate increase rule | ||
| let prev_feerate = | ||
| pending_splice.last_funding_feerate_sat_per_1000_weight.unwrap_or_else(|| { | ||
| fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::UrgentOnChainSweep) |
There was a problem hiding this comment.
This should be unreachable, right?
| if let Some(parent_funding_txid) = channel_parameters.splice_parent_funding_txid.as_ref() { | ||
| // Only one splice can be negotiated at a time after we've exchanged `channel_ready` | ||
| // (implying our funding is confirmed) that spends our currently locked funding. | ||
| if !self.pending_funding.is_empty() { |
There was a problem hiding this comment.
This made sure a dual-funded channel became locked prior to allowing a splice to happen.
| let discard_txids: Vec<_> = events[1..] | ||
| .iter() | ||
| .map(|e| match e { | ||
| Event::DiscardFunding { funding_info: FundingInfo::Tx { transaction }, .. } => { |
There was a problem hiding this comment.
Shouldn't we be getting the contribution variant?
| &[first_splice_tx.compute_txid()], | ||
| ); | ||
|
|
||
| // Clean up old watched outpoints. |
There was a problem hiding this comment.
Do this in lock_rbf_splice_after_blocks like we do with lock_splice_after_blocks?
| expect_splice_pending_event(&nodes[1], &node_id_0); | ||
|
|
||
| // Step 12: Mine, lock, and verify DiscardFunding for the replaced splice candidate. | ||
| lock_rbf_splice_after_blocks( |
There was a problem hiding this comment.
Would be nice to not have a separate version to lock_splice_after_blocks and instead add an argument is_rbf
| self.contributions.iter().flat_map(|c| c.contributed_inputs()) | ||
| } | ||
|
|
||
| fn contributed_outputs(&self) -> impl Iterator<Item = &TxOut> + '_ { |
There was a problem hiding this comment.
Reminder to filter by script_pubkey instead. IIRC you also mentioned that the change output may not be included here?
| MAX_LOCAL_BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, | ||
| }; | ||
| use crate::ln::funding::{FundingContribution, FundingTemplate, FundingTxInput}; | ||
| use crate::ln::funding::{ |
| pub(super) enum FeeRateAdjustmentError { | ||
| /// The counterparty's proposed feerate is below `min_feerate`, which was used as the feerate | ||
| /// during coin selection. | ||
| /// during coin selection. We'll retry via RBF at our preferred feerate. |
There was a problem hiding this comment.
Commit 9c91289 only has test code but the message makes it seem like we're changing actual code behavior
After a splice has been negotiated but hasn't been locked, its fee rate may be bumped using replace-by-fee (RBF). This requires the channel to re-enter quiescence upon which
tx_init_rbf/tx_ack_rbfare exchanged and the interactive-tx constructor protocol commences as usual.This PR adds support for initiating and accepting RBF attempts, as well as contributing to the attempt as an acceptor when losing the quiescence tie breaker, assuming the initiator's fee rate allows for it without needing to re-run coin selection. Instead, the difference in fee rate may be fine if not paying for common fields and shared input / outputs as the acceptor allows for it or if the change output (if any) can be adjusted accordingly.
TODO for follow-up PRs:
feerateinFundingTemplateto bemin_feerateand have user supply it inFundingContributionbuildershave RBF acceptor w/oQuiescentActionreuse any contributions from previous attemptssplice_channelcontribution if splice not yet lockedDiscardFundingis generated in edge casesBased on #4416