Simplify contribution pop in reset_pending_splice_state
What the developer wrote
Authored by Jeffrey Czyz
Simplify contribution pop in reset_pending_splice_state
The was_negotiated check is unnecessary because reset_pending_splice_state
only runs when funding_negotiation is present, meaning
on_tx_signatures_exchange hasn't been called yet. Since the feerate is
only recorded in last_funding_feerate_sat_per_1000_weight during
on_tx_signatures_exchange, the current round's feerate can never match
it. So the contribution can always be unconditionally popped.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
What changed, and why it matters
This is a small code cleanup in the Lightning payment channel code that handles failed or aborted splice-in transactions. The change removes an unnecessary check when undoing a splice contribution, replacing it with an unconditional pop plus a debug-only sanity check. The commit message argues the old check could never trigger, so behavior should be unchanged. There is no direct evidence of a security bug, but any change to state-rollback logic in financial software warrants careful review because mistakes can leave funds in an inconsistent state.
Treat as a low-risk refactor but verify with targeted tests that aborted splice rounds still correctly undo their contribution and that `prior_contributed_inputs` filtering remains correct. Review whether the `debug_assert!` assumption holds under all caller paths, especially any future callers that might run after `on_tx_signatures_exchange`. No immediate security patch appears required based on the supplied materials.
Security signals we found
State rollback logic in a Bitcoin Lightning channel implementation
Removal of a conditional guard on popping splice contribution history
Addition of a debug-only assertion about feerate ordering
Potential for inconsistent channel state if the pop logic is wrong
No explicit security claim or CVE in commit or references
Evidence from the diff
In reset_pending_splice_state, the code previously popped the latest splice contribution only if it was not from a ‘negotiated round’ (determined by comparing the contribution’s feerate to last_funding_feerate_sat_per_1000_weight). The patch removes that conditional and unconditionally pops the most recent contribution, adding a debug_assert! that the popped contribution’s feerate is greater than the last negotiated feerate (or that no negotiated feerate exists). The commit message explains the conditional was dead code because reset_pending_splice_state runs before on_tx_signatures_exchange, where last_funding_feerate_sat_per_1000_weight is set, so the feerates can never match. The change is intended to be behavior-preserving.
Changed components
lightning/src/ln/channel.rsreset_pending_splice_statepending_splice.contributionssplice negotiation / abort pathInspect captured patch +13 / −13
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b26ec70..455a5ae 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -7317,20 +7317,20 @@ 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
+ // Pop the current round's contribution, if any (acceptors may not have one). This
+ // must happen after `maybe_create_splice_funding_failed` for correct filtering.
+ let pending_splice = self
+ .pending_splice
+ .as_mut()
+ .expect("reset_pending_splice_state requires pending_splice");
+ if let Some(contribution) = pending_splice.contributions.pop() {
+ debug_assert!(
+ 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();
- }
- }
+ .map(|f| contribution.feerate() > FeeRate::from_sat_per_kwu(f as u64))
+ .unwrap_or(true),
+ "current round's feerate should be greater than the last negotiated feerate",
+ );
}
if self.pending_funding().is_empty() {
Why this scored 24/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.