Stop skipping the line in quiescence if our peer speaks first

In the case where we prepare to initiate quiescence, but cannot yet
send our `stfu` because we're waiting on some channel operations to
settle, and our peer ultimately sends their `stfu` before we can,
we would detect this case and, if we were able, send an `stfu`
which would allow us to send "something fundamental" first.

While this is a nifty optimization, its a bit overkill - the chance
that both us and our peer decide to attempt something fundamental
at the same time is pretty low, and worse this required additional
state tracking.

We simply remove this optimization here, simplifying the quiescence
state machine a good bit.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -2728,10 +2728,6 @@ where
 	/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
 	/// store it here and only release it to the `ChannelManager` once it asks for it.
 	blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
-
-	/// Only set when a counterparty `stfu` has been processed to track which node is allowed to
-	/// propose "something fundamental" upon becoming quiescent.
-	is_holder_quiescence_initiator: Option<bool>,
 }
 
 /// A channel struct implementing this trait can receive an initial counterparty commitment
@@ -3306,8 +3302,6 @@ where
 			blocked_monitor_updates: Vec::new(),
 
 			is_manual_broadcast: false,
-
-			is_holder_quiescence_initiator: None,
 		};
 
 		Ok((funding, channel_context))
@@ -3544,8 +3538,6 @@ where
 			blocked_monitor_updates: Vec::new(),
 			local_initiated_shutdown: None,
 			is_manual_broadcast: false,
-
-			is_holder_quiescence_initiator: None,
 		};
 
 		Ok((funding, channel_context))
@@ -8202,7 +8194,6 @@ where
 			self.context.channel_state.clear_local_stfu_sent();
 			self.context.channel_state.clear_remote_stfu_sent();
 			self.context.channel_state.clear_quiescent();
-			self.context.is_holder_quiescence_initiator.take();
 		}
 
 		self.context.channel_state.set_peer_disconnected();
@@ -11591,18 +11582,10 @@ where
 			self.context.channel_state.clear_awaiting_quiescence();
 			self.context.channel_state.clear_remote_stfu_sent();
 			self.context.channel_state.set_quiescent();
-			if let Some(initiator) = self.context.is_holder_quiescence_initiator.as_ref() {
-				log_debug!(
-					logger,
-					"Responding to counterparty stfu with our own, channel is now quiescent and we are{} the initiator",
-					if !initiator { " not" } else { "" }
-				);
-
-				*initiator
-			} else {
-				debug_assert!(false, "Quiescence initiator must have been set when we received stfu");
-				false
-			}
+			// 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());
@@ -11633,9 +11616,7 @@ where
 			));
 		}
 
-		if self.context.channel_state.is_awaiting_quiescence()
-			|| !self.context.channel_state.is_local_stfu_sent()
-		{
+		if !self.context.channel_state.is_local_stfu_sent() {
 			if !msg.initiator {
 				return Err(ChannelError::WarnAndDisconnect(
 					"Peer sent unexpected `stfu` without signaling as initiator".to_owned()
@@ -11649,23 +11630,13 @@ where
 			// then.
 			self.context.channel_state.set_remote_stfu_sent();
 
-			let is_holder_initiator = if self.context.channel_state.is_awaiting_quiescence() {
-				// We were also planning to propose quiescence, let the tie-breaker decide the
-				// initiator.
-				self.funding.is_outbound()
-			} else {
-				false
-			};
-			self.context.is_holder_quiescence_initiator = Some(is_holder_initiator);
-
 			log_debug!(logger, "Received counterparty stfu proposing quiescence");
 			return self.send_stfu(logger).map(|stfu| Some(stfu));
 		}
 
 		// We already sent `stfu` and are now processing theirs. It may be in response to ours, or
 		// we happened to both send `stfu` at the same time and a tie-break is needed.
 		let is_holder_quiescence_initiator = !msg.initiator || self.funding.is_outbound();
-		self.context.is_holder_quiescence_initiator = Some(is_holder_quiescence_initiator);
 
 		// We were expecting to receive `stfu` because we already sent ours.
 		self.mark_response_received();
@@ -11733,13 +11704,10 @@ where
 		debug_assert!(!self.context.channel_state.is_local_stfu_sent());
 		debug_assert!(!self.context.channel_state.is_remote_stfu_sent());
 
-		if self.context.channel_state.is_quiescent() {
-			self.mark_response_received();
-			self.context.channel_state.clear_quiescent();
-			self.context.is_holder_quiescence_initiator.take().expect("Must always be set while quiescent")
-		} else {
-			false
-		}
+		self.mark_response_received();
+		let was_quiescent = self.context.channel_state.is_quiescent();
+		self.context.channel_state.clear_quiescent();
+		was_quiescent
 	}
 
 	pub fn remove_legacy_scids_before_block(&mut self, height: u32) -> alloc::vec::Drain<'_, u64> {
@@ -14001,8 +13969,6 @@ where
 
 				blocked_monitor_updates: blocked_monitor_updates.unwrap(),
 				is_manual_broadcast: is_manual_broadcast.unwrap_or(false),
-
-				is_holder_quiescence_initiator: None,
 			},
 			interactive_tx_signing_session,
 			holder_commitment_point,

lightning/src/ln/quiescence_tests.rs

@@ -33,7 +33,7 @@ fn test_quiescence_tie() {
 	assert!(stfu_node_0.initiator && stfu_node_1.initiator);
 
 	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());
+	assert!(nodes[1].node.exit_quiescence(&nodes[0].node.get_our_node_id(), &chan_id).unwrap());
 }
 
 #[test]
@@ -173,7 +173,8 @@ fn allow_shutdown_while_awaiting_quiescence(local_shutdown: bool) {
 
 	// Now that the state machine is no longer pending, and `closing_signed` is ready to be sent,
 	// make sure we're still not waiting for the quiescence handshake to complete.
-	local_node.node.exit_quiescence(&remote_node_id, &chan_id).unwrap();
+	// Note that we never actually reached full quiescence here.
+	assert!(!local_node.node.exit_quiescence(&remote_node_id, &chan_id).unwrap());
 
 	let _ = get_event_msg!(local_node, MessageSendEvent::SendClosingSigned, remote_node_id);
 	check_added_monitors(local_node, 2); // One for the last revoke_and_ack, another for closing_signed
@@ -279,8 +280,8 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() {
 	let stfu = get_event_msg!(&nodes[0], MessageSendEvent::SendStfu, node_id_1);
 	nodes[1].node.handle_stfu(node_id_0, &stfu);
 
-	nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap();
-	nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap();
+	assert!(nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap());
+	assert!(nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap());
 
 	// After exiting quiescence, we should be able to resume payments from nodes[0].
 	send_payment(&nodes[0], &[&nodes[1]], payment_amount);
@@ -336,8 +337,8 @@ fn test_quiescence_on_final_revoke_and_ack_pending_monitor_update() {
 		panic!();
 	}
 
-	nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap();
-	nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap();
+	assert!(nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap());
+	assert!(nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap());
 }
 
 #[test]
@@ -406,8 +407,8 @@ fn quiescence_updates_go_to_holding_cell(fail_htlc: bool) {
 	let stfu = get_event_msg!(&nodes[0], MessageSendEvent::SendStfu, node_id_1);
 	nodes[1].node.handle_stfu(node_id_0, &stfu);
 
-	nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap();
-	nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap();
+	assert!(nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap());
+	assert!(nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap());
 
 	// Now that quiescence is over, nodes are allowed to make updates again. nodes[1] will have its
 	// outbound HTLC finally go out, along with the fail/claim of nodes[0]'s payment.