Pick NegotiationFailureReason at error construction
What the developer wrote
Authored by Jeffrey Czyz
Pick NegotiationFailureReason at error construction
QuiescentError::FailSplice was built with a placeholder
NegotiationFailureReason::Unknown and expected callers to chain a
with_negotiation_failure_reason builder. Sites that forgot the chain
leaked Unknown into Event::SpliceNegotiationFailed, and the pattern
forced splice-specific reason vocabulary into the generic
QuiescentAction helper.
Each call site in propose_quiescence now picks the reason at
construction. The pending-quiescent-action branch is unreachable, so
it asserts unconditionally; the match retains arms for both action
variants so release builds return a sensible error if the invariant
is violated. abandon_quiescent_action returns SpliceFundingFailed
directly without round-tripping through QuiescentError, since the
reason was always discarded there.
Make funding_contributed's pending-quiescent-action check exhaustive
on QuiescentAction. A future variant produces a compile error here
and at the matching arm in propose_quiescence, forcing the author to
decide how it interacts with funding contribution.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
What changed, and why it matters
This commit is a code-quality and correctness fix in a Lightning network library. It removes a risky pattern where an error reason was left as a placeholder ('Unknown') and only filled in later by callers. Some callers forgot to fill it in, so users could see a confusing 'Unknown' reason when a splice negotiation actually failed for a known reason. The change makes the reason mandatory at the moment the error is created, and removes a helper that made it easy to forget. It is not a direct exploit fix, but it prevents a class of state-reporting bugs that could hide what went wrong during channel operations.
Review as a defensive correctness improvement. No urgent deployment required for security reasons, but include in normal release cycle to prevent misleading failure reasons in splice negotiation events. Ensure tests cover both ChannelClosing and Unknown reason paths.
Security signals we found
Elimination of placeholder error reason that could be leaked to event consumers
Removal of builder pattern that allowed callers to forget setting a security-relevant failure reason
Addition of debug_assert! and exhaustive match to enforce invariants at compile time
Refactoring only; no new cryptographic, network, or memory-safety vulnerability introduced
Evidence from the diff
The patch refactors QuiescentError construction in rust-lightning’s channel splicing/quiescence logic. Previously QuiescentError::FailSplice was constructed with NegotiationFailureReason::Unknown and callers were expected to call .with_negotiation_failure_reason(). Some call sites omitted this, leaking Unknown into Event::SpliceNegotiationFailed. The patch deletes the builder method and quiescent_action_into_error(), and instead constructs QuiescentError::FailSplice with the correct NegotiationFailureReason directly at each call site in propose_quiescence. abandon_quiescent_action now returns SpliceFundingFailed directly. The pending-quiescent-action branch is marked unreachable via debug_assert! but kept exhaustive for release builds. funding_contributed’s match on QuiescentAction is made exhaustive so future variants cause compile errors.
Changed components
lightning/src/ln/channel.rslightning/src/ln/splicing_tests.rsQuiescentErrorQuiescentActionpropose_quiescenceabandon_quiescent_actionfunding_contributedEvent::SpliceNegotiationFailedInspect captured patch +50 / −52
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index fde56a9..e119204 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3250,16 +3250,6 @@ pub(super) enum QuiescentError {
FailSplice(SpliceFundingFailed, NegotiationFailureReason),
}
-impl QuiescentError {
- fn with_negotiation_failure_reason(mut self, reason: NegotiationFailureReason) -> Self {
- match self {
- QuiescentError::FailSplice(_, ref mut r) => *r = reason,
- _ => debug_assert!(false, "Expected FailSplice variant"),
- }
- self
- }
-}
-
pub(crate) enum StfuResponse {
Stfu(msgs::Stfu),
SpliceInit(msgs::SpliceInit),
@@ -7217,27 +7207,13 @@ where
.expect("is_initiator is true so this always returns Some")
}
- fn quiescent_action_into_error(&self, action: QuiescentAction) -> QuiescentError {
- match action {
- QuiescentAction::Splice { contribution, .. } => QuiescentError::FailSplice(
- self.splice_funding_failed_for(contribution),
- NegotiationFailureReason::Unknown,
- ),
- #[cfg(any(test, fuzzing, feature = "_test_utils"))]
- QuiescentAction::DoNothing => QuiescentError::DoNothing,
- }
- }
-
fn abandon_quiescent_action(&mut self) -> Option<SpliceFundingFailed> {
- let action = self.quiescent_action.take()?;
- match self.quiescent_action_into_error(action) {
- QuiescentError::FailSplice(failed, _) => Some(failed),
- #[cfg(any(test, fuzzing, feature = "_test_utils"))]
- QuiescentError::DoNothing => None,
- _ => {
- debug_assert!(false);
- None
+ match self.quiescent_action.take()? {
+ QuiescentAction::Splice { contribution, .. } => {
+ Some(self.splice_funding_failed_for(contribution))
},
+ #[cfg(any(test, fuzzing, feature = "_test_utils"))]
+ QuiescentAction::DoNothing => None,
}
}
@@ -12734,22 +12710,28 @@ where
) -> Result<Option<msgs::Stfu>, QuiescentError> {
debug_assert!(contribution.is_splice());
- if let Some(QuiescentAction::Splice { contribution: existing, .. }) = &self.quiescent_action
- {
- let pending_splice = self.pending_splice.as_ref();
- let prior_inputs = pending_splice
- .into_iter()
- .flat_map(|pending_splice| pending_splice.contributed_inputs());
- let prior_outputs = pending_splice
- .into_iter()
- .flat_map(|pending_splice| pending_splice.contributed_outputs());
- return match contribution.into_unique_contributions(
- existing.contributed_inputs().chain(prior_inputs),
- existing.contributed_outputs().chain(prior_outputs),
- ) {
- None => Err(QuiescentError::DoNothing),
- Some((inputs, outputs)) => Err(QuiescentError::DiscardFunding { inputs, outputs }),
- };
+ match self.quiescent_action.as_ref() {
+ Some(QuiescentAction::Splice { contribution: existing, .. }) => {
+ let pending_splice = self.pending_splice.as_ref();
+ let prior_inputs = pending_splice
+ .into_iter()
+ .flat_map(|pending_splice| pending_splice.contributed_inputs());
+ let prior_outputs = pending_splice
+ .into_iter()
+ .flat_map(|pending_splice| pending_splice.contributed_outputs());
+ return match contribution.into_unique_contributions(
+ existing.contributed_inputs().chain(prior_inputs),
+ existing.contributed_outputs().chain(prior_outputs),
+ ) {
+ None => Err(QuiescentError::DoNothing),
+ Some((inputs, outputs)) => {
+ Err(QuiescentError::DiscardFunding { inputs, outputs })
+ },
+ };
+ },
+ #[cfg(any(test, fuzzing, feature = "_test_utils"))]
+ Some(QuiescentAction::DoNothing) => unreachable!(),
+ None => {},
}
let initiated_funding_negotiation = self
@@ -14396,9 +14378,6 @@ where
) -> Result<Option<msgs::Stfu>, QuiescentError> {
log_debug!(logger, "Attempting to initiate quiescence");
- // TODO: NegotiationFailureReason is splice-specific, but propose_quiescence is
- // generic. The reason should be selected by the caller, but it currently can't
- // distinguish why quiescence failed. Revisit when a second quiescent protocol is added.
if !self.context.is_usable() {
debug_assert!(
self.context.channel_state.is_local_shutdown_sent()
@@ -14406,15 +14385,34 @@ where
"splice_channel should have prevented reaching propose_quiescence on a non-ready channel"
);
log_debug!(logger, "Channel is not in a usable state to propose quiescence");
- return Err(self.quiescent_action_into_error(action)
- .with_negotiation_failure_reason(NegotiationFailureReason::ChannelClosing));
+ return Err(match action {
+ QuiescentAction::Splice { contribution, .. } => QuiescentError::FailSplice(
+ self.splice_funding_failed_for(contribution),
+ NegotiationFailureReason::ChannelClosing,
+ ),
+ #[cfg(any(test, fuzzing, feature = "_test_utils"))]
+ QuiescentAction::DoNothing => QuiescentError::DoNothing,
+ });
}
+
if self.quiescent_action.is_some() {
+ debug_assert!(
+ false,
+ "callers must not invoke propose_quiescence with {:?} while quiescent_action is set",
+ action,
+ );
log_debug!(
logger,
"Channel already has a pending quiescent action and cannot start another",
);
- return Err(self.quiescent_action_into_error(action));
+ return Err(match action {
+ #[cfg(any(test, fuzzing, feature = "_test_utils"))]
+ QuiescentAction::DoNothing => QuiescentError::DoNothing,
+ QuiescentAction::Splice { contribution, .. } => QuiescentError::FailSplice(
+ self.splice_funding_failed_for(contribution),
+ NegotiationFailureReason::Unknown,
+ ),
+ });
}
// Since we don't have a pending quiescent action, we should never be in a state where we
// sent `stfu` without already having become quiescent.
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index a6823e3..519d52a 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -3847,7 +3847,7 @@ fn do_abandon_splice_quiescent_action_on_shutdown(local_shutdown: bool, pending_
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_capacity, 0);
// When testing with a prior pending splice, complete splice A first so that
- // `quiescent_action_into_error` filters against `pending_splice.contributed_inputs/outputs`.
+ // `splice_funding_failed_for` filters against `pending_splice.contributed_inputs/outputs`.
if pending_splice {
let funding_contribution = do_initiate_splice_in(
&nodes[0],
Why this scored 27/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.