From 6d2b110ac6c9844d651644a44eabda87d0dcd721 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 23 Sep 2025 17:06:55 -0700 Subject: [PATCH] Wipe splice state upon failed interactive funding construction An interactive funding construction can be considered failed upon a disconnect or a `tx_abort` message. So far, we've consumed the `InteractiveTxConstructor` in the latter case, but not the former. Additionally, we may have splice-specific state that needs to be consumed as well to allow us to negotiate another splice later on. This commit ensures that we properly consume all splice and interactive funding state whenever possible upon a disconnect or `tx_abort`. The interactive funding state is safe to consume as long as we have either yet to reach `AwaitingSignatures`, or we have but `tx_signatures` has not been sent/received. In all of these cases, we also make sure to clear the quiescent state flag such that we're able to resume processing updates on the channel. The splice state is safe to consume as long as we don't have a pending `FundingNegotiation::AwaitingSignatures` with a `tx_signatures` sent/received and we don't have any negotiated candidates. Note that until splice RBF is supported, it is not currently possible to have any negotiated candidates with a pending interactive funding transaction. --- lightning/src/ln/channel.rs | 160 ++++++++++++++++------ lightning/src/ln/quiescence_tests.rs | 60 +++++++++ lightning/src/ln/splicing_tests.rs | 190 +++++++++++++++++++++++++++ 3 files changed, 372 insertions(+), 38 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 1ca067f43f4..2cd1b044e2a 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( &mut self, context: &ChannelContext, confirmed_funding_index: usize, height: u32, ) -> Option @@ -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 { + 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( channel_type: &ChannelTypeFeatures, fee_estimator: &LowerBoundedFeeEstimator, @@ -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 983fd8bd2d9..63c50094a67 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 1537a36cc8b..88271d71930 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);