Finish validation in `splice_ack` before taking a `&mut self`
What the developer wrote
Authored by Leo Nash
Finish validation in `splice_ack` before taking a `&mut self`
As much as possible, we want to only mutate state once we are done with
input validation.
This also removes complaints when helper functions during validation
take a `&self`.
What changed, and why it matters
This change is a defensive code-quality refactor in a Lightning channel splicing feature. It moves input validation earlier so the function only mutates internal state after all checks pass. The commit message frames this as a hardening measure, but the diff itself does not fix a known exploitable bug or change observable behavior under normal operation.
Treat as a low-risk hardening improvement. No urgent action required. Reviewers may want to confirm no other splice-related functions mutate state before finishing validation, and ensure test coverage exists for the `ConstructingTransaction` and `AwaitingSignatures` error paths.
Security signals we found
State mutation deferred until after input validation
Removal of error-path state restoration for `funding_negotiation`
Use of immutable borrows during validation to prevent accidental mutation
Commit message explicitly describes the change as a security-oriented hardening practice
Evidence from the diff
In Channel::splice_ack, the original code took a mutable borrow of self.pending_splice at the start, then performed state-machine validation and sometimes restored the taken funding_negotiation value on error. The patch restructures the code to validate using immutable references (as_ref() and pattern matching) and only acquire &mut self after validation succeeds. This eliminates the need to restore state on error paths and reduces the risk of future bugs where partial mutation could occur before a validation failure. The functional error cases remain identical.
Changed components
lightning/src/ln/channel.rsChannel::splice_ackLightning splicing protocol handlingInspect captured patch +19 / −19
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 3d25934..732eedd 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -11451,26 +11451,17 @@ where
ES::Target: EntropySource,
L::Target: Logger,
{
- let pending_splice = if let Some(ref mut pending_splice) = &mut self.pending_splice {
- pending_splice
- } else {
- return Err(ChannelError::Ignore(format!("Channel is not in pending splice")));
- };
-
// TODO(splicing): Add check that we are the splice (quiescence) initiator
- let funding_negotiation_context = match pending_splice.funding_negotiation.take() {
+ let funding_negotiation_context = match &self
+ .pending_splice
+ .as_ref()
+ .ok_or(ChannelError::Ignore(format!("Channel is not in pending splice")))?
+ .funding_negotiation
+ {
Some(FundingNegotiation::AwaitingAck(context)) => context,
- Some(FundingNegotiation::ConstructingTransaction(funding, constructor)) => {
- pending_splice.funding_negotiation =
- Some(FundingNegotiation::ConstructingTransaction(funding, constructor));
- return Err(ChannelError::WarnAndDisconnect(format!(
- "Got unexpected splice_ack; splice negotiation already in progress"
- )));
- },
- Some(FundingNegotiation::AwaitingSignatures(funding)) => {
- pending_splice.funding_negotiation =
- Some(FundingNegotiation::AwaitingSignatures(funding));
+ Some(FundingNegotiation::ConstructingTransaction(_, _))
+ | Some(FundingNegotiation::AwaitingSignatures(_)) => {
return Err(ChannelError::WarnAndDisconnect(format!(
"Got unexpected splice_ack; splice negotiation already in progress"
)));
@@ -11507,6 +11498,17 @@ where
self.funding.get_value_satoshis(),
);
+ let pending_splice =
+ self.pending_splice.as_mut().expect("We should have returned an error earlier!");
+ // TODO: Good candidate for a let else statement once MSRV >= 1.65
+ let funding_negotiation_context = if let Some(FundingNegotiation::AwaitingAck(context)) =
+ pending_splice.funding_negotiation.take()
+ {
+ context
+ } else {
+ panic!("We should have returned an error earlier!");
+ };
+
let mut interactive_tx_constructor = funding_negotiation_context
.into_interactive_tx_constructor(
&self.context,
@@ -11525,8 +11527,6 @@ where
debug_assert!(self.interactive_tx_signing_session.is_none());
- let pending_splice =
- self.pending_splice.as_mut().expect("pending_splice should still be set");
pending_splice.funding_negotiation = Some(FundingNegotiation::ConstructingTransaction(
splice_funding,
interactive_tx_constructor,
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.