Rename should_reset_pending_splice_state argument
What the developer wrote
Authored by Wilmer Paulino
Rename should_reset_pending_splice_state argument
There's a case in `should_reset_pending_splice_state` where we are
awaiting signatures, but still want to preserve the pending negotiation
upon a disconnection. We previously used `counterparty_aborted` as a way
to toggle this behavior. Now that we support the user manually canceling
an ongoing negotiation, we interpret the argument a bit more
generically in terms of whether we wish to resume the negotiation or not
when we are found in such a state.
What changed, and why it matters
This commit renames and flips the meaning of a flag used during Lightning channel splice negotiations. It changes when the software decides to keep or discard an in-progress splice after a disconnect or abort. The change appears intended to support a new 'user manually cancels' case, but the logic is subtle: several call sites now pass the opposite boolean, and the function's internal cases were reordered. There is no direct evidence this fixes an exploitable vulnerability, but the change touches safety-critical state cleanup during channel funding/splicing, where mistakes can lead to stuck funds or inconsistent channel state.
Treat as a state-correctness refactor rather than a confirmed security fix. Review the new boolean polarity at every call site to ensure no call site now incorrectly resets or preserves splice state. Add targeted tests for user-initiated splice cancellation, disconnect during AwaitingSignatures, and tx_abort handling. Monitor project release notes for any follow-up security advisory.
Security signals we found
State-machine change in channel funding/splicing logic
Boolean polarity inversion at multiple call sites
No tests or advisory references supplied
Touches cleanup of pending splice state on disconnect/abort
Subtle reordering of conditional branches in should_reset_pending_splice_state
Evidence from the diff
The patch renames should_reset_pending_splice_state’s parameter from counterparty_aborted to allow_resumption and inverts the boolean passed at most call sites. The function decides whether to reset pending_splice state. The new semantics: allow_resumption=true means ‘preserve state if we are in AwaitingSignatures so we can resume’; allow_resumption=false means ‘reset unless we have already received commitment_signed while awaiting signatures’. The commit message says this is needed because user-initiated cancellation now exists, so the old ‘counterparty_aborted’ framing is too narrow. The diff is a pure refactor of control flow with no new tests or public disclosure in the supplied materials.
Changed components
lightning/src/ln/channel.rsFundedChannel splice negotiation state handlingshould_reset_pending_splice_statereset_pending_splice_statemaybe_fail_splice_negotiationmaybe_splice_funding_failedInspect captured patch +14 / −12
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d37ab2b..3f3a6fe 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1724,7 +1724,7 @@ where
if matches!(chan.context.channel_state, ChannelState::ChannelReady(_)) {
chan.context.channel_state.clear_local_stfu_sent();
chan.context.channel_state.clear_remote_stfu_sent();
- if chan.should_reset_pending_splice_state(false) {
+ if chan.should_reset_pending_splice_state(true) {
// If there was a pending splice negotiation that failed due to disconnecting, we
// also take the opportunity to clean up our state.
let splice_funding_failed = chan.reset_pending_splice_state();
@@ -1841,7 +1841,7 @@ where
None
},
ChannelPhase::Funded(funded_channel) => {
- if funded_channel.should_reset_pending_splice_state(false) {
+ if funded_channel.should_reset_pending_splice_state(true) {
funded_channel.reset_pending_splice_state()
} else {
debug_assert!(false, "We should never fail an interactive funding negotiation once we're exchanging tx_signatures");
@@ -2024,7 +2024,7 @@ where
"Received tx_abort while awaiting tx_signatures exchange".to_owned(),
));
}
- if funded_channel.should_reset_pending_splice_state(true) {
+ if funded_channel.should_reset_pending_splice_state(false) {
let has_funding_negotiation = funded_channel
.pending_splice
.as_ref()
@@ -7159,7 +7159,7 @@ where
fn maybe_fail_splice_negotiation(&mut self) -> Option<SpliceFundingFailed> {
if matches!(self.context.channel_state, ChannelState::ChannelReady(_)) {
- if self.should_reset_pending_splice_state(false) {
+ if self.should_reset_pending_splice_state(true) {
self.reset_pending_splice_state()
} else {
self.abandon_quiescent_action()
@@ -7216,7 +7216,7 @@ where
/// Returns a boolean indicating whether we should reset the splice's
/// [`PendingFunding::funding_negotiation`].
- fn should_reset_pending_splice_state(&self, counterparty_aborted: bool) -> bool {
+ fn should_reset_pending_splice_state(&self, allow_resumption: bool) -> bool {
self.pending_splice
.as_ref()
.map(|pending_splice| {
@@ -7228,7 +7228,11 @@ where
funding_negotiation,
FundingNegotiation::AwaitingSignatures { .. }
);
- if counterparty_aborted {
+ if allow_resumption {
+ // If we want to resume the negotiation after reconnecting, we must be
+ // in [`FundingNegotiation::AwaitingSignatures`] to not reset our state.
+ !is_awaiting_signatures
+ } else {
!is_awaiting_signatures
|| !self
.context()
@@ -7236,8 +7240,6 @@ where
.as_ref()
.expect("We have a pending splice awaiting signatures")
.has_received_commitment_signed()
- } else {
- !is_awaiting_signatures
}
})
.unwrap_or_else(|| {
@@ -7251,7 +7253,7 @@ where
}
fn reset_pending_splice_state(&mut self) -> Option<SpliceFundingFailed> {
- debug_assert!(self.should_reset_pending_splice_state(true));
+ debug_assert!(self.should_reset_pending_splice_state(false));
// Only clear the signing session if the current round is mid-signing. When an earlier
// round completed signing and a later RBF round is in AwaitingAck or
@@ -7325,7 +7327,7 @@ where
}
pub(super) fn maybe_splice_funding_failed(&self) -> Option<SpliceFundingFailed> {
- if !self.should_reset_pending_splice_state(false) {
+ if !self.should_reset_pending_splice_state(true) {
return None;
}
@@ -15647,7 +15649,7 @@ impl<SP: SignerProvider> Writeable for FundedChannel<SP> {
ChannelState::ChannelReady(_) => {
channel_state.clear_local_stfu_sent();
channel_state.clear_remote_stfu_sent();
- if self.should_reset_pending_splice_state(false)
+ if self.should_reset_pending_splice_state(true)
|| !self.has_pending_splice_awaiting_signatures()
{
// We shouldn't be quiescent anymore upon reconnecting if:
@@ -16037,7 +16039,7 @@ impl<SP: SignerProvider> Writeable for FundedChannel<SP> {
// We don't have to worry about resetting the pending `FundingNegotiation` because we
// can only read `FundingNegotiation::AwaitingSignatures` variants anyway.
let pending_splice =
- self.pending_splice.as_ref().filter(|_| !self.should_reset_pending_splice_state(false));
+ self.pending_splice.as_ref().filter(|_| !self.should_reset_pending_splice_state(true));
let monitor_pending_tx_signatures =
self.context.monitor_pending_tx_signatures.then_some(());
Why this scored 34/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.