From a5b3bd295e49205310b0b188c3a271eb120b7259 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 28 May 2025 10:35:59 -0700 Subject: [PATCH] Allow counterparty pending monitor update within quiescence handshake Previously, if we were negotiating quiescence, and we had already sent our `stfu`, we'd disconnect upon receiving the counterparty's `stfu` if we had a pending monitor update. This could result from processing a counterparty's final `revoke_and_ack` to an update, and immediately processing their `stfu` (which is valid from their point of view) without complete the monitor update. This was unintended, as we are able to track the quiescent and pending monitor update flags at the same time. Note that this commit still considers whether our signer owes any messages, as these are indicative of a channel update still pending. --- lightning/src/ln/channel.rs | 8 +++-- lightning/src/ln/quiescence_tests.rs | 54 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c5eb9de777a..6dfe6b1608c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -9675,10 +9675,14 @@ impl FundedChannel where self.mark_response_received(); if self.context.is_waiting_on_peer_pending_channel_update() - || self.context.is_monitor_or_signer_pending_channel_update() + || self.context.signer_pending_revoke_and_ack + || self.context.signer_pending_commitment_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. + // be pending, so anything pending currently must be from a counterparty update. We may + // have a monitor update pending if we've processed a message from the counterparty, but + // we don't consider this when becoming quiescent since the states are not mutually + // exclusive. return Err(ChannelError::WarnAndDisconnect( "Received counterparty stfu while having pending counterparty updates".to_owned() )); diff --git a/lightning/src/ln/quiescence_tests.rs b/lightning/src/ln/quiescence_tests.rs index 9bbb94777b7..82aa0208c67 100644 --- a/lightning/src/ln/quiescence_tests.rs +++ b/lightning/src/ln/quiescence_tests.rs @@ -276,6 +276,60 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() { send_payment(&nodes[0], &[&nodes[1]], payment_amount); } +#[test] +fn test_quiescence_on_final_revoke_and_ack_pending_monitor_update() { + // Test that we do not let a pending monitor update for a final `revoke_and_ack` prevent us from + // entering quiescence. This was caught by the fuzzer, reported as #3805. + 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; + 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], 1); + + nodes[1].node.maybe_propose_quiescence(&node_id_0, &chan_id).unwrap(); + let stfu = get_event_msg!(&nodes[1], MessageSendEvent::SendStfu, node_id_0); + nodes[0].node.handle_stfu(node_id_1, &stfu); + + let update_add = get_htlc_update_msgs!(&nodes[0], node_id_1); + nodes[1].node.handle_update_add_htlc(node_id_0, &update_add.update_add_htlcs[0]); + nodes[1].node.handle_commitment_signed_batch_test(node_id_0, &update_add.commitment_signed); + check_added_monitors(&nodes[1], 1); + + let (revoke_and_ack, commit_sig) = get_revoke_commit_msgs!(&nodes[1], node_id_0); + nodes[0].node.handle_revoke_and_ack(node_id_1, &revoke_and_ack); + check_added_monitors(&nodes[0], 1); + nodes[0].node.handle_commitment_signed_batch_test(node_id_1, &commit_sig); + check_added_monitors(&nodes[0], 1); + + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + let msgs = nodes[0].node.get_and_clear_pending_msg_events(); + if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &msgs[0] { + nodes[1].node.handle_revoke_and_ack(node_id_0, &msg); + check_added_monitors(&nodes[1], 1); + } else { + panic!(); + } + if let MessageSendEvent::SendStfu { msg, .. } = &msgs[1] { + nodes[1].node.handle_stfu(node_id_0, &msg); + } else { + panic!(); + } + + nodes[0].node.exit_quiescence(&node_id_1, &chan_id).unwrap(); + nodes[1].node.exit_quiescence(&node_id_0, &chan_id).unwrap(); +} + #[test] fn test_quiescence_updates_go_to_holding_cell() { quiescence_updates_go_to_holding_cell(false);