Clear duplicate monitor-pending RAA on signer resend

The `chanmon_consistency` fuzz target found a reconnect ordering where
`signer_pending_revoke_and_ack` and `monitor_pending_revoke_and_ack`
could both describe the same owed `revoke_and_ack`.

The channel first received a `commitment_signed` whose monitor update
completed, but the signer could not provide the next point or secret,
leaving `signer_pending_revoke_and_ack` set. Later, receiving the peer
`revoke_and_ack` freed holding-cell HTLCs and produced a held monitor
update. While that monitor update was still blocked,
`channel_reestablish` saw the peer one state behind and recorded
`monitor_pending_revoke_and_ack`, plus the corresponding monitor-pending
`commitment_signed`, so the messages could be replayed once monitor
updating was restored.

If the signer unblocked before the held monitor update was released,
`signer_maybe_unblocked` generated and sent the already monitor-safe RAA
using `signer_pending_revoke_and_ack`. The monitor-pending flag was not
cleared at that point, so `monitor_updating_restored` later generated
the same RAA again when the held update completed. The peer had already
advanced after accepting the signer-unblocked RAA, so it rejected the
duplicate secret as not corresponding to its current pubkey and
force-closed.

Fix this by clearing `monitor_pending_revoke_and_ack` in the
signer-resume path only once a signer-pending RAA is actually being
returned.

Author: wpaulino

lightning/src/ln/async_signer_tests.rs

@@ -18,7 +18,7 @@ use bitcoin::{Amount, TxOut};
 
 use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
 use crate::chain::ChannelMonitorUpdateStatus;
-use crate::events::{ClosureReason, Event};
+use crate::events::{ClosureReason, Event, HTLCHandlingFailureType};
 use crate::ln::chan_utils::ClosingTransaction;
 use crate::ln::channel::DISCONNECT_PEER_AWAITING_RESPONSE_TICKS;
 use crate::ln::channel_state::{ChannelDetails, ChannelShutdownState};
@@ -498,6 +498,211 @@ fn test_async_raa_peer_disconnect() {
 	do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeReestablish, false);
 }
 
+#[test]
+fn test_signer_unblocked_clears_monitor_pending_raa_after_reestablish() {
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	let node_a_id = nodes[0].node.get_our_node_id();
+	let node_b_id = nodes[1].node.get_our_node_id();
+	let node_c_id = nodes[2].node.get_our_node_id();
+
+	create_announced_chan_between_nodes(&nodes, 0, 1);
+	let chan_bc = create_announced_chan_between_nodes(&nodes, 1, 2);
+
+	// Rebalance so that node C can send a payment back through node B later in the test.
+	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5_000_000);
+
+	// Put the B-C channel into AwaitingRAA by having C fail a payment backwards and retaining C's
+	// final RAA instead of delivering it to B immediately.
+	let (_, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000);
+	nodes[2].node.fail_htlc_backwards(&payment_hash_1);
+	expect_and_process_pending_htlcs_and_htlc_handling_failed(
+		&nodes[2],
+		&[HTLCHandlingFailureType::Receive { payment_hash: payment_hash_1 }],
+	);
+	check_added_monitors(&nodes[2], 1);
+
+	let updates = get_htlc_update_msgs(&nodes[2], &node_b_id);
+	assert!(updates.update_add_htlcs.is_empty());
+	assert_eq!(updates.update_fail_htlcs.len(), 1);
+	assert!(updates.update_fail_malformed_htlcs.is_empty());
+	assert!(updates.update_fee.is_none());
+	nodes[1].node.handle_update_fail_htlc(node_c_id, &updates.update_fail_htlcs[0]);
+
+	let pending_c_raa =
+		commitment_signed_dance_return_raa(&nodes[1], &nodes[2], &updates.commitment_signed, false);
+	check_added_monitors(&nodes[0], 0);
+
+	// While B is waiting for C's RAA, forward another A-to-C payment. B accepts it on the A-B
+	// channel, but cannot forward it over B-C yet, so it is held in B's holding cell.
+	let (route, payment_hash_2, _payment_preimage_2, payment_secret_2) =
+		get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+	let onion_2 = RecipientOnionFields::secret_only(payment_secret_2, 1_000_000);
+	let id_2 = PaymentId(payment_hash_2.0);
+	nodes[0].node.send_payment_with_route(route, payment_hash_2, onion_2, id_2).unwrap();
+	check_added_monitors(&nodes[0], 1);
+
+	let send_event = SendEvent::from_node(&nodes[0]);
+	assert_eq!(send_event.node_id, node_b_id);
+	assert_eq!(send_event.msgs.len(), 1);
+	nodes[1].node.handle_update_add_htlc(node_a_id, &send_event.msgs[0]);
+	do_commitment_signed_dance(&nodes[1], &nodes[0], &send_event.commitment_msg, false, false);
+
+	expect_and_process_pending_htlcs(&nodes[1], false);
+	check_added_monitors(&nodes[1], 0);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+	// Now make B owe C an RAA whose monitor update has already completed, but whose RAA cannot be
+	// constructed because B's signer is unavailable.
+	let (route, payment_hash_3, _payment_preimage_3, payment_secret_3) =
+		get_route_and_payment_hash!(nodes[2], nodes[0], 1_000_000);
+	let onion_3 = RecipientOnionFields::secret_only(payment_secret_3, 1_000_000);
+	let id_3 = PaymentId(payment_hash_3.0);
+	nodes[2].node.send_payment_with_route(route, payment_hash_3, onion_3, id_3).unwrap();
+	check_added_monitors(&nodes[2], 1);
+
+	let send_event = SendEvent::from_node(&nodes[2]);
+	assert_eq!(send_event.node_id, node_b_id);
+	assert_eq!(send_event.msgs.len(), 1);
+	nodes[1].node.handle_update_add_htlc(node_c_id, &send_event.msgs[0]);
+	nodes[1].disable_channel_signer_op(&node_c_id, &chan_bc.2, SignerOp::ReleaseCommitmentSecret);
+	nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &send_event.commitment_msg);
+	check_added_monitors(&nodes[1], 1);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+	// Deliver C's earlier RAA to B while monitor updating is blocked. This frees B's holding-cell
+	// HTLC and leaves a monitor update in flight.
+	chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
+	nodes[1].node.handle_revoke_and_ack(node_c_id, &pending_c_raa);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+	check_added_monitors(&nodes[1], 1);
+
+	nodes[1].node.peer_disconnected(node_c_id);
+	nodes[2].node.peer_disconnected(node_b_id);
+
+	let init_msg = msgs::Init {
+		features: nodes[2].node.init_features(),
+		networks: None,
+		remote_network_address: None,
+	};
+	nodes[1].node.peer_connected(node_c_id, &init_msg, true).unwrap();
+	let bs_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[2]);
+	assert_eq!(bs_reestablish.len(), 1);
+	let init_msg = msgs::Init {
+		features: nodes[1].node.init_features(),
+		networks: None,
+		remote_network_address: None,
+	};
+	nodes[2].node.peer_connected(node_b_id, &init_msg, false).unwrap();
+	let cs_reestablish = get_chan_reestablish_msgs!(nodes[2], nodes[1]);
+	assert_eq!(cs_reestablish.len(), 1);
+
+	nodes[1].node.handle_channel_reestablish(node_c_id, &cs_reestablish[0]);
+
+	// The signer-pending path now generates the owed RAA before the held monitor update
+	// completes.
+	nodes[1].enable_channel_signer_op(&node_c_id, &chan_bc.2, SignerOp::ReleaseCommitmentSecret);
+	nodes[1].node.signer_unblocked(Some((node_c_id, chan_bc.2)));
+	let (_, signer_revoke_and_ack, signer_commitment_update, _, _, _, _, _) =
+		handle_chan_reestablish_msgs!(nodes[1], nodes[2]);
+	assert!(signer_revoke_and_ack.is_some());
+
+	// Once the held monitor update completes, B must not generate the same RAA a second time via
+	// the monitor-pending path.
+	chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
+	let (latest_update, _) = nodes[1].chain_monitor.get_latest_mon_update_id(chan_bc.2);
+	nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(chan_bc.2, latest_update);
+	check_added_monitors(&nodes[1], 0);
+	let (_, duplicate_revoke_and_ack, monitor_commitment_update, _, _, _, _, _) =
+		handle_chan_reestablish_msgs!(nodes[1], nodes[2]);
+	assert!(duplicate_revoke_and_ack.is_none());
+
+	nodes[2].node.handle_channel_reestablish(node_b_id, &bs_reestablish[0]);
+	let (_, c_revoke_and_ack, c_commitment_update, _, _, _, _, _) =
+		handle_chan_reestablish_msgs!(nodes[2], nodes[1]);
+	assert!(c_revoke_and_ack.is_none());
+	assert!(c_commitment_update.is_none());
+
+	nodes[2].node.handle_revoke_and_ack(node_b_id, &signer_revoke_and_ack.unwrap());
+	check_added_monitors(&nodes[2], 1);
+
+	let commitment_update = signer_commitment_update.or(monitor_commitment_update);
+	if let Some(commitment_update) = commitment_update {
+		let send_event = SendEvent::from_commitment_update(node_c_id, chan_bc.2, commitment_update);
+		assert_eq!(send_event.node_id, node_c_id);
+		for update_add in send_event.msgs {
+			nodes[2].node.handle_update_add_htlc(node_b_id, &update_add);
+		}
+		nodes[2].node.handle_commitment_signed_batch_test(node_b_id, &send_event.commitment_msg);
+		check_added_monitors(&nodes[2], 1);
+		let (c_raa, c_commitment_signed) = get_revoke_commit_msgs(&nodes[2], &node_b_id);
+		nodes[1].node.handle_revoke_and_ack(node_c_id, &c_raa);
+		check_added_monitors(&nodes[1], 1);
+		nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &c_commitment_signed);
+		check_added_monitors(&nodes[1], 1);
+		let b_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, node_c_id);
+		nodes[2].node.handle_revoke_and_ack(node_b_id, &b_raa);
+		check_added_monitors(&nodes[2], 1);
+	}
+
+	let (route, final_payment_hash, _final_payment_preimage, final_payment_secret) =
+		get_route_and_payment_hash!(nodes[1], nodes[2], 100_000);
+	let final_payment_id = PaymentId(final_payment_hash.0);
+	nodes[1]
+		.node
+		.send_payment_with_route(
+			route,
+			final_payment_hash,
+			RecipientOnionFields::secret_only(final_payment_secret, 100_000),
+			final_payment_id,
+		)
+		.unwrap();
+	check_added_monitors(&nodes[1], 1);
+	let final_payment_event = nodes[1].node.get_and_clear_pending_msg_events().remove(0);
+	match &final_payment_event {
+		MessageSendEvent::UpdateHTLCs { node_id, .. } => assert_eq!(*node_id, node_c_id),
+		_ => panic!("Unexpected event"),
+	}
+	do_pass_along_path(
+		PassAlongPathArgs::new(
+			&nodes[1],
+			&[&nodes[2]],
+			100_000,
+			final_payment_hash,
+			final_payment_event,
+		)
+		.with_payment_secret(final_payment_secret)
+		.without_clearing_recipient_events(),
+	);
+
+	let claimable_events = nodes[2].node.get_and_clear_pending_events();
+	let final_claimable = claimable_events
+		.iter()
+		.find(|event| {
+			matches!(
+				event,
+				Event::PaymentClaimable { payment_hash, .. } if *payment_hash == final_payment_hash
+			)
+		})
+		.unwrap();
+	check_payment_claimable(
+		final_claimable,
+		final_payment_hash,
+		final_payment_secret,
+		100_000,
+		None,
+		node_c_id,
+	);
+	expect_htlc_failure_conditions(
+		nodes[1].node.get_and_clear_pending_events(),
+		&[HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_bc.2 }],
+	);
+}
+
 fn do_test_async_raa_peer_disconnect(
 	test_case: UnblockSignerAcrossDisconnectCase, raa_blocked_by_commit_point: bool,
 ) {

lightning/src/ln/channel.rs

@@ -10259,6 +10259,13 @@ where
 			self.context.signer_pending_commitment_update = true;
 			commitment_update = None;
 		}
+		if revoke_and_ack.is_some() {
+			// If signer-pending state regenerated an RAA, the monitor update for that RAA was
+			// already persisted before we set `signer_pending_revoke_and_ack`. Thus, if reconnect
+			// also marked the same RAA monitor-pending while another monitor update was in flight,
+			// the RAA we're returning here satisfies that monitor-pending resend.
+			self.context.monitor_pending_revoke_and_ack = false;
+		}
 
 		let (closing_signed, signed_closing_tx, shutdown_result) = if self.context.signer_pending_closing {
 			debug_assert!(self.context.last_sent_closing_fee.is_some());