Avoid disconnect on message timeout while waiting on monitor/signer

When sending/receiving `commitment_signed`/`revoke_and_ack`, we may
expect the counterparty to follow up with one of their own in response.
In some cases, they are not allowed to send it because they are actually
waiting for one from us. Async monitor updates and signing requests may
result in the message we need to send to the counterparty being delayed,
and our disconnect logic previously did not consider that. It doesn't
make sense to disconnect our counterparty when we're the ones seemingly
blocking progress.

This commit ensures we no longer disconnect when we're waiting on an
async monitor update or signing request, unless we're negotiating
quiescence. Note that while our counterparty is still able to enforce a
similar disconnect logic on their side, as they have no insight into why
we're not able to make progress, this commit at least helps prevent
reconnection cycles with those that don't enforce one.

Author: wpaulino

lightning/src/ln/async_signer_tests.rs

@@ -21,6 +21,7 @@ use crate::chain::ChannelMonitorUpdateStatus;
 use crate::events::bump_transaction::WalletSource;
 use crate::events::{ClosureReason, Event};
 use crate::ln::chan_utils::ClosingTransaction;
+use crate::ln::channel::DISCONNECT_PEER_AWAITING_RESPONSE_TICKS;
 use crate::ln::channel_state::{ChannelDetails, ChannelShutdownState};
 use crate::ln::channelmanager::{PaymentId, RAACommitmentOrder, RecipientOnionFields};
 use crate::ln::msgs::{BaseMessageHandler, ChannelMessageHandler, MessageSendEvent};
@@ -1091,3 +1092,130 @@ fn do_test_closing_signed(extra_closing_signed: bool, reconnect: bool) {
 	check_closed_event!(nodes[0], 1, ClosureReason::LocallyInitiatedCooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
 	check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyInitiatedCooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
 }
+
+#[test]
+fn test_no_disconnect_while_async_revoke_and_ack_expecting_remote_commitment_signed() {
+	// Nodes with async signers may be expecting to receive a `commitment_signed` from the
+	// counterparty even if a `revoke_and_ack` has yet to be sent due to an async signer. Test that
+	// we don't disconnect the async signer node due to not receiving the `commitment_signed` within
+	// the timeout while the `revoke_and_ack` is not ready.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+
+	let node_id_0 = nodes[0].node.get_our_node_id();
+	let node_id_1 = nodes[1].node.get_our_node_id();
+
+	let payment_amount = 1_000_000;
+	send_payment(&nodes[0], &[&nodes[1]], payment_amount * 4);
+
+	nodes[1].disable_channel_signer_op(&node_id_0, &chan_id, SignerOp::ReleaseCommitmentSecret);
+
+	// We'll send a payment from both nodes to each other.
+	let (route1, payment_hash1, _, payment_secret1) =
+		get_route_and_payment_hash!(&nodes[0], &nodes[1], payment_amount);
+	let onion1 = RecipientOnionFields::secret_only(payment_secret1);
+	let payment_id1 = PaymentId(payment_hash1.0);
+	nodes[0].node.send_payment_with_route(route1, payment_hash1, onion1, payment_id1).unwrap();
+	check_added_monitors(&nodes[0], 1);
+
+	let (route2, payment_hash2, _, payment_secret2) =
+		get_route_and_payment_hash!(&nodes[1], &nodes[0], payment_amount);
+	let onion2 = RecipientOnionFields::secret_only(payment_secret2);
+	let payment_id2 = PaymentId(payment_hash2.0);
+	nodes[1].node.send_payment_with_route(route2, payment_hash2, onion2, payment_id2).unwrap();
+	check_added_monitors(&nodes[1], 1);
+
+	let update = get_htlc_update_msgs!(&nodes[0], node_id_1);
+	nodes[1].node.handle_update_add_htlc(node_id_0, &update.update_add_htlcs[0]);
+	nodes[1].node.handle_commitment_signed_batch_test(node_id_0, &update.commitment_signed);
+	check_added_monitors(&nodes[1], 1);
+
+	let update = get_htlc_update_msgs!(&nodes[1], node_id_0);
+	nodes[0].node.handle_update_add_htlc(node_id_1, &update.update_add_htlcs[0]);
+	nodes[0].node.handle_commitment_signed_batch_test(node_id_1, &update.commitment_signed);
+	check_added_monitors(&nodes[0], 1);
+
+	// nodes[0] can only respond with a `revoke_and_ack`. The `commitment_signed` that would follow
+	// is blocked on receiving a counterparty `revoke_and_ack`, which nodes[1] is still pending on.
+	let revoke_and_ack = get_event_msg!(&nodes[0], MessageSendEvent::SendRevokeAndACK, node_id_1);
+	nodes[1].node.handle_revoke_and_ack(node_id_0, &revoke_and_ack);
+	check_added_monitors(&nodes[1], 1);
+
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+	// nodes[0] will disconnect the counterparty as it's waiting on a `revoke_and_ack`.
+	// nodes[1] is waiting on a `commitment_signed`, but since it hasn't yet sent its own
+	// `revoke_and_ack`, it shouldn't disconnect yet.
+	for _ in 0..DISCONNECT_PEER_AWAITING_RESPONSE_TICKS {
+		nodes[0].node.timer_tick_occurred();
+		nodes[1].node.timer_tick_occurred();
+	}
+	let has_disconnect_event = |event| {
+		matches!(
+			event, MessageSendEvent::HandleError { action , .. }
+			if matches!(action, msgs::ErrorAction::DisconnectPeerWithWarning { .. })
+		)
+	};
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().into_iter().any(has_disconnect_event));
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+}
+
+#[test]
+fn test_no_disconnect_while_async_commitment_signed_expecting_remote_revoke_and_ack() {
+	// Nodes with async signers may be expecting to receive a `revoke_and_ack` from the
+	// counterparty even if a `commitment_signed` has yet to be sent due to an async signer. Test
+	// that we don't disconnect the async signer node due to not receiving the `revoke_and_ack`
+	// within the timeout while the `commitment_signed` is not ready.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+
+	let node_id_0 = nodes[0].node.get_our_node_id();
+	let node_id_1 = nodes[1].node.get_our_node_id();
+
+	// Route a payment and attempt to claim it.
+	let payment_amount = 1_000_000;
+	let (preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], payment_amount);
+	nodes[1].node.claim_funds(preimage);
+	check_added_monitors(&nodes[1], 1);
+
+	// We'll disable signing counterparty commitments on the payment sender.
+	nodes[0].disable_channel_signer_op(&node_id_1, &chan_id, SignerOp::SignCounterpartyCommitment);
+
+	// After processing the `update_fulfill`, they'll only be able to send `revoke_and_ack` until
+	// the `commitment_signed` is no longer pending.
+	let update = get_htlc_update_msgs!(&nodes[1], node_id_0);
+	nodes[0].node.handle_update_fulfill_htlc(node_id_1, &update.update_fulfill_htlcs[0]);
+	nodes[0].node.handle_commitment_signed_batch_test(node_id_1, &update.commitment_signed);
+	check_added_monitors(&nodes[0], 1);
+
+	let revoke_and_ack = get_event_msg!(&nodes[0], MessageSendEvent::SendRevokeAndACK, node_id_1);
+	nodes[1].node.handle_revoke_and_ack(node_id_0, &revoke_and_ack);
+	check_added_monitors(&nodes[1], 1);
+
+	// The payment sender shouldn't disconnect the counterparty due to a missing `revoke_and_ack`
+	// because the `commitment_signed` isn't ready yet. The payment recipient may disconnect the
+	// sender because it doesn't have an async signer and it's expecting a timely
+	// `commitment_signed` response.
+	for _ in 0..DISCONNECT_PEER_AWAITING_RESPONSE_TICKS {
+		nodes[0].node.timer_tick_occurred();
+		nodes[1].node.timer_tick_occurred();
+	}
+	let has_disconnect_event = |event| {
+		matches!(
+			event, MessageSendEvent::HandleError { action , .. }
+			if matches!(action, msgs::ErrorAction::DisconnectPeerWithWarning { .. })
+		)
+	};
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().into_iter().any(has_disconnect_event));
+
+	expect_payment_sent(&nodes[0], preimage, None, false, false);
+	expect_payment_claimed!(nodes[1], payment_hash, payment_amount);
+}

lightning/src/ln/channel.rs

@@ -3205,23 +3205,32 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		}
 	}
 
+	/// Checks whether the channel has any HTLC additions, HTLC removals, or fee updates that have
+	/// been sent by either side but not yet irrevocably committed on both commitments because we're
+	/// waiting on a pending monitor update or signer request.
+	pub fn is_monitor_or_signer_pending_channel_update(&self) -> bool {
+		self.monitor_pending_revoke_and_ack || self.signer_pending_revoke_and_ack
+			|| self.monitor_pending_commitment_signed || self.signer_pending_commitment_update
+	}
+
 	/// Checks whether the channel has any HTLC additions, HTLC removals, or fee updates that have
 	/// been sent by either side but not yet irrevocably committed on both commitments. Holding cell
 	/// updates are not considered because they haven't been sent to the peer yet.
 	///
 	/// This can be used to satisfy quiescence's requirement when sending `stfu`:
 	///  - MUST NOT send `stfu` if any of the sender's htlc additions, htlc removals
 	///    or fee updates are pending for either peer.
+	///
+	/// Note that it is still possible for an update to be pending that's not captured here due to a
+	/// pending monitor update or signer request. `is_monitor_or_signer_pending_channel_update`
+	/// should also be checked in such cases.
 	fn has_pending_channel_update(&self) -> bool {
 		// An update from the local/remote node may be pending on the remote/local commitment since
 		// they are not tracked within our state, so we rely on whether any `commitment_signed` or
 		// `revoke_and_ack` messages are owed.
 		//
 		// We check these flags first as they are more likely to be set.
-		if self.channel_state.is_awaiting_remote_revoke() || self.expecting_peer_commitment_signed
-			|| self.monitor_pending_revoke_and_ack || self.signer_pending_revoke_and_ack
-			|| self.monitor_pending_commitment_signed || self.signer_pending_commitment_update
-		{
+		if self.channel_state.is_awaiting_remote_revoke() || self.expecting_peer_commitment_signed {
 			return true;
 		}
 
@@ -7419,8 +7428,11 @@ impl<SP: Deref> FundedChannel<SP> where
 			|| self.context.channel_state.is_remote_stfu_sent()
 			// Cleared upon receiving a message that triggers the end of quiescence.
 			|| self.context.channel_state.is_quiescent()
-			// Cleared upon receiving `revoke_and_ack`.
-			|| self.context.has_pending_channel_update()
+			// Cleared upon receiving `revoke_and_ack`. Since we're not queiscent, as we just
+			// checked above, we intentionally don't disconnect our counterparty if we're waiting on
+			// a monitor update or signer request.
+			|| (self.context.has_pending_channel_update()
+				&& !self.context.is_monitor_or_signer_pending_channel_update())
 		{
 			// This is the first tick we've seen after expecting to make forward progress.
 			self.context.sent_message_awaiting_response = Some(1);
@@ -9308,7 +9320,9 @@ impl<SP: Deref> FundedChannel<SP> where
 		);
 		debug_assert!(self.context.is_live());
 
-		if self.context.has_pending_channel_update() {
+		if self.context.has_pending_channel_update()
+			|| self.context.is_monitor_or_signer_pending_channel_update()
+		{
 			return Err(ChannelError::Ignore(
 				"We cannot send `stfu` while state machine is pending".to_owned()
 			));
@@ -9396,7 +9410,9 @@ impl<SP: Deref> FundedChannel<SP> where
 		// We were expecting to receive `stfu` because we already sent ours.
 		self.mark_response_received();
 
-		if self.context.has_pending_channel_update() {
+		if self.context.has_pending_channel_update()
+			|| self.context.is_monitor_or_signer_pending_channel_update()
+		{
 			// Since we've already sent `stfu`, it should not be possible for one of our updates to
 			// be pending, so anything pending currently must be from a counterparty update.
 			return Err(ChannelError::WarnAndDisconnect(

lightning/src/ln/quiescence_tests.rs

@@ -482,6 +482,13 @@ fn test_quiescence_timeout_while_waiting_for_holder_stfu() {
 	check_added_monitors(&nodes[0], 1);
 	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
 
+	// Since nodes[0] has no context of the quiescence attempt yet, it should not disconnect the
+	// counterparty just because it hasn't completed it's monitor update in a timely manner.
+	for _ in 0..DISCONNECT_PEER_AWAITING_RESPONSE_TICKS {
+		nodes[0].node.timer_tick_occurred();
+	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+
 	nodes[0].node.handle_stfu(node_id_1, &stfu);
 	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());