Preserve original contribution on counterparty RBF abort
What changed, and why it matters
This commit fixes a bug in Lightning Dev Kit's channel splicing/RBF logic. When a counterparty started a fee-bump (RBF) negotiation that later aborted, the local node's record of its own funding contribution was accidentally left at the higher, adjusted fee rate instead of reverting to the original. The fix makes the contribution history append-only and pops the unconfirmed round's entry on abort, restoring the original contribution. The bug could cause incorrect fee accounting and, in some abort paths, a panic or wrong wallet events when the node later initiated its own RBF.
Treat as a correctness/security fix worth including in release notes. Review related abort paths to ensure no other stale state remains. Consider whether the panic path and wrong DiscardFunding events warrant a CVE; the commit itself does not claim security relevance, but the state inconsistency could affect channel safety.
Security signals we found
State inconsistency after aborted RBF: stale higher feerate persisted in contribution record
Potential panic in subsequent local RBF due to violated feerate assumptions
Incorrect wallet reclaim events (DiscardFunding) for UTXOs still committed to a prior splice round
Append-only contribution log with rollback on abort
Test added/updated to reproduce and verify abort behavior
Evidence from the diff
In lightning/src/ln/channel.rs, PendingFunding::contributions was previously a single mutable entry that got replaced when acting as RBF acceptor. If the RBF round aborted, the replaced (higher-feerate) contribution persisted. The patch changes contributions to an append-only log: each negotiation round pushes a new FundingContribution. On abort, reset_pending_splice_state now pops the last entry if its feerate does not match the locked/negotiated feerate, restoring the prior round. The acceptor path now clones the last entry and appends the feerate-adjusted version instead of popping and replacing. Tests are updated to assert the original feerate is restored and that no spurious DiscardFunding events are emitted for UTXOs still committed to an earlier round.
Changed components
lightning/src/ln/channel.rslightning/src/ln/splicing_tests.rsPendingFunding::contributionsFundedChannel::reset_pending_splice_stateRBF acceptor contribution adjustment pathInspect captured patch +48 / −31
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 57aa83a..a0f1609 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -2908,11 +2908,17 @@ struct PendingFunding {
/// Used for validating the 25/24 feerate increase rule on RBF attempts.
last_funding_feerate_sat_per_1000_weight: Option<u32>,
- /// The funding contributions from all explicit splice/RBF attempts on this channel.
- /// Each entry reflects the feerate-adjusted contribution that was actually used in that
- /// negotiation. The last entry is re-used when the counterparty initiates an RBF and we
- /// have no pending `QuiescentAction`. When re-used as acceptor, the last entry is replaced
- /// with the version adjusted for the new feerate.
+ /// The funding contributions from splice/RBF rounds where we contributed.
+ ///
+ /// A new entry is appended when we contribute to a negotiation round (either as initiator
+ /// or acceptor). Rounds where we don't contribute (e.g., counterparty-only splice) do not
+ /// add an entry. Once non-empty, every subsequent round appends: when the counterparty
+ /// initiates an RBF, the last entry is adjusted to the new feerate and appended as a new
+ /// entry (or the RBF is rejected if the adjustment fails, in which case no round starts).
+ ///
+ /// If the round aborts, the last entry is popped in
+ /// [`FundedChannel::reset_pending_splice_state`], restoring the prior round's contribution
+ /// as the most recent entry.
contributions: Vec<FundingContribution>,
}
@@ -6958,6 +6964,22 @@ where
into_contributed_inputs_and_outputs
);
+ // Pop the current round's contribution if it wasn't from a negotiated round. Each round
+ // pushes a new entry to `contributions`; if the round aborts, we undo the push so that
+ // `contributions.last()` reflects the most recent negotiated round's contribution. This
+ // must happen after `maybe_create_splice_funding_failed` so that
+ // `prior_contributed_inputs` still includes the prior rounds' entries for filtering.
+ if let Some(pending_splice) = self.pending_splice.as_mut() {
+ if let Some(last) = pending_splice.contributions.last() {
+ let was_negotiated = pending_splice
+ .last_funding_feerate_sat_per_1000_weight
+ .is_some_and(|f| last.feerate() == FeeRate::from_sat_per_kwu(f as u64));
+ if !was_negotiated {
+ pending_splice.contributions.pop();
+ }
+ }
+ }
+
if self.pending_funding().is_empty() {
self.pending_splice.take();
}
@@ -12736,11 +12758,12 @@ where
} else if prior_net_value.is_some() {
let prior_contribution = self
.pending_splice
- .as_mut()
+ .as_ref()
.expect("pending_splice is Some")
.contributions
- .pop()
- .expect("prior_net_value was Some");
+ .last()
+ .expect("prior_net_value was Some")
+ .clone();
let adjusted_contribution = prior_contribution
.for_acceptor_at_feerate(feerate, holder_balance.unwrap())
.expect("feerate compatibility already checked");
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index 6971e91..c66d1a0 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -5239,9 +5239,9 @@ fn test_splice_rbf_acceptor_recontributes() {
#[test]
fn test_splice_rbf_after_counterparty_rbf_aborted() {
- // When a counterparty-initiated RBF is aborted, the acceptor's prior contribution retains
- // the adjusted feerate. Initiating our own RBF afterward must not panic even though the
- // prior contribution's feerate may be >= the new rbf_feerate.
+ // When a counterparty-initiated RBF is aborted, the acceptor's prior contribution is
+ // restored to the original feerate (before adjustment). Initiating our own RBF afterward
+ // uses this restored contribution.
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -5326,8 +5326,8 @@ fn test_splice_rbf_after_counterparty_rbf_aborted() {
let tx_ack_rbf = complete_rbf_handshake(&nodes[0], &nodes[1]);
assert!(tx_ack_rbf.funding_output_contribution.is_some());
- // Step 4: Abort the RBF. Node 0 sends tx_abort; node 1's prior contribution retains the
- // adjusted feerate.
+ // Step 4: Abort the RBF. Node 0 sends tx_abort; node 1's prior contribution is restored
+ // to the original feerate (the RBF round's adjusted entry is popped from contributions).
// Drain node 0's pending TxAddInput from the interactive tx negotiation start.
nodes[0].node.get_and_clear_pending_msg_events();
@@ -5347,11 +5347,17 @@ fn test_splice_rbf_after_counterparty_rbf_aborted() {
nodes[1].node.get_and_clear_pending_events();
// Step 5: Node 1 initiates its own RBF via splice_channel → rbf_sync.
- // The prior contribution's feerate is now >= rbf_feerate. This must not panic.
+ // The prior contribution's feerate is restored to the original floor feerate, not the
+ // RBF-adjusted feerate.
provide_utxo_reserves(&nodes, 2, added_value * 2);
let funding_template = nodes[1].node.splice_channel(&channel_id, &node_id_0).unwrap();
assert!(funding_template.min_rbf_feerate().is_some());
+ assert_eq!(
+ funding_template.prior_contribution().unwrap().feerate(),
+ feerate,
+ "Prior contribution should have the original feerate, not the RBF-adjusted one",
+ );
let wallet = WalletSync::new(Arc::clone(&nodes[1].wallet_source), nodes[1].logger);
let rbf_contribution = funding_template.rbf_sync(FeeRate::MAX, &wallet);
@@ -5646,24 +5652,12 @@ fn test_splice_rbf_acceptor_contributes_then_disconnects() {
other => panic!("Expected DiscardFunding with Contribution, got {:?}", other),
}
- // The acceptor should also get SpliceFailed + DiscardFunding with its contributed
- // inputs/outputs so it can reclaim its UTXOs.
+ // The acceptor re-contributed the same UTXOs as round 0 (via prior contribution
+ // adjustment). Since those UTXOs are still committed to round 0's splice, they are
+ // filtered from the DiscardFunding event. With all inputs/outputs filtered, no events
+ // are emitted for the acceptor.
let events = nodes[1].node.get_and_clear_pending_events();
- assert_eq!(events.len(), 2, "{events:?}");
- match &events[0] {
- Event::SpliceFailed { channel_id: cid, .. } => assert_eq!(*cid, channel_id),
- other => panic!("Expected SpliceFailed, got {:?}", other),
- }
- match &events[1] {
- Event::DiscardFunding {
- funding_info: FundingInfo::Contribution { inputs, outputs },
- ..
- } => {
- assert!(!inputs.is_empty(), "Expected acceptor inputs, got empty");
- assert!(!outputs.is_empty(), "Expected acceptor outputs, got empty");
- },
- other => panic!("Expected DiscardFunding with Contribution, got {:?}", other),
- }
+ assert_eq!(events.len(), 0, "{events:?}");
// Reconnect.
let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
Why this scored 57/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.