RBF splice funding transactions#4427
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
1552b89 to
08c05f6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4427 +/- ##
==========================================
+ Coverage 85.97% 85.99% +0.02%
==========================================
Files 159 159
Lines 104722 105977 +1255
Branches 104722 105977 +1255
==========================================
+ Hits 90030 91132 +1102
- Misses 12191 12309 +118
- Partials 2501 2536 +35
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. |
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
eb2ab08 to
9c91289
Compare
lightning/src/ln/channel.rs
Outdated
| 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 🤦
lightning/src/ln/channel.rs
Outdated
| ))); | ||
| } | ||
|
|
||
| let first_candidate = match pending_splice.negotiated_candidates.first() { |
There was a problem hiding this comment.
Shouldn't this be the last candidate?
There was a problem hiding this comment.
IIUC, it doesn't matter since the keys should be the same in each.
There was a problem hiding this comment.
Yeah I realized as I read the rest of the diff, still seems best to use the last one though in case we need to do something else with it in the future.
lightning/src/ln/splicing_tests.rs
Outdated
| 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?
There was a problem hiding this comment.
When the splice is locked, we get DiscardFunding from the monitor, so we don't have the contributions.
lightning/src/ln/splicing_tests.rs
Outdated
| &[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?
There was a problem hiding this comment.
Good call. Claude decided to simplify this a bit, so let me know if it still looks sane lol.
| 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
There was a problem hiding this comment.
Done, though need to pass txids. May want to give that a close look.
| 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?
There was a problem hiding this comment.
Done. The change output will be included but may be filtered later. IIRC, the issue may have been later when converting this to InteractiveTxConstructor and so forth that we couldn't differentiate change from splice-out outputs for mixed mode. But that shouldn't matter since we can generalize the filtering to any script pubkey.
| 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
There was a problem hiding this comment.
Ah, implementing an earlier requested refactoring effectively makes this into a fixup now.
jkczyz
left a comment
There was a problem hiding this comment.
Will likely need to get to these tomorrow, but wanted to respond to feedback.
lightning/src/ln/channel.rs
Outdated
| ))); | ||
| } | ||
|
|
||
| let first_candidate = match pending_splice.negotiated_candidates.first() { |
There was a problem hiding this comment.
IIUC, it doesn't matter since the keys should be the same in each.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
9c91289 to
e9cbe71
Compare
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
e9cbe71 to
c5d7dd2
Compare
|
Squashed |
lightning/src/ln/channel.rs
Outdated
| (5, sent_funding_txid, option), | ||
| (7, received_funding_txid, option), | ||
| (9, last_funding_feerate_sat_per_1000_weight, option), | ||
| (11, contributions, optional_vec), |
There was a problem hiding this comment.
We may want to consider making these even (will prevent downgrading if you RBF) such that we don't lose this context
| let splice_funding_failed = maybe_create_splice_funding_failed!( | ||
| self, | ||
| self.pending_splice.as_mut(), | ||
| self.pending_splice.as_ref(), |
There was a problem hiding this comment.
If we successfully negotiate an RBF attempt and discard it here, we need to reset the last stored feerate to the previously negotiated candidate (if any)
There was a problem hiding this comment.
Yeah, ended up moving the last_funding_feerate_sat_per_1000_weight update to after exchanging signatures.
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
The maybe_create_splice_funding_failed! macro only emitted SpliceFailed and DiscardFunding events for the splice initiator. When an acceptor contributed inputs/outputs and the negotiation failed (e.g., disconnect), their contributions were silently discarded with no event notification, preventing the acceptor from reclaiming its UTXOs. Replace the is_initiator() filter with a post-hoc check on whether there are contributions to discard. The initiator always gets events, the acceptor gets events when it has contributions, and acceptors without contributions get no events (nothing to discard). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the last_funding_feerate_sat_per_1000_weight update from the start of negotiation (send_splice_init, splice_init, tx_init_rbf) to on_tx_signatures_exchange, when the candidate is finalized. This ensures the feerate floor reflects the last successful negotiation rather than the most recently attempted one, so a failed RBF round doesn't inflate the feerate baseline for subsequent attempts. Thread funding_feerate_sat_per_1000_weight through the ConstructingTransaction and AwaitingSignatures variants so it's available at finalization time. Co-Authored-By: Claude Opus 4.6 (1M context) <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>
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>
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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <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>
Extend test_splice_rbf_disconnect_filters_prior_contributions to verify that after a failed RBF round, the feerate floor remains at the last successful round's feerate. Attempts a second RBF at the same feerate as the failed round, which should succeed because the floor was never updated by the failed round. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the generic error handling in splice_init and tx_init_rbf 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. - FeeBufferInsufficient: our fee buffer can't cover the acceptor's estimated fee at this feerate. Proceed without contribution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c5d7dd2 to
76880a2
Compare
lightning/src/ln/channel.rs
Outdated
| (5, sent_funding_txid, option), | ||
| (7, received_funding_txid, option), | ||
| (9, last_funding_feerate_sat_per_1000_weight, option), | ||
| (11, contributions, optional_vec), |
| let splice_funding_failed = maybe_create_splice_funding_failed!( | ||
| self, | ||
| self.pending_splice.as_mut(), | ||
| self.pending_splice.as_ref(), |
There was a problem hiding this comment.
Yeah, ended up moving the last_funding_feerate_sat_per_1000_weight update to after exchanging signatures.
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