Wipe splice state upon failed interactive funding construction
What changed, and why it matters
This commit fixes a state cleanup bug in the Lightning Dev Kit's handling of failed or aborted splice negotiations. Previously, if a peer disconnected during a splice negotiation, the channel could be left in a 'quiescent' (frozen) state and retain leftover splice data, preventing future splice attempts and possibly blocking normal payments. The patch ensures that when a splice fails before signatures are exchanged, all related temporary state is cleared both on disconnect and when a tx_abort message is received, and it adds tests for these scenarios.
Review the new reset helpers for correctness, especially the conditions under which pending_splice is fully dropped versus only funding_negotiation. Ensure the tx_abort safety path cannot be abused to force unilateral channel closes. Run the new disconnect/reload tests and consider fuzzing the splice state machine around disconnect and tx_abort boundaries.
Security signals we found
State cleanup bug in interactive funding / splice negotiation
Channel could remain quiescent after disconnect, blocking normal updates
Leftover pending_splice state could prevent future splice attempts
New safety check closes channel on tx_abort during signature exchange
Serialization hardened to reject non-AwaitingSignatures splice funding state on load
Evidence from the diff
The change centralizes cleanup of interactive-transaction and splice state in FundedChannel. It introduces helpers should_reset_pending_splice_funding_negotiation, should_reset_pending_splice_state, and reset_pending_splice_state, which clear the quiescent flag, the interactive_tx_signing_session, the pending FundingNegotiation, and (when safe) the entire pending_splice. The cleanup now runs on disconnect and on tx_abort, not only on explicit tx_abort. A safety check closes the channel if tx_abort is received while awaiting tx_signatures exchange. Serialization is updated so that only AwaitingSignatures splice state is persisted. New tests verify quiescence termination and splice state reset on disconnect/reload.
Changed components
lightning/src/ln/channel.rslightning/src/ln/quiescence_tests.rslightning/src/ln/splicing_tests.rsInspect captured patch +372 / −38
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 1ca067f..2cd1b04 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1686,27 +1686,20 @@ where
let logger = WithChannelContext::from(logger, &self.context(), None);
log_info!(logger, "Failed interactive transaction negotiation: {reason}");
- let _interactive_tx_constructor = match &mut self.phase {
+ match &mut self.phase {
ChannelPhase::Undefined => unreachable!(),
- ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => None,
+ ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => {},
ChannelPhase::UnfundedV2(pending_v2_channel) => {
- pending_v2_channel.interactive_tx_constructor.take()
+ pending_v2_channel.interactive_tx_constructor.take();
+ },
+ ChannelPhase::Funded(funded_channel) => {
+ if funded_channel.should_reset_pending_splice_funding_negotiation().unwrap_or(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");
+ }
},
- ChannelPhase::Funded(funded_channel) => funded_channel
- .pending_splice
- .as_mut()
- .and_then(|pending_splice| pending_splice.funding_negotiation.take())
- .and_then(|funding_negotiation| {
- if let FundingNegotiation::ConstructingTransaction {
- interactive_tx_constructor,
- ..
- } = funding_negotiation
- {
- Some(interactive_tx_constructor)
- } else {
- None
- }
- }),
};
reason.into_tx_abort_msg(self.context().channel_id)
@@ -1818,11 +1811,11 @@ where
where
L::Target: Logger,
{
- // This checks for and resets the interactive negotiation state by `take()`ing it from the channel.
- // The existence of the `tx_constructor` indicates that we have not moved into the signing
- // phase for this interactively constructed transaction and hence we have not exchanged
- // `tx_signatures`. Either way, we never close the channel upon receiving a `tx_abort`:
- // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L574-L576
+ // If we have not sent a `tx_abort` message for this negotiation previously, we need to echo
+ // back a tx_abort message according to the spec:
+ // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L560-L561
+ // For rationale why we echo back `tx_abort`:
+ // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L578-L580
let should_ack = match &mut self.phase {
ChannelPhase::Undefined => unreachable!(),
ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => {
@@ -1832,19 +1825,27 @@ where
ChannelPhase::UnfundedV2(pending_v2_channel) => {
pending_v2_channel.interactive_tx_constructor.take().is_some()
},
- ChannelPhase::Funded(funded_channel) => funded_channel
- .pending_splice
- .as_mut()
- .and_then(|pending_splice| pending_splice.funding_negotiation.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(),
+ ));
+ }
+ } else {
+ // We were not tracking the pending funding negotiation state anymore, likely
+ // due to a disconnection or already having sent our own `tx_abort`.
+ false
+ }
+ },
};
- // NOTE: Since at this point we have not sent a `tx_abort` message for this negotiation
- // previously (tx_constructor was `Some`), we need to echo back a tx_abort message according
- // to the spec:
- // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L560-L561
- // For rationale why we echo back `tx_abort`:
- // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L578-L580
Ok(should_ack.then(|| {
let logger = WithChannelContext::from(logger, &self.context(), None);
let reason =
@@ -2554,6 +2555,15 @@ impl FundingNegotiation {
}
impl PendingFunding {
+ fn can_abandon_funding_negotiation(&self) -> bool {
+ self.funding_negotiation
+ .as_ref()
+ .map(|funding_negotiation| {
+ !matches!(funding_negotiation, FundingNegotiation::AwaitingSignatures { .. })
+ })
+ .unwrap_or(true)
+ }
+
fn check_get_splice_locked<SP: Deref>(
&mut self, context: &ChannelContext<SP>, confirmed_funding_index: usize, height: u32,
) -> Option<msgs::SpliceLocked>
@@ -6739,6 +6749,45 @@ 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 should_reset_pending_splice_state(&self) -> bool {
+ self.should_reset_pending_splice_funding_negotiation().unwrap_or(true)
+ && self.pending_funding().is_empty()
+ }
+
+ fn reset_pending_splice_state(&mut self) -> bool {
+ debug_assert!(self.should_reset_pending_splice_funding_negotiation().unwrap_or(true));
+ 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() {
+ self.pending_splice.take();
+ }
+ has_funding_negotiation
+ }
+
#[rustfmt::skip]
fn check_remote_fee<F: Deref, L: Deref>(
channel_type: &ChannelTypeFeatures, fee_estimator: &LowerBoundedFeeEstimator<F>,
@@ -8864,7 +8913,14 @@ where
}
self.context.channel_state.clear_local_stfu_sent();
self.context.channel_state.clear_remote_stfu_sent();
- self.context.channel_state.clear_quiescent();
+ 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.
+ self.reset_pending_splice_state();
+ debug_assert!(!self.context.channel_state.is_quiescent());
+ }
}
self.context.channel_state.set_peer_disconnected();
@@ -13875,7 +13931,12 @@ where
}
channel_state.clear_local_stfu_sent();
channel_state.clear_remote_stfu_sent();
- channel_state.clear_quiescent();
+ 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.
+ channel_state.clear_quiescent();
+ }
},
ChannelState::FundingNegotiated(_)
if self.context.interactive_tx_signing_session.is_some() => {},
@@ -14238,6 +14299,20 @@ 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()
+ };
+
write_tlv_fields!(writer, {
(0, self.context.announcement_sigs, option),
// minimum_depth and counterparty_selected_channel_reserve_satoshis used to have a
@@ -14281,12 +14356,12 @@ 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, self.context.interactive_tx_signing_session, option), // Added in 0.2
+ (58, 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
(63, holder_commitment_point_current, option), // Added in 0.2
- (64, self.pending_splice, option), // Added in 0.2
+ (64, pending_splice, option), // Added in 0.2
(65, self.quiescent_action, option), // Added in 0.2
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
@@ -14950,6 +15025,15 @@ where
}
};
+ if let Some(funding_negotiation) = pending_splice
+ .as_ref()
+ .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref())
+ {
+ if !matches!(funding_negotiation, FundingNegotiation::AwaitingSignatures { .. }) {
+ return Err(DecodeError::InvalidValue);
+ }
+ }
+
Ok(FundedChannel {
funding: FundingScope {
value_to_self_msat,
diff --git a/lightning/src/ln/quiescence_tests.rs b/lightning/src/ln/quiescence_tests.rs
index 983fd8b..63c5009 100644
--- a/lightning/src/ln/quiescence_tests.rs
+++ b/lightning/src/ln/quiescence_tests.rs
@@ -7,6 +7,7 @@ use crate::ln::functional_test_utils::*;
use crate::ln::msgs;
use crate::ln::msgs::{BaseMessageHandler, ChannelMessageHandler, ErrorAction, MessageSendEvent};
use crate::util::errors::APIError;
+use crate::util::ser::Writeable;
use crate::util::test_channel_signer::SignerOp;
#[test]
@@ -699,3 +700,62 @@ fn test_quiescence_during_disconnection() {
do_test_quiescence_during_disconnection(false, true);
do_test_quiescence_during_disconnection(true, true);
}
+
+#[test]
+fn test_quiescence_termination_on_disconnect() {
+ do_test_quiescence_termination_on_disconnect(false);
+ do_test_quiescence_termination_on_disconnect(true);
+}
+
+fn do_test_quiescence_termination_on_disconnect(reload: bool) {
+ // Tests that we terminate quiescence on disconnect if there's no quiescence protocol taking place.
+ let chanmon_cfgs = create_chanmon_cfgs(2);
+ let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+ let (persister_0a, persister_1a);
+ let (chain_monitor_0a, chain_monitor_1a);
+ let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+ let (node_0a, node_1a);
+ let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+ let node_id_0 = nodes[0].node.get_our_node_id();
+ let node_id_1 = nodes[1].node.get_our_node_id();
+
+ let (_, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
+
+ nodes[0].node.maybe_propose_quiescence(&node_id_1, &channel_id).unwrap();
+
+ 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);
+ nodes[0].node.handle_stfu(node_id_1, &stfu);
+
+ if reload {
+ let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();
+ reload_node!(
+ nodes[0],
+ &nodes[0].node.encode(),
+ &[&encoded_monitor_0],
+ persister_0a,
+ chain_monitor_0a,
+ node_0a
+ );
+ let encoded_monitor_1 = get_monitor!(nodes[1], channel_id).encode();
+ reload_node!(
+ nodes[1],
+ &nodes[1].node.encode(),
+ &[&encoded_monitor_1],
+ persister_1a,
+ chain_monitor_1a,
+ node_1a
+ );
+ } else {
+ nodes[0].node.peer_disconnected(node_id_1);
+ nodes[1].node.peer_disconnected(node_id_0);
+ }
+
+ let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
+ reconnect_args.send_channel_ready = (true, true);
+ reconnect_nodes(reconnect_args);
+
+ send_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+}
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index 1537a36..88271d7 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -19,6 +19,7 @@ use crate::ln::funding::{FundingTxInput, SpliceContribution};
use crate::ln::msgs::{self, BaseMessageHandler, ChannelMessageHandler, MessageSendEvent};
use crate::ln::types::ChannelId;
use crate::util::errors::APIError;
+use crate::util::ser::Writeable;
use bitcoin::{Amount, OutPoint as BitcoinOutPoint, ScriptBuf, Transaction, TxOut};
@@ -315,6 +316,195 @@ fn lock_splice_after_blocks<'a, 'b, 'c, 'd>(
node_b.chain_source.remove_watched_txn_and_outputs(prev_funding_outpoint, prev_funding_script);
}
+#[test]
+fn test_splice_state_reset_on_disconnect() {
+ do_test_splice_state_reset_on_disconnect(false);
+ do_test_splice_state_reset_on_disconnect(true);
+}
+
+fn do_test_splice_state_reset_on_disconnect(reload: bool) {
+ // Tests that we're able to forget our pending splice state after a disconnect such that we can
+ // retry later.
+ let chanmon_cfgs = create_chanmon_cfgs(2);
+ let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+ let (persister_0a, persister_0b, persister_0c, persister_1a, persister_1b, persister_1c);
+ let (
+ chain_monitor_0a,
+ chain_monitor_0b,
+ chain_monitor_0c,
+ chain_monitor_1a,
+ chain_monitor_1b,
+ chain_monitor_1c,
+ );
+ let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+ let (node_0a, node_0b, node_0c, node_1a, node_1b, node_1c);
+ let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+ let node_id_0 = nodes[0].node.get_our_node_id();
+ let node_id_1 = nodes[1].node.get_our_node_id();
+
+ let (_, _, channel_id, _) =
+ create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 50_000_000);
+
+ let contribution = SpliceContribution::SpliceOut {
+ outputs: vec![TxOut {
+ value: Amount::from_sat(1_000),
+ script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
+ }],
+ };
+ nodes[0]
+ .node
+ .splice_channel(
+ &channel_id,
+ &node_id_1,
+ contribution.clone(),
+ FEERATE_FLOOR_SATS_PER_KW,
+ None,
+ )
+ .unwrap();
+
+ // Attempt a splice negotiation that only goes up to receiving `splice_init`. 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);
+ nodes[0].node.handle_stfu(node_id_1, &stfu);
+
+ let splice_init = get_event_msg!(nodes[0], MessageSendEvent::SendSpliceInit, node_id_1);
+ nodes[1].node.handle_splice_init(node_id_0, &splice_init);
+ let _ = get_event_msg!(nodes[1], MessageSendEvent::SendSpliceAck, node_id_0);
+
+ if reload {
+ let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();
+ reload_node!(
+ nodes[0],
+ &nodes[0].node.encode(),
+ &[&encoded_monitor_0],
+ persister_0a,
+ chain_monitor_0a,
+ node_0a
+ );
+ let encoded_monitor_1 = get_monitor!(nodes[1], channel_id).encode();
+ reload_node!(
+ nodes[1],
+ &nodes[1].node.encode(),
+ &[&encoded_monitor_1],
+ persister_1a,
+ chain_monitor_1a,
+ node_1a
+ );
+ } else {
+ nodes[0].node.peer_disconnected(node_id_1);
+ nodes[1].node.peer_disconnected(node_id_0);
+ }
+
+ let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
+ reconnect_args.send_channel_ready = (true, true);
+ reconnect_nodes(reconnect_args);
+
+ nodes[0]
+ .node
+ .splice_channel(
+ &channel_id,
+ &node_id_1,
+ contribution.clone(),
+ FEERATE_FLOOR_SATS_PER_KW,
+ None,
+ )
+ .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.
+ 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);
+ nodes[0].node.handle_stfu(node_id_1, &stfu);
+
+ let splice_init = get_event_msg!(nodes[0], MessageSendEvent::SendSpliceInit, node_id_1);
+ nodes[1].node.handle_splice_init(node_id_0, &splice_init);
+ 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,
+ );
+
+ if reload {
+ let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();
+ reload_node!(
+ nodes[0],
+ &nodes[0].node.encode(),
+ &[&encoded_monitor_0],
+ persister_0b,
+ chain_monitor_0b,
+ node_0b
+ );
+ let encoded_monitor_1 = get_monitor!(nodes[1], channel_id).encode();
+ reload_node!(
+ nodes[1],
+ &nodes[1].node.encode(),
+ &[&encoded_monitor_1],
+ persister_1b,
+ chain_monitor_1b,
+ node_1b
+ );
+ } else {
+ nodes[0].node.peer_disconnected(node_id_1);
+ nodes[1].node.peer_disconnected(node_id_0);
+ }
+
+ let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
+ reconnect_args.send_channel_ready = (true, true);
+ reconnect_nodes(reconnect_args);
+
+ // Attempt a splice negotiation that completes, (i.e. `tx_signatures` are exchanged). Reconnecting
+ // should not abort the negotiation or reset the splice state.
+ let splice_tx = splice_channel(&nodes[0], &nodes[1], channel_id, contribution);
+
+ if reload {
+ let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();
+ reload_node!(
+ nodes[0],
+ &nodes[0].node.encode(),
+ &[&encoded_monitor_0],
+ persister_0c,
+ chain_monitor_0c,
+ node_0c
+ );
+ let encoded_monitor_1 = get_monitor!(nodes[1], channel_id).encode();
+ reload_node!(
+ nodes[1],
+ &nodes[1].node.encode(),
+ &[&encoded_monitor_1],
+ persister_1c,
+ chain_monitor_1c,
+ node_1c
+ );
+ } else {
+ nodes[0].node.peer_disconnected(node_id_1);
+ nodes[1].node.peer_disconnected(node_id_0);
+ }
+
+ let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
+ reconnect_args.send_channel_ready = (true, true);
+ reconnect_nodes(reconnect_args);
+
+ mine_transaction(&nodes[0], &splice_tx);
+ mine_transaction(&nodes[1], &splice_tx);
+ lock_splice_after_blocks(&nodes[0], &nodes[1], channel_id, ANTI_REORG_DELAY - 1);
+}
+
#[test]
fn test_splice_in() {
let chanmon_cfgs = create_chanmon_cfgs(2);
Why this scored 44/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.