Avoid resetting splice state on FundingNegotiation::AwaitingSignatures
What changed, and why it matters
This commit fixes a bug in how the Lightning Dev Kit handles interrupted splice negotiations. Previously, if a splice negotiation reached the 'AwaitingSignatures' stage and the connection dropped, the node would incorrectly reset its splice state on reconnection. That prevented the channel from ever resuming the pending splice negotiation via the standard channel_reestablish flow. The fix stops the reset in that specific stage and simplifies the related state-cleanup logic. It is a protocol-correctness fix rather than a direct theft-of-funds vulnerability, but it could cause channels to become stuck or force unnecessary closures.
Review the new state-machine transitions for FundingNegotiation and verify that AwaitingSignatures is preserved across disconnect/reconnect in both directions. Run the updated splicing_tests and add coverage for the AwaitingSignatures reconnection case if not already present. Consider whether any nodes already running the buggy code need migration guidance for stuck splice states.
Security signals we found
State-machine bug in splice negotiation lifecycle
Incorrect reset of pending splice state on reconnection
Potential channel stall or forced close after reconnection
Protocol-correctness fix for channel_reestablish handling
Evidence from the diff
The patch changes FundedChannel splice-state cleanup so that FundingNegotiation::AwaitingSignatures is no longer discarded on reconnection. It merges should_reset_pending_splice_funding_negotiation into PendingFunding::can_abandon_state, adds has_pending_splice_awaiting_signatures, and updates channel_reestablish and serialization paths to preserve pending splice state when signatures are awaited. reset_pending_splice_state now also asserts that no interactive_tx_signing_session exists. A test is updated to abort mid-construction instead of completing the negotiation.
Changed components
lightning/src/ln/channel.rslightning/src/ln/splicing_tests.rsFundedChannel splice state managementFundingNegotiation::AwaitingSignatures handlingchannel_reestablish logicInspect captured patch +59 / −76
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 4eb5513..d66ddc9 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1694,8 +1694,7 @@ where
pending_v2_channel.interactive_tx_constructor.take();
},
ChannelPhase::Funded(funded_channel) => {
- if funded_channel.should_reset_pending_splice_funding_negotiation().unwrap_or(true)
- {
+ if funded_channel.should_reset_pending_splice_state() {
funded_channel.reset_pending_splice_state();
} else {
debug_assert!(false, "We should never fail an interactive funding negotiation once we're exchanging tx_signatures");
@@ -1829,18 +1828,15 @@ where
pending_v2_channel.interactive_tx_constructor.take().is_some()
},
ChannelPhase::Funded(funded_channel) => {
- if let Some(should_reset) =
- funded_channel.should_reset_pending_splice_funding_negotiation()
- {
- if should_reset {
- // We may have still tracked the pending funding negotiation state, so we
- // should ack with our own `tx_abort`.
- funded_channel.reset_pending_splice_state()
- } else {
- return Err(ChannelError::close(
- "Received tx_abort while awaiting tx_signatures exchange".to_owned(),
- ));
- }
+ if funded_channel.has_pending_splice_awaiting_signatures() {
+ return Err(ChannelError::close(
+ "Received tx_abort while awaiting tx_signatures exchange".to_owned(),
+ ));
+ }
+ if funded_channel.should_reset_pending_splice_state() {
+ let has_funding_negotiation = funded_channel.reset_pending_splice_state();
+ debug_assert!(has_funding_negotiation);
+ true
} else {
// We were not tracking the pending funding negotiation state anymore, likely
// due to a disconnection or already having sent our own `tx_abort`.
@@ -2583,13 +2579,17 @@ impl FundingNegotiation {
}
impl PendingFunding {
- fn can_abandon_funding_negotiation(&self) -> bool {
+ fn can_abandon_state(&self) -> bool {
self.funding_negotiation
.as_ref()
.map(|funding_negotiation| {
!matches!(funding_negotiation, FundingNegotiation::AwaitingSignatures { .. })
})
- .unwrap_or(true)
+ .unwrap_or_else(|| {
+ let has_negotiated_candidates = !self.negotiated_candidates.is_empty();
+ debug_assert!(has_negotiated_candidates);
+ !has_negotiated_candidates
+ })
}
fn check_get_splice_locked<SP: Deref>(
@@ -6773,40 +6773,35 @@ where
)
}
- /// Returns `None` if there is no [`FundedChannel::pending_splice`], otherwise a boolean
- /// indicating whether we should reset the splice's [`PendingFunding::funding_negotiation`].
- fn should_reset_pending_splice_funding_negotiation(&self) -> Option<bool> {
- self.pending_splice.as_ref().map(|pending_splice| {
- if pending_splice.can_abandon_funding_negotiation() {
- true
- } else {
- self.context
- .interactive_tx_signing_session
- .as_ref()
- .map(|signing_session| !signing_session.has_received_commitment_signed())
- .unwrap_or_else(|| {
- debug_assert!(false);
- false
- })
- }
- })
+ fn has_pending_splice_awaiting_signatures(&self) -> bool {
+ self.pending_splice
+ .as_ref()
+ .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref())
+ .map(|funding_negotiation| {
+ matches!(funding_negotiation, FundingNegotiation::AwaitingSignatures { .. })
+ })
+ .unwrap_or(false)
}
+ /// Returns a boolean indicating whether we should reset the splice's
+ /// [`PendingFunding::funding_negotiation`].
fn should_reset_pending_splice_state(&self) -> bool {
- self.should_reset_pending_splice_funding_negotiation().unwrap_or(true)
- && self.pending_funding().is_empty()
+ self.pending_splice
+ .as_ref()
+ .map(|pending_splice| pending_splice.can_abandon_state())
+ .unwrap_or(false)
}
fn reset_pending_splice_state(&mut self) -> bool {
- debug_assert!(self.should_reset_pending_splice_funding_negotiation().unwrap_or(true));
+ debug_assert!(self.should_reset_pending_splice_state());
+ debug_assert!(self.context.interactive_tx_signing_session.is_none());
self.context.channel_state.clear_quiescent();
- self.context.interactive_tx_signing_session.take();
let has_funding_negotiation = self
.pending_splice
.as_mut()
.and_then(|pending_splice| pending_splice.funding_negotiation.take())
.is_some();
- if self.should_reset_pending_splice_state() {
+ if self.pending_funding().is_empty() {
self.pending_splice.take();
}
has_funding_negotiation
@@ -8948,13 +8943,16 @@ where
}
self.context.channel_state.clear_local_stfu_sent();
self.context.channel_state.clear_remote_stfu_sent();
- if self.should_reset_pending_splice_funding_negotiation().unwrap_or(true) {
- // If we were in quiescence but a splice was never negotiated, or the negotiation
- // failed due to disconnecting, we shouldn't be quiescent anymore upon reconnecting.
- // If there was a pending splice negotiation that has failed due to disconnecting,
- // we also take the opportunity to clean up our state.
+ if self.should_reset_pending_splice_state() {
+ // If there was a pending splice negotiation that failed due to disconnecting, we
+ // also take the opportunity to clean up our state.
self.reset_pending_splice_state();
debug_assert!(!self.context.channel_state.is_quiescent());
+ } else if !self.has_pending_splice_awaiting_signatures() {
+ // We shouldn't be quiescent anymore upon reconnecting if:
+ // - We were in quiescence but a splice/RBF was never negotiated or
+ // - We were in quiescence but the splice negotiation failed due to disconnecting
+ self.context.channel_state.clear_quiescent();
}
}
@@ -13993,10 +13991,13 @@ where
}
channel_state.clear_local_stfu_sent();
channel_state.clear_remote_stfu_sent();
- if self.should_reset_pending_splice_funding_negotiation().unwrap_or(true) {
- // If we were in quiescence but a splice was never negotiated, or the
- // negotiation failed due to disconnecting, we shouldn't be quiescent
- // anymore upon reconnecting.
+ if self.should_reset_pending_splice_state()
+ || !self.has_pending_splice_awaiting_signatures()
+ {
+ // We shouldn't be quiescent anymore upon reconnecting if:
+ // - We were in quiescence but a splice/RBF was never negotiated or
+ // - We were in quiescence but the splice negotiation failed due to
+ // disconnecting
channel_state.clear_quiescent();
}
},
@@ -14361,19 +14362,10 @@ where
let holder_commitment_point_next = self.holder_commitment_point.next_point();
let holder_commitment_point_pending_next = self.holder_commitment_point.pending_next_point;
- let interactive_tx_signing_session =
- if self.should_reset_pending_splice_funding_negotiation().unwrap_or(false) {
- None
- } else {
- self.context.interactive_tx_signing_session.as_ref()
- };
- let pending_splice = if self.should_reset_pending_splice_state() {
- None
- } else {
- // We don't have to worry about resetting the pending `FundingNegotiation` because we
- // can only read `FundingNegotiation::AwaitingSignatures` variants anyway.
- self.pending_splice.as_ref()
- };
+ // 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());
write_tlv_fields!(writer, {
(0, self.context.announcement_sigs, option),
@@ -14418,7 +14410,7 @@ where
(53, funding_tx_broadcast_safe_event_emitted, option), // Added in 0.0.124
(55, removed_htlc_attribution_data, optional_vec), // Added in 0.2
(57, holding_cell_attribution_data, optional_vec), // Added in 0.2
- (58, interactive_tx_signing_session, option), // Added in 0.2
+ (58, self.context.interactive_tx_signing_session, option), // Added in 0.2
(59, self.funding.minimum_depth_override, option), // Added in 0.2
(60, self.context.historical_scids, optional_vec), // Added in 0.2
(61, fulfill_attribution_data, optional_vec), // Added in 0.2
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index a7d5744..1384281 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -414,9 +414,9 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) {
)
.unwrap();
- // Attempt a splice negotiation that only goes up to exchanging `tx_complete`. Reconnecting
- // should implicitly abort the negotiation and reset the splice state such that we're able to
- // retry another splice later.
+ // Attempt a splice negotiation that ends mid-construction of the funding transaction.
+ // Reconnecting should implicitly abort the negotiation and reset the splice state such that
+ // we're able to retry another splice later.
let stfu = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1);
nodes[1].node.handle_stfu(node_id_0, &stfu);
let stfu = get_event_msg!(nodes[1], MessageSendEvent::SendStfu, node_id_0);
@@ -427,18 +427,9 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) {
let splice_ack = get_event_msg!(nodes[1], MessageSendEvent::SendSpliceAck, node_id_0);
nodes[0].node.handle_splice_ack(node_id_1, &splice_ack);
- let new_funding_script = chan_utils::make_funding_redeemscript(
- &splice_init.funding_pubkey,
- &splice_ack.funding_pubkey,
- )
- .to_p2wsh();
- let _ = complete_interactive_funding_negotiation(
- &nodes[0],
- &nodes[1],
- channel_id,
- contribution.clone(),
- new_funding_script,
- );
+ let tx_add_input = get_event_msg!(nodes[0], MessageSendEvent::SendTxAddInput, node_id_1);
+ nodes[1].node.handle_tx_add_input(node_id_0, &tx_add_input);
+ let _ = get_event_msg!(nodes[1], MessageSendEvent::SendTxComplete, node_id_0);
if reload {
let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();
Why this scored 33/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.