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.

Author: wpaulino

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,

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);
+}