Merge initial and retry stfu send paths

lightning/src/ln/channel.rs

@@ -80,7 +80,7 @@ use crate::util::config::{
 	MaxDustHTLCExposure, UserConfig,
 };
 use crate::util::errors::APIError;
-use crate::util::logger::{Logger, Record, WithContext};
+use crate::util::logger::{Level as LoggerLevel, Logger, Record, WithContext};
 use crate::util::scid_utils::{block_from_scid, scid_from_parts};
 use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, Writeable, Writer};
 use crate::util::wallet_utils::Input;
@@ -679,10 +679,9 @@ mod state_flags {
 	pub const LOCAL_SHUTDOWN_SENT: u32 = 1 << 11;
 	pub const SHUTDOWN_COMPLETE: u32 = 1 << 12;
 	pub const WAITING_FOR_BATCH: u32 = 1 << 13;
-	pub const AWAITING_QUIESCENCE: u32 = 1 << 14;
-	pub const LOCAL_STFU_SENT: u32 = 1 << 15;
-	pub const REMOTE_STFU_SENT: u32 = 1 << 16;
-	pub const QUIESCENT: u32 = 1 << 17;
+	pub const LOCAL_STFU_SENT: u32 = 1 << 14;
+	pub const REMOTE_STFU_SENT: u32 = 1 << 15;
+	pub const QUIESCENT: u32 = 1 << 16;
 }
 
 define_state_flags!(
@@ -749,13 +748,8 @@ define_state_flags!(
 			implicit ACK, so instead we have to hold them away temporarily to be sent later.",
 			AWAITING_REMOTE_REVOKE, state_flags::AWAITING_REMOTE_REVOKE,
 			is_awaiting_remote_revoke, set_awaiting_remote_revoke, clear_awaiting_remote_revoke),
-		("Indicates a local request has been made for the channel to become quiescent. Both nodes \
-			must send `stfu` for the channel to become quiescent. This flag will be cleared and we \
-			will no longer attempt quiescence if either node requests a shutdown.",
-			AWAITING_QUIESCENCE, state_flags::AWAITING_QUIESCENCE,
-			is_awaiting_quiescence, set_awaiting_quiescence, clear_awaiting_quiescence),
 		("Indicates we have sent a `stfu` message to the counterparty. This message can only be sent \
-			if either `AWAITING_QUIESCENCE` or `REMOTE_STFU_SENT` is set. Shutdown requests are \
+			if `REMOTE_STFU_SENT` is set, or a `QuiscentAction` is pending. Shutdown requests are \
 			rejected if this flag is set.",
 			LOCAL_STFU_SENT, state_flags::LOCAL_STFU_SENT,
 			is_local_stfu_sent, set_local_stfu_sent, clear_local_stfu_sent),
@@ -950,12 +944,6 @@ impl ChannelState {
 		clear_awaiting_remote_revoke,
 		ChannelReady
 	);
-	impl_state_flag!(
-		is_awaiting_quiescence,
-		set_awaiting_quiescence,
-		clear_awaiting_quiescence,
-		ChannelReady
-	);
 	impl_state_flag!(is_local_stfu_sent, set_local_stfu_sent, clear_local_stfu_sent, ChannelReady);
 	impl_state_flag!(
 		is_remote_stfu_sent,
@@ -1750,10 +1738,6 @@ where
 		let splice_funding_failed = if let ChannelPhase::Funded(chan) = &mut self.phase {
 			// Reset any quiescence-related state as it is implicitly terminated once disconnected.
 			if matches!(chan.context.channel_state, ChannelState::ChannelReady(_)) {
-				if chan.quiescent_action.is_some() {
-					// If we were trying to get quiescent, try again after reconnection.
-					chan.context.channel_state.set_awaiting_quiescence();
-				}
 				chan.context.channel_state.clear_local_stfu_sent();
 				chan.context.channel_state.clear_remote_stfu_sent();
 				if chan.should_reset_pending_splice_state(false) {
@@ -7088,7 +7072,6 @@ where
 			} else {
 				match self.quiescent_action.take() {
 					Some(QuiescentAction::LegacySplice(instructions)) => {
-						self.context.channel_state.clear_awaiting_quiescence();
 						let (inputs, outputs) = instructions.into_contributed_inputs_and_outputs();
 						Some(SpliceFundingFailed {
 							funding_txo: None,
@@ -7098,7 +7081,6 @@ where
 						})
 					},
 					Some(QuiescentAction::Splice { contribution, .. }) => {
-						self.context.channel_state.clear_awaiting_quiescence();
 						let (inputs, outputs) = contribution.into_contributed_inputs_and_outputs();
 						Some(SpliceFundingFailed {
 							funding_txo: None,
@@ -10747,11 +10729,6 @@ where
 		// From here on out, we may not fail!
 
 		self.context.channel_state.set_remote_shutdown_sent();
-		if self.context.channel_state.is_awaiting_quiescence() {
-			// We haven't been able to send `stfu` yet, and there's no point in attempting
-			// quiescence anymore since the counterparty wishes to close the channel.
-			self.context.channel_state.clear_awaiting_quiescence();
-		}
 		self.context.update_time_counter += 1;
 
 		let monitor_update = if update_shutdown_script {
@@ -11526,17 +11503,6 @@ where
 		let announcement_sigs =
 			self.get_announcement_sigs(node_signer, chain_hash, user_config, block_height, logger);
 
-		if let Some(quiescent_action) = self.quiescent_action.as_ref() {
-			// TODO(splicing): If we didn't win quiescence, then we can contribute as an acceptor
-			// instead of waiting for the splice to lock.
-			if matches!(
-				quiescent_action,
-				QuiescentAction::Splice { .. } | QuiescentAction::LegacySplice(_)
-			) {
-				self.context.channel_state.set_awaiting_quiescence();
-			}
-		}
-
 		Some(SpliceFundingPromotion {
 			funding_txo,
 			monitor_update,
@@ -13314,9 +13280,6 @@ where
 		// From here on out, we may not fail!
 		self.context.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw;
 		self.context.channel_state.set_local_shutdown_sent();
-		if self.context.channel_state.is_awaiting_quiescence() {
-			self.context.channel_state.clear_awaiting_quiescence();
-		}
 		self.context.local_initiated_shutdown = Some(());
 		self.context.update_time_counter += 1;
 
@@ -13410,65 +13373,17 @@ where
 			);
 			return Err(action);
 		}
+		// Since we don't have a pending quiescent action, we should never be in a state where we
+		// sent `stfu` without already having become quiescent.
+		debug_assert!(!self.context.channel_state.is_local_stfu_sent());
 
 		self.quiescent_action = Some(action);
-		if self.context.channel_state.is_quiescent()
-			|| self.context.channel_state.is_awaiting_quiescence()
-			|| self.context.channel_state.is_local_stfu_sent()
-		{
-			log_debug!(logger, "Channel is either pending quiescence or already quiescent");
+		if self.context.channel_state.is_quiescent() {
+			log_debug!(logger, "Channel is already quiescent");
 			return Ok(None);
 		}
 
-		self.context.channel_state.set_awaiting_quiescence();
-		if self.context.is_live() {
-			match self.send_stfu(logger) {
-				Ok(stfu) => Ok(Some(stfu)),
-				Err(e) => {
-					log_debug!(logger, "{e}");
-					Ok(None)
-				},
-			}
-		} else {
-			log_debug!(logger, "Waiting for peer reconnection to send stfu");
-			Ok(None)
-		}
-	}
-
-	// Assumes we are either awaiting quiescence or our counterparty has requested quiescence.
-	#[rustfmt::skip]
-	pub fn send_stfu<L: Logger>(&mut self, logger: &L) -> Result<msgs::Stfu, &'static str> {
-		debug_assert!(!self.context.channel_state.is_local_stfu_sent());
-		debug_assert!(
-			self.context.channel_state.is_awaiting_quiescence()
-				|| self.context.channel_state.is_remote_stfu_sent()
-		);
-		debug_assert!(self.context.is_live());
-
-		if self.context.is_waiting_on_peer_pending_channel_update()
-			|| self.context.is_monitor_or_signer_pending_channel_update()
-		{
-			return Err("We cannot send `stfu` while state machine is pending")
-		}
-
-		let initiator = if self.context.channel_state.is_remote_stfu_sent() {
-			// We may have also attempted to initiate quiescence.
-			self.context.channel_state.clear_awaiting_quiescence();
-			self.context.channel_state.clear_remote_stfu_sent();
-			self.context.channel_state.set_quiescent();
-			// We are sending an stfu in response to our couterparty's stfu, but had not yet sent
-			// our own stfu (even if `awaiting_quiescence` was set). Thus, the counterparty is the
-			// initiator and they can do "something fundamental".
-			false
-		} else {
-			log_debug!(logger, "Sending stfu as quiescence initiator");
-			debug_assert!(self.context.channel_state.is_awaiting_quiescence());
-			self.context.channel_state.clear_awaiting_quiescence();
-			self.context.channel_state.set_local_stfu_sent();
-			true
-		};
-
-		Ok(msgs::Stfu { channel_id: self.context.channel_id, initiator })
+		Ok(self.try_send_stfu(false, logger))
 	}
 
 	#[rustfmt::skip]
@@ -13505,10 +13420,7 @@ where
 			self.context.channel_state.set_remote_stfu_sent();
 
 			log_debug!(logger, "Received counterparty stfu proposing quiescence");
-			return self
-				.send_stfu(logger)
-				.map(|stfu| Some(StfuResponse::Stfu(stfu)))
-				.map_err(|e| ChannelError::Ignore(e.to_owned()));
+			return Ok(self.try_send_stfu(false, logger).map(|stfu| StfuResponse::Stfu(stfu)))
 		}
 
 		// We already sent `stfu` and are now processing theirs. It may be in response to ours, or
@@ -13610,17 +13522,28 @@ where
 		Ok(None)
 	}
 
-	pub fn try_send_stfu<L: Logger>(
-		&mut self, logger: &L,
-	) -> Result<Option<msgs::Stfu>, ChannelError> {
+	pub fn try_send_stfu<L: Logger>(&mut self, is_retry: bool, logger: &L) -> Option<msgs::Stfu> {
 		// We must never see both stfu flags set, we always set the quiescent flag instead.
 		debug_assert!(
 			!(self.context.channel_state.is_local_stfu_sent()
 				&& self.context.channel_state.is_remote_stfu_sent())
 		);
 
+		// We only need to send `stfu` when we're awaiting quiescence and haven't sent it yet, or
+		// in response to a counterparty one.
+		if self.quiescent_action.is_none() && !self.context.channel_state.is_remote_stfu_sent() {
+			return None;
+		}
+		if self.context.channel_state.is_local_stfu_sent()
+			|| self.context.channel_state.is_quiescent()
+		{
+			return None;
+		}
+
+		let logger_level = if is_retry { LoggerLevel::Trace } else { LoggerLevel::Debug };
 		if !self.context.is_live() {
-			return Ok(None);
+			log_given_level!(logger, logger_level, "Waiting for peer reconnection to send stfu");
+			return None;
 		}
 
 		if let Some(action) = self.quiescent_action.as_ref() {
@@ -13630,35 +13553,46 @@ where
 			let has_splice_action = matches!(action, QuiescentAction::Splice { .. })
 				|| matches!(action, QuiescentAction::LegacySplice(_));
 			if has_splice_action && self.pending_splice.is_some() {
-				return Ok(None);
+				log_given_level!(
+					logger,
+					logger_level,
+					"Waiting for pending splice to lock before sending stfu for new splice"
+				);
+				return None;
 			}
 		}
 
-		// We need to send our `stfu`, either because we're trying to initiate quiescence, or the
-		// counterparty is and we've yet to send ours.
-		if self.context.channel_state.is_awaiting_quiescence()
-			|| (self.context.channel_state.is_remote_stfu_sent()
-				&& !self.context.channel_state.is_local_stfu_sent())
+		if self.context.is_waiting_on_peer_pending_channel_update()
+			|| self.context.is_monitor_or_signer_pending_channel_update()
 		{
-			return self
-				.send_stfu(logger)
-				.map(|stfu| Some(stfu))
-				.map_err(|e| ChannelError::Ignore(e.to_owned()));
+			log_given_level!(
+				logger,
+				logger_level,
+				"Waiting for state machine pending changes to complete before sending stfu"
+			);
+			return None;
 		}
 
-		// We're either:
-		//  - already quiescent
-		//  - in a state where quiescence is not possible
-		//  - not currently trying to become quiescent
-		Ok(None)
+		let initiator = if self.context.channel_state.is_remote_stfu_sent() {
+			// Since we may have also attempted to initiate quiescence but the counterparty
+			// initiated first, we'll retry after we're no longer quiescent.
+			self.context.channel_state.clear_remote_stfu_sent();
+			self.context.channel_state.set_quiescent();
+			false
+		} else {
+			log_debug!(logger, "Sending stfu as quiescence initiator");
+			self.context.channel_state.set_local_stfu_sent();
+			true
+		};
+
+		Some(msgs::Stfu { channel_id: self.context.channel_id, initiator })
 	}
 
 	#[cfg(any(test, fuzzing, feature = "_test_utils"))]
 	#[rustfmt::skip]
 	pub fn exit_quiescence(&mut self) -> bool {
 		// Make sure we either finished the quiescence handshake and are quiescent, or we never
 		// attempted to initiate quiescence at all.
-		debug_assert!(!self.context.channel_state.is_awaiting_quiescence());
 		debug_assert!(!self.context.channel_state.is_local_stfu_sent());
 		debug_assert!(!self.context.channel_state.is_remote_stfu_sent());
 
@@ -14763,11 +14697,6 @@ impl<SP: SignerProvider> Writeable for FundedChannel<SP> {
 			match channel_state {
 				ChannelState::AwaitingChannelReady(_) => {},
 				ChannelState::ChannelReady(_) => {
-					if self.quiescent_action.is_some() {
-						// If we're trying to get quiescent to do something, try again when we
-						// reconnect to the peer.
-						channel_state.set_awaiting_quiescence();
-					}
 					channel_state.clear_local_stfu_sent();
 					channel_state.clear_remote_stfu_sent();
 					if self.should_reset_pending_splice_state(false)

lightning/src/ln/channelmanager.rs

@@ -13342,17 +13342,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 					let logger = WithContext::from(
 						&self.logger, Some(*counterparty_node_id), Some(*channel_id), None
 					);
-					match funded_chan.try_send_stfu(&&logger) {
-						Ok(None) => {},
-						Ok(Some(stfu)) => {
-							pending_msg_events.push(MessageSendEvent::SendStfu {
-								node_id: chan.context().get_counterparty_node_id(),
-								msg: stfu,
-							});
-						},
-						Err(e) => {
-							log_debug!(logger, "Could not advance quiescence handshake: {}", e);
-						}
+					if let Some(stfu) = funded_chan.try_send_stfu(true, &&logger) {
+						pending_msg_events.push(MessageSendEvent::SendStfu {
+							node_id: chan.context().get_counterparty_node_id(),
+							msg: stfu,
+						});
 					}
 				}
 			}

lightning/src/ln/quiescence_tests.rs

@@ -35,6 +35,11 @@ fn test_quiescence_tie() {
 
 	assert!(nodes[0].node.exit_quiescence(&nodes[1].node.get_our_node_id(), &chan_id).unwrap());
 	assert!(nodes[1].node.exit_quiescence(&nodes[0].node.get_our_node_id(), &chan_id).unwrap());
+
+	// Since node 1 lost the tie, they'll attempt quiescence again.
+	let stfu =
+		get_event_msg!(nodes[1], MessageSendEvent::SendStfu, nodes[0].node.get_our_node_id());
+	assert!(stfu.initiator);
 }
 
 #[test]

lightning/src/ln/splicing_tests.rs

@@ -1145,6 +1145,49 @@ fn fails_initiating_concurrent_splices(reconnect: bool) {
 	);
 }
 
+#[test]
+fn test_initiating_splice_holds_stfu_with_pending_splice() {
+	// Test that we don't send stfu too early for a new splice while we're already pending one.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let config = test_default_channel_config();
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let node_0_id = nodes[0].node.get_our_node_id();
+	provide_utxo_reserves(&nodes, 2, Amount::ONE_BTC);
+
+	let initial_channel_value_sat = 100_000;
+	let (_, _, channel_id, _) =
+		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_value_sat, 0);
+
+	// Have both nodes attempt a splice, but only node 0 will call back and negotiate the splice.
+	let value_added = Amount::from_sat(10_000);
+	let funding_contribution_0 = initiate_splice_in(&nodes[0], &nodes[1], channel_id, value_added);
+
+	let feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64);
+	let funding_template = nodes[1].node.splice_channel(&channel_id, &node_0_id, feerate).unwrap();
+
+	let (splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution_0);
+
+	// With the splice negotiated, have node 1 call back. This will queue the quiescent action, but
+	// it shouldn't send stfu yet as there's a pending splice.
+	let wallet = WalletSync::new(Arc::clone(&nodes[1].wallet_source), &nodes[1].logger);
+	let funding_contribution = funding_template.splice_in_sync(value_added, &wallet).unwrap();
+	nodes[1]
+		.node
+		.funding_contributed(&channel_id, &node_0_id, funding_contribution.clone(), None)
+		.unwrap();
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+	mine_transaction(&nodes[0], &splice_tx);
+	mine_transaction(&nodes[1], &splice_tx);
+	let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], 5);
+	assert!(
+		matches!(stfu, Some(MessageSendEvent::SendStfu { node_id, .. }) if node_id == node_0_id)
+	);
+}
+
 #[cfg(test)]
 #[derive(PartialEq)]
 enum SpliceStatus {