Skip to content

Unify splice and RBF APIs#4486

Open
jkczyz wants to merge 3 commits intolightningdevkit:mainfrom
jkczyz:2026-03-splicing-rbf-merge
Open

Unify splice and RBF APIs#4486
jkczyz wants to merge 3 commits intolightningdevkit:mainfrom
jkczyz:2026-03-splicing-rbf-merge

Conversation

@jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Mar 16, 2026

Users previously had to choose between splice_channel and rbf_channel upfront. This PR merges them into a single entry point and makes the supporting changes needed to enable that.

  • Merge rbf_channel into splice_channel. The returned FundingTemplate now carries PriorContribution (Adjusted/Unadjusted) so users can reuse their existing contribution for an RBF without new coin selection.
  • To support this, move feerate parameters from splice_channel/rbf_channel to FundingTemplate's splice methods, giving users control at coin-selection time and exposing the minimum RBF feerate via min_rbf_feerate().
  • Additionally, funding_contributed now automatically adjusts the contribution feerate upward to meet the 25/24 RBF requirement when a pending splice appears between splice_channel and funding_contributed calls, falling back to waiting for the pending splice to lock when adjustment isn't possible.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 16, 2026

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

Comment on lines +12055 to 12094
/// Attempts to adjust the contribution's feerate to the minimum RBF feerate so the splice can
/// proceed as an RBF immediately rather than waiting for the pending splice to lock.
/// Returns the adjusted contribution on success, or the original on failure.
fn maybe_adjust_for_rbf<L: Logger>(
&self, contribution: FundingContribution, min_rbf_feerate: FeeRate, logger: &L,
) -> FundingContribution {
if contribution.feerate() >= min_rbf_feerate {
return contribution;
}

let holder_balance = match self
.get_holder_counterparty_balances_floor_incl_fee(&self.funding)
.map(|(holder, _)| holder)
{
Ok(balance) => balance,
Err(_) => return contribution,
};

if let Err(e) =
contribution.net_value_for_initiator_at_feerate(min_rbf_feerate, holder_balance)
{
log_info!(
logger,
"Cannot adjust to minimum RBF feerate {}: {}; will proceed as fresh splice after lock",
min_rbf_feerate,
e,
);
return contribution;
}

log_info!(
logger,
"Adjusting contribution feerate from {} to minimum RBF feerate {}",
contribution.feerate(),
min_rbf_feerate,
);
contribution
.for_initiator_at_feerate(min_rbf_feerate, holder_balance)
.expect("feerate compatibility already checked")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: When maybe_adjust_for_rbf cannot adjust the feerate (the Err path at line ~12082), it returns the original contribution unchanged and logs "will proceed as fresh splice after lock". However, nothing actually implements the "wait for lock" behavior. The contribution is queued as-is into QuiescentAction::Splice, and later in stfu() (line ~13734), the decision to send tx_init_rbf vs splice_init is based solely on self.pending_splice.is_some() — there's no check whether the queued contribution's feerate satisfies the 25/24 RBF minimum.

So the flow is:

  1. funding_contributedmaybe_adjust_for_rbf fails → original low-feerate contribution queued
  2. Quiescence completes → stfu() sees pending_splice.is_some() → sends tx_init_rbf with the too-low feerate
  3. Counterparty rejects with InsufficientRbfFeeratetx_abort → splice fails entirely

The splice doesn't "proceed as fresh splice after lock" — it fails immediately. Either stfu() should check can_initiate_rbf() + feerate compatibility before choosing the RBF path, or funding_contributed should avoid queuing contributions that can't meet the RBF minimum when a pending splice exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: When maybe_adjust_for_rbf cannot adjust the feerate (the Err path at line ~12082), it returns the original contribution unchanged and logs "will proceed as fresh splice after lock". However, nothing actually implements the "wait for lock" behavior. The contribution is queued as-is into QuiescentAction::Splice, and later in stfu() (line ~13734), the decision to send tx_init_rbf vs splice_init is based solely on self.pending_splice.is_some() — there's no check whether the queued contribution's feerate satisfies the 25/24 RBF minimum.

That's inaccurate. The fee rate check is try_send_stfu, so we'll never send stfu when there's a pending splice that we can't RBF.

Comment on lines +6732 to +6734
for output in pending_splice.prior_contributed_outputs() {
contributed_outputs.retain(|o| o.script_pubkey != output.script_pubkey);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering outputs by script_pubkey alone could incorrectly remove the wrong output if two outputs in different rounds happen to share the same script_pubkey but have different values (e.g., different splice-out amounts to the same address). The same pattern is used in quiescent_action_into_error. Consider comparing the full TxOut (script_pubkey + value) instead:

Suggested change
for output in pending_splice.prior_contributed_outputs() {
contributed_outputs.retain(|o| o.script_pubkey != output.script_pubkey);
}
for output in pending_splice.prior_contributed_outputs() {
contributed_outputs.retain(|o| o != output);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. We don't want to reclaim an address if still in use (e.g., if the change output was adjusted).

Comment on lines +6779 to +6785
if let Some(ref pending_splice) = self.pending_splice {
for input in pending_splice.contributed_inputs() {
inputs.retain(|i| *i != input);
}
for output in pending_splice.contributed_outputs() {
outputs.retain(|o| o.script_pubkey != output.script_pubkey);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here — filtering contributed outputs by script_pubkey only. Should compare full TxOut to avoid false matches when the same script_pubkey is reused with different amounts across rounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

Comment on lines +13734 to +13738
if self.pending_splice.is_some() {
let tx_init_rbf = self.send_tx_init_rbf(context);
self.pending_splice.as_mut().unwrap()
.contributions.push(prior_contribution);
return Ok(Some(StfuResponse::TxInitRbf(tx_init_rbf)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the other half of the maybe_adjust_for_rbf bug: the RBF vs. fresh-splice decision here is purely self.pending_splice.is_some(), with no check on whether the contribution's feerate actually satisfies the 25/24 RBF minimum. When maybe_adjust_for_rbf couldn't adjust the feerate, we still land here and send tx_init_rbf with a feerate that will be rejected.

Consider checking self.can_initiate_rbf() and comparing against contribution.feerate() before choosing this path, falling back to send_splice_init when the feerate is insufficient (and the pending splice would need to lock first).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the other half of the maybe_adjust_for_rbf bug: the RBF vs. fresh-splice decision here is purely self.pending_splice.is_some(), with no check on whether the contribution's feerate actually satisfies the 25/24 RBF minimum. When maybe_adjust_for_rbf couldn't adjust the feerate, we still land here and send tx_init_rbf with a feerate that will be rejected.

Wrong for the same reason. We'll never send stfu in this case.

Consider checking self.can_initiate_rbf() and comparing against contribution.feerate() before choosing this path, falling back to send_splice_init when the feerate is insufficient (and the pending splice would need to lock first).

Like what we currently do by not sending stfu.

}

impl_writeable_tlv_based_enum_upgradable!(FundingNegotiation,
(0, AwaitingSignatures) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: When restoring from old data, funding_feerate_sat_per_1000_weight defaults to 0. This propagates to last_funding_feerate_sat_per_1000_weight = Some(0) when signing completes, making the 25/24 RBF minimum check (new * 24) < (0 * 25) always pass. So the first RBF after restoring old state won't enforce the feerate bump rule. Consider whether this is acceptable or if a sentinel/None should be used instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, this an acceptable tradeoff. Our counterparty should reject, and we won't be able to RBF until they do first. For accepting, we check against fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::UrgentOnChainSweep) if not set.

@ldk-claude-review-bot
Copy link
Collaborator

ldk-claude-review-bot commented Mar 16, 2026

@TheBlueMatt
Copy link
Collaborator

Can be rebased (and presumably undrafted) now 🎉

…plate

The user doesn't choose the feerate at splice_channel/rbf_channel time —
they choose it when performing coin selection. Moving feerate to the
FundingTemplate::splice_* methods gives users more control and lets
rbf_channel expose the minimum RBF feerate (25/24 of previous) on the
template so users can choose an appropriate feerate.

splice_channel and rbf_channel no longer take min_feerate/max_feerate.
Instead, FundingTemplate gains a min_rbf_feerate() accessor that returns
the RBF floor when applicable (from negotiated candidates or in-progress
funding negotiations). The feerate parameters move to the splice_in_sync,
splice_out_sync, and splice_in_and_out_sync methods (and their async
variants), which validate that min_feerate >= min_rbf_feerate before
coin selection.

Fee estimation documentation moves from splice_channel/rbf_channel to
funding_contributed, where the contribution (and its feerate range)
is actually provided and the splice process begins.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-merge branch from 1986278 to 7fb35ec Compare March 17, 2026 02:57
@jkczyz jkczyz marked this pull request as ready for review March 17, 2026 02:57
@jkczyz jkczyz requested review from TheBlueMatt and wpaulino March 17, 2026 02:57
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum PriorContribution {
/// The prior contribution's feerate meets or exceeds the minimum RBF feerate.
Adjusted(FundingContribution),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need to expose this in a public API? Why shouldn't we do the adjustment when building the funding contribution instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time, I was thinking that we should attempt it before returning the FundingTemplate since we need the channel balance to call net_value_for_initiator_at_feerate. But it seems we should just include the balance in the template instead. We can't really control that shifting whether we do the computation upfront or wait for the user to do it later.

///
/// `max_feerate` is the maximum feerate the caller is willing to accept as acceptor. It is
/// used as the returned contribution's `max_feerate` and also constrains coin selection when
/// re-running it for unadjusted prior contributions or fee-bump-only contributions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explicitly call out that you can RBF your counterparty's transaction, and in doing so will take over responsibility for all the fees, so to be sure to check if that's what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, though they would still pay for their own inputs/outputs.

.as_ref()
.and_then(|pending_splice| pending_splice.funding_negotiation.as_ref())
{
// A splice is currently being negotiated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some checks in can_initiate_rbf that might also apply here. eg if the channel is 0-conf we can't ever rbf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those will be checked when try_send_stfu is called. For the second case (i.e., negotiation fails), we'd have a min_rbf_feerate set when not needed, though. Refactored the RBF-compatibility check to be used first to avoid this.

/// coin selection.
pub fn splice_in_sync<W: CoinSelectionSourceSync>(
self, value_added: Amount, wallet: W,
self, value_added: Amount, min_feerate: FeeRate, max_feerate: FeeRate, wallet: W,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the existing methods do in the context of an RBF is pretty unclear. It currently appears to just entirely replace the existing splice-in with a higher feerate. ISTM we could instead be being asked to add additional funds in addition to what was already there? For splice-out that's almost certainly what we're being asked to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... it would be confusing if they were to call the say splice_in when the prior contribution was created with splice_out, effectively making it a splice_in_and_out. Curious what @wpaulino thinks given he's working on the the mixed-mode use cases. Updated the docs for now but open to considering the prior contribution in some way.

@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-merge branch from 7fb35ec to 1189c8b Compare March 18, 2026 16:42
@jkczyz jkczyz requested a review from TheBlueMatt March 18, 2026 18:33
Some(PriorContribution { contribution, holder_balance }) => {
// The prior contribution's feerate is the negotiated feerate from the
// previous splice, which is always below the RBF minimum (negotiated + 25).
debug_assert!(contribution.feerate < rbf_feerate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (debug builds): This debug_assert is incorrect and will fire after a counterparty-initiated RBF is aborted.

Scenario:

  1. Our splice completes at feerate X, contribution stored in contributions
  2. Counterparty initiates RBF at feerate Y ≥ X×25/24
  3. In tx_init_rbf handler, our prior contribution is pop()'d, adjusted to feerate Y via for_acceptor_at_feerate, and push()'d back
  4. RBF is aborted (tx_abort) — our contributions.last() now has feerate = Y
  5. User calls splice_channel()can_initiate_rbf() returns min_rbf = X×25/24
  6. build_prior_contribution() clones the contribution with feerate = Y
  7. User calls rbf_sync()rbf_feerate = X×25/24 ≤ Y → debug_assert!(Y < X×25/24) fires

In release builds the fallback path handles this correctly (the net_value_for_initiator_at_feerate call returns FeeRateTooLow and coin selection re-runs). But in debug/test builds this panics.

The comment "The prior contribution's feerate is the negotiated feerate from the previous splice, which is always below the RBF minimum" is not always true — it may be a feerate-adjusted version from a failed counterparty RBF.

Some(PriorContribution { contribution, holder_balance }) => {
// The prior contribution's feerate is the negotiated feerate from the
// previous splice, which is always below the RBF minimum (negotiated + 25).
debug_assert!(contribution.feerate < rbf_feerate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same debug_assert issue as in rbf — will fire in debug builds after a counterparty-initiated RBF abort leaves a higher-feerate contribution in contributions.last(). See comment on the rbf method above for the full scenario.

Comment on lines +12196 to +12203
// If a pending splice exists with negotiated candidates, attempt to adjust the
// contribution's feerate to the minimum RBF feerate so it can proceed as an RBF immediately
// rather than waiting for the splice to lock.
let contribution = if let Ok(min_rbf_feerate) = self.can_initiate_rbf() {
self.maybe_adjust_for_rbf(contribution, min_rbf_feerate, logger)
} else {
contribution
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The try_send_stfu guard at line ~13806 does correctly implement the "wait for lock" behavior by refusing to send stfu when contribution.feerate() < min_rbf_feerate. So the prior review concern about the unadjusted contribution immediately becoming a tx_init_rbf is mitigated — the splice will indeed wait. However, consider adding a brief comment here (or in the log message below at line 12098) noting that try_send_stfu gates this, since the "will proceed as fresh splice after lock" log message in maybe_adjust_for_rbf otherwise reads as if waiting is automatic when it actually depends on a downstream guard.

jkczyz and others added 2 commits March 18, 2026 13:53
…uted

When splice_channel is called before a counterparty's splice exists, the
user builds a contribution at their chosen feerate without a minimum RBF feerate.
If the counterparty completes a splice before funding_contributed is
called, the contribution's feerate may be below the 25/24 RBF
requirement. Rather than always waiting for the pending splice to lock
(which would proceed as a fresh splice), funding_contributed now attempts
to adjust the contribution's feerate upward to the minimum RBF feerate when the
budget allows, enabling an immediate RBF.

When the adjustment isn't possible (max_feerate too low or insufficient
fee buffer), the contribution is left unchanged and try_send_stfu delays
until the pending splice locks, at which point the splice proceeds at the
original feerate.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Users previously had to choose between splice_channel (fresh splice) and
rbf_channel (fee bump) upfront. Since splice_channel already detects
pending splices and computes the minimum RBF feerate, rbf_channel was
redundant. Merging into a single API lets the user call one method and
discover from the returned FundingTemplate whether an RBF is possible.

The FundingTemplate now carries the user's prior contribution from the
previous splice negotiation when one is available. This lets users reuse
their existing contribution for an RBF without performing new coin
selection. A PriorContribution enum distinguishes whether the
contribution has been adjusted to the minimum RBF feerate (Adjusted) or
could not be adjusted due to insufficient fee buffer or max_feerate
constraints (Unadjusted).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-merge branch from 305e81e to 8fd4927 Compare March 18, 2026 18:57
Comment on lines +6530 to +6532
/// while a previous splice was still being negotiated, its
/// [`min_rbf_feerate`][FundingTemplate::min_rbf_feerate] may be stale after the failure.
/// Call this method again to get a fresh template.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc error: "Call this method again to get a fresh template" — but "this method" is funding_contributed, which doesn't return a FundingTemplate. Should read "Call [ChannelManager::splice_channel] again to get a fresh template."

Suggested change
/// while a previous splice was still being negotiated, its
/// [`min_rbf_feerate`][FundingTemplate::min_rbf_feerate] may be stale after the failure.
/// Call this method again to get a fresh template.
/// [`min_rbf_feerate`][FundingTemplate::min_rbf_feerate] may be stale after the failure.
/// Call [`ChannelManager::splice_channel`] again to get a fresh template.

Comment on lines +12196 to +12203
// If a pending splice exists with negotiated candidates, attempt to adjust the
// contribution's feerate to the minimum RBF feerate so it can proceed as an RBF immediately
// rather than waiting for the splice to lock.
let contribution = if let Ok(min_rbf_feerate) = self.can_initiate_rbf() {
self.maybe_adjust_for_rbf(contribution, min_rbf_feerate, logger)
} else {
contribution
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: validate_splice_contributions at line 12177 runs before this adjustment. For splice-out contributions (no inputs, fees from channel balance), the adjusted contribution has a more negative net_value() due to higher fees — but this adjusted value is never re-validated against channel reserves.

compute_feerate_adjustment checks holder_balance >= target_fee + value_removed, but doesn't check the v2 channel reserve constraint that validate_splice_contributions enforces (post-splice balance >= 1% of post-splice channel value). A marginal splice-out could pass validation at the original feerate but violate channel reserves after the ~4% RBF bump.

In practice this window is very small (the 25/24 fee increase is tiny relative to reserves), and the commitment transaction construction would catch any real violation, so this wouldn't cause fund loss — just a late negotiation failure with a less clear error.

Comment on lines +217 to +224
if feerate > max_feerate {
return Err(());
}

if let Some(min_rbf_feerate) = min_rbf_feerate {
if feerate < min_rbf_feerate {
return Err(());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feerate validation failures here return opaque Err(()) with no diagnostic info. Previously, the feerate checks in splice_channel/rbf_channel returned descriptive APIError::APIMisuseError messages (e.g., "min_feerate exceeds max_feerate"). Now those checks moved here, the user gets Err(()) from the splice methods with no clue whether the issue is feerate > max_feerate, feerate < min_rbf_feerate, or coin selection failure.

Consider at minimum documenting the error conditions on each splice method, or providing a richer error type. For example, a user who forgets to check min_rbf_feerate() and passes a low feerate will get a silent Err(()) with no guidance.

Comment on lines +13806 to +13814
Ok(min_rbf_feerate) if contribution.feerate() < min_rbf_feerate => {
log_given_level!(
logger,
logger_level,
"Waiting for splice to lock: feerate {} below minimum RBF feerate {}",
contribution.feerate(),
min_rbf_feerate,
);
return None;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This gate resolves the concern from the prior review comment at line 12110 about maybe_adjust_for_rbf returning the original low-feerate contribution. When adjustment fails, the contribution is queued with feerate < min_rbf_feerate. This gate catches it: try_send_stfu returns None, preventing stfu from being sent. The splice waits until the pending splice locks (clearing pending_splice), at which point self.pending_splice.is_some() is false, the gate is skipped, and the splice proceeds as fresh via splice_init. The "wait for lock" behavior is correctly implemented here.

Similarly, the concern at line 13749 (decision purely based on pending_splice.is_some()) is safe because this gate ensures we never reach the stfu() handler at line 13745 with an insufficient feerate.

// Compute the RBF feerate floor from either negotiated candidates (via
// can_initiate_rbf) or an in-progress funding negotiation (which will become a
// negotiated candidate once it completes).
let min_rbf_feerate = self.can_initiate_rbf().ok().flatten().or_else(|| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just look at self.pending_splice.last_funding_feerate_sat_per_1000_weight without calling can_initiate_rbf?

contribution: FundingContribution,
/// The holder's balance, used for feerate adjustment. `None` when the balance computation
/// fails, in which case adjustment is skipped and coin selection is re-run.
holder_balance: Option<Amount>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't guaranteed to remain consistent by the time we actually use it. This reminds me -- do we even verify the contribution is still valid given the current set of balances by the time we're ready to send splice_init/tx_init_rbf (since by then we're quiescent and are guaranteed the balance will remain the same until we abort)?

Comment on lines +11966 to +11973
// A splice is currently being negotiated.
// - If the negotiation succeeds, the user's splice will need to satisfy the RBF
// feerate requirement. Derive the minimum RBF feerate from the negotiation's
// feerate so the user can choose an appropriate feerate.
// - If the negotiation fails (e.g., tx_abort), the splice will proceed as a fresh
// splice instead. In this case, the min_rbf_feerate becomes stale, causing a
// slightly higher feerate than necessary. Call splice_channel again after
// receiving SpliceFailed to get a fresh template without the RBF constraint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something worth documenting to the user? It's a pretty contrived scenario, can we just keep the original feerate the user provided in case the RBF fails?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants