Merge pull request #3721 from wpaulino/no-disconnect-async-monitor-signer

Avoid disconnect on message timeout while waiting on monitor/signer

Author: TheBlueMatt

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

@@ -3302,23 +3302,33 @@ 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.channel_state.is_monitor_update_in_progress()
+			|| self.signer_pending_revoke_and_ack
+			|| 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.
-	fn has_pending_channel_update(&self) -> bool {
+	///
+	/// 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 is_waiting_on_peer_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;
 		}
 
@@ -7513,8 +7523,11 @@ impl<SP: Deref> FundedChannel<SP> where
 			|| self.context.channel_state.is_local_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.is_waiting_on_peer_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);
@@ -9397,7 +9410,9 @@ impl<SP: Deref> FundedChannel<SP> where
 		);
 		debug_assert!(self.context.is_live());
 
-		if self.context.has_pending_channel_update() {
+		if self.context.is_waiting_on_peer_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()
 			));
@@ -9458,10 +9473,11 @@ impl<SP: Deref> FundedChannel<SP> where
 				));
 			}
 
-			// We don't check `has_pending_channel_update` prior to setting the flag because it
-			// considers pending updates from either node. This means we may accept a counterparty
-			// `stfu` while they had pending updates, but that's fine as we won't send ours until
-			// _all_ pending updates complete, allowing the channel to become quiescent then.
+			// We don't check `is_waiting_on_peer_pending_channel_update` prior to setting the flag
+			// because it considers pending updates from either node. This means we may accept a
+			// counterparty `stfu` while they had pending updates, but that's fine as we won't send
+			// ours until _all_ pending updates complete, allowing the channel to become quiescent
+			// then.
 			self.context.channel_state.set_remote_stfu_sent();
 
 			let is_holder_initiator = if self.context.channel_state.is_awaiting_quiescence() {
@@ -9485,7 +9501,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.is_waiting_on_peer_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

@@ -180,10 +180,10 @@ fn allow_shutdown_while_awaiting_quiescence(local_shutdown: bool) {
 }
 
 #[test]
-fn test_quiescence_tracks_monitor_update_in_progress_and_waits_for_async_signer() {
+fn test_quiescence_waits_for_async_signer_and_monitor_update() {
 	// Test that quiescence:
 	//   a) considers an async signer when determining whether a pending channel update exists
-	//   b) tracks in-progress monitor updates until no longer quiescent
+	//   b) waits until pending monitor updates complete to send `stfu`/become quiescent
 	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]);
@@ -244,29 +244,12 @@ fn test_quiescence_tracks_monitor_update_in_progress_and_waits_for_async_signer(
 	let revoke_and_ack = find_msg!(msg_events, SendRevokeAndACK);
 	let stfu = find_msg!(msg_events, SendStfu);
 
-	// While handling the last `revoke_and_ack` on nodes[0], we'll hold the monitor update and
-	// become quiescent.
+	// While handling the last `revoke_and_ack` on nodes[0], we'll hold the monitor update. We
+	// cannot become quiescent until it completes.
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
 	nodes[0].node.handle_revoke_and_ack(node_id_1, &revoke_and_ack);
 
 	nodes[0].node.handle_stfu(node_id_1, &stfu);
-	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();
-
-	// After exiting quiescence, we should be able to resume payments from nodes[0], but the monitor
-	// update has yet to complete. Attempting to send a payment now will be delayed until the
-	// monitor update completes.
-	{
-		let (route, payment_hash, _, payment_secret) =
-			get_route_and_payment_hash!(&nodes[0], &nodes[1], payment_amount);
-		let onion = RecipientOnionFields::secret_only(payment_secret);
-		let payment_id = PaymentId(payment_hash.0);
-		nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap();
-	}
-	check_added_monitors(&nodes[0], 0);
 	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
 
 	// We have two updates pending:
@@ -276,17 +259,21 @@ fn test_quiescence_tracks_monitor_update_in_progress_and_waits_for_async_signer(
 			chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
 		let chain_monitor = &nodes[0].chain_monitor.chain_monitor;
 		// One for the latest commitment transaction update from the last `revoke_and_ack`
-		chain_monitor.channel_monitor_updated(chan_id, latest_update - 1).unwrap();
+		chain_monitor.channel_monitor_updated(chan_id, latest_update).unwrap();
 		expect_payment_sent(&nodes[0], preimage, None, true, true);
 		// One for the commitment secret update from the last `revoke_and_ack`
-		chain_monitor.channel_monitor_updated(chan_id, latest_update).unwrap();
+		chain_monitor.channel_monitor_updated(chan_id, latest_update + 1).unwrap();
 	}
 
-	// With the pending monitor updates complete, we'll see a new monitor update go out when freeing
-	// the holding cells to send out the new HTLC.
-	nodes[0].chain_monitor.complete_sole_pending_chan_update(&chan_id);
-	let _ = get_htlc_update_msgs!(&nodes[0], node_id_1);
-	check_added_monitors(&nodes[0], 1);
+	// With the updates completed, we can now become quiescent.
+	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();
+
+	// After exiting quiescence, we should be able to resume payments from nodes[0].
+	send_payment(&nodes[0], &[&nodes[1]], payment_amount);
 }
 
 #[test]