fix duplicate HTLC fail-back on stale force-close

Anyitechs · Anyitechs · commit 42e9ba796092 · 2025-10-06T01:11:08.000+01:00
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
@@ -1287,7 +1287,7 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// a downstream channel force-close remaining unconfirmed by the time the upstream timeout
 	/// expires. This is used to tell us we already generated an event to fail this HTLC back
 	/// during a previous block scan.
-	failed_back_htlc_ids: HashSet<SentHTLCId>,
+	failed_back_htlc_ids: HashMap<SentHTLCId, u64>,
 
 	// The auxiliary HTLC data associated with a holder commitment transaction. This includes
 	// non-dust HTLC sources, along with dust HTLCs and their sources. Note that this assumes any
@@ -1885,7 +1885,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			initial_counterparty_commitment_tx: None,
 			balances_empty_height: None,
 
-			failed_back_htlc_ids: new_hash_set(),
+			failed_back_htlc_ids: new_hash_map(),
 
 			// There are never any HTLCs in the initial commitment transaction
 			current_holder_htlc_data: CommitmentHTLCData::new(),
@@ -5435,7 +5435,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		let mut matured_htlcs = Vec::new();
 
 		// Produce actionable events from on-chain events having reached their threshold.
-		for entry in onchain_events_reaching_threshold_conf {
+		for entry in onchain_events_reaching_threshold_conf.clone() {
 			match entry.event {
 				OnchainEvent::HTLCUpdate { source, payment_hash, htlc_value_satoshis, commitment_tx_output_idx } => {
 					// Check for duplicate HTLC resolutions.
@@ -5488,6 +5488,85 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				OnchainEvent::FundingSpendConfirmation { commitment_tx_to_counterparty_output, .. } => {
 					self.funding_spend_confirmed = Some(entry.txid);
 					self.confirmed_commitment_tx_counterparty_output = commitment_tx_to_counterparty_output;
+
+					// Only act on stale force-closes where the confirmed funding spend is the
+					// previous counterparty commitment transaction.
+					let prev_txid = match self.funding.prev_counterparty_commitment_txid {
+						Some(txid) => {
+							if txid != entry.txid { continue; }
+							txid
+						},
+						None => { continue; }
+					};
+
+					// Process only HTLCs from that previous counterparty commitment.
+					let prev_outputs = if let Some(outputs) =
+						self.funding.counterparty_claimable_outpoints.get(&prev_txid) {
+						outputs
+					} else { continue; };
+
+					// Build the set of HTLC ids present in the stale (previous) commitment.
+					let mut prev_ids: HashSet<SentHTLCId> = new_hash_set();
+					for &(_, ref source_opt) in prev_outputs.iter() {
+						if let Some(source) = source_opt.as_ref() {
+							prev_ids.insert(SentHTLCId::from_source(&**source));
+						}
+					}
+
+                    // Avoid duplicate fail-back if an HTLCUpdate for this source has already been
+                    // generated (either matured or still awaiting maturity).
+                    let mut seen_monitor_htlc_ids: HashSet<SentHTLCId> = new_hash_set();
+                    for e in onchain_events_reaching_threshold_conf.iter() {
+                        if let OnchainEvent::HTLCUpdate { source, .. } = &e.event {
+                            seen_monitor_htlc_ids.insert(SentHTLCId::from_source(source));
+                        }
+                    }
+                    for e in self.onchain_events_awaiting_threshold_conf.iter() {
+                        if let OnchainEvent::HTLCUpdate { source, .. } = &e.event {
+                            seen_monitor_htlc_ids.insert(SentHTLCId::from_source(source));
+                        }
+                    }
+
+					// Consider current live HTLCs (holder + current counterparty) with sources,
+					// and fail back only those NOT present in the stale (previous) commitment.
+                    let current_counterparty_iter = if let Some(txid) = self.funding.current_counterparty_commitment_txid {
+                        if let Some(htlc_outputs) = self.funding.counterparty_claimable_outpoints.get(&txid) {
+                            Some(htlc_outputs.iter().map(|&(ref a, ref b)| (a, b.as_ref().map(|boxed| &**boxed))))
+                        } else { None }
+                    } else { None }.into_iter().flatten();
+                    let current_htlcs = holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES)
+                        .chain(current_counterparty_iter);
+
+                    // Count expected duplicates per id from the current view to bound emissions.
+                    let mut expected_current_counts: HashMap<SentHTLCId, u64> = new_hash_map();
+                    for (_, source_opt) in current_htlcs.clone() {
+                        if let Some(source) = source_opt { *expected_current_counts.entry(SentHTLCId::from_source(source)).or_default() += 1; }
+                    }
+
+					for (htlc, source_opt) in current_htlcs {
+						if let Some(source) = source_opt {
+
+							let sent_id = SentHTLCId::from_source(source);
+							if prev_ids.contains(&sent_id) { continue; }
+							if seen_monitor_htlc_ids.contains(&sent_id) { continue; }
+
+							let already_emitted_count = *self.failed_back_htlc_ids.get(&sent_id).unwrap_or(&0);
+							let expected_count = *expected_current_counts.get(&sent_id).unwrap_or(&0);
+
+							if already_emitted_count < expected_count {
+								log_info!(logger,
+									"Detected stale force-close. Failing back all HTLCs for hash {}.",
+									&htlc.payment_hash);
+								self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
+									source: source.clone(),
+									payment_preimage: None,
+									payment_hash: htlc.payment_hash,
+									htlc_value_satoshis: Some(htlc.amount_msat / 1000),
+								}));
+								*self.failed_back_htlc_ids.entry(sent_id).or_default() += 1;
+							}
+						}
+					}
 				},
 				OnchainEvent::AlternativeFundingConfirmation {} => {
 					// An alternative funding transaction has irrevocably confirmed and we're no
@@ -5547,7 +5626,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				if duplicate_event {
 					continue;
 				}
-				if !self.failed_back_htlc_ids.insert(SentHTLCId::from_source(source)) {
+				let htlc_id = SentHTLCId::from_source(source);
+				if *self.failed_back_htlc_ids.get(&htlc_id).unwrap_or(&0) > 0 {
 					continue;
 				}
 				if !duplicate_event {
@@ -5560,6 +5640,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						payment_hash: htlc.payment_hash,
 						htlc_value_satoshis: Some(htlc.amount_msat / 1000),
 					}));
+					*self.failed_back_htlc_ids.entry(htlc_id).or_default() += 1;
 				}
 			}
 		}
@@ -6546,7 +6627,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			initial_counterparty_commitment_info,
 			initial_counterparty_commitment_tx,
 			balances_empty_height,
-			failed_back_htlc_ids: new_hash_set(),
+			failed_back_htlc_ids: new_hash_map(),
 
 			current_holder_htlc_data,
 			prev_holder_htlc_data,
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -32,8 +32,8 @@ use crate::ln::channel::{
 	MIN_CHAN_DUST_LIMIT_SATOSHIS, UNFUNDED_CHANNEL_AGE_LIMIT_TICKS,
 };
 use crate::ln::channelmanager::{
-	PaymentId, RAACommitmentOrder, RecipientOnionFields, BREAKDOWN_TIMEOUT, DISABLE_GOSSIP_TICKS,
-	ENABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA,
+	PaymentId, RAACommitmentOrder, RecipientOnionFields, Retry, BREAKDOWN_TIMEOUT,
+	DISABLE_GOSSIP_TICKS, ENABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA,
 };
 use crate::ln::msgs;
 use crate::ln::msgs::{
@@ -9680,3 +9680,147 @@ pub fn test_multi_post_event_actions() {
 	do_test_multi_post_event_actions(true);
 	do_test_multi_post_event_actions(false);
 }
+
+#[xtest(feature = "_externalize_tests")]
+fn test_stale_force_close_with_identical_htlcs() {
+	// Test that when two identical HTLCs are relayed and force-closes
+	// with a stale state, that we fail both HTLCs back immediately.
+	let chanmon_cfgs = create_chanmon_cfgs(4);
+	let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
+	let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);
+
+	let chan_a_b = create_announced_chan_between_nodes(&nodes, 0, 1);
+	let _chan_b_c = create_announced_chan_between_nodes(&nodes, 1, 2);
+	let _chan_d_b = create_announced_chan_between_nodes(&nodes, 3, 1);
+
+	let (_payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
+
+	let payment_params = PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), 100);
+	let scorer = test_utils::TestScorer::new();
+	let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
+	let route_params = RouteParameters::from_payment_params_and_value(payment_params, 1_000_000);
+
+	let route = get_route(
+		&nodes[0].node.get_our_node_id(),
+		&route_params,
+		&nodes[0].network_graph.read_only(),
+		None,
+		nodes[0].logger,
+		&scorer,
+		&Default::default(),
+		&random_seed_bytes,
+	)
+	.unwrap();
+
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
+	nodes[0]
+		.node
+		.send_payment(
+			payment_hash,
+			RecipientOnionFields::secret_only(payment_secret),
+			PaymentId([1; 32]),
+			route_params.clone(),
+			Retry::Attempts(0),
+		)
+		.unwrap();
+
+	let ev1 = remove_first_msg_event_to_node(
+		&nodes[1].node.get_our_node_id(),
+		&mut nodes[0].node.get_and_clear_pending_msg_events(),
+	);
+	let mut send_ev1 = SendEvent::from_event(ev1);
+
+	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &send_ev1.msgs[0]);
+	nodes[1].node.handle_commitment_signed_batch_test(
+		nodes[0].node.get_our_node_id(),
+		&send_ev1.commitment_msg,
+	);
+
+	let mut b_events = nodes[1].node.get_and_clear_pending_msg_events();
+	for ev in b_events.drain(..) {
+		match ev {
+			MessageSendEvent::SendRevokeAndACK { node_id, msg } => {
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+				nodes[0].node.handle_revoke_and_ack(nodes[1].node.get_our_node_id(), &msg);
+			},
+			MessageSendEvent::UpdateHTLCs { node_id, updates, .. } => {
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+				nodes[0].node.handle_commitment_signed_batch_test(
+					nodes[1].node.get_our_node_id(),
+					&updates.commitment_signed,
+				);
+				let mut a_events = nodes[0].node.get_and_clear_pending_msg_events();
+				for a_ev in a_events.drain(..) {
+					if let MessageSendEvent::SendRevokeAndACK { node_id, msg } = a_ev {
+						assert_eq!(node_id, nodes[1].node.get_our_node_id());
+						nodes[1].node.handle_revoke_and_ack(nodes[0].node.get_our_node_id(), &msg);
+					}
+				}
+			},
+			_ => {},
+		}
+	}
+
+	nodes[1].node.process_pending_htlc_forwards();
+	let _ = nodes[1].node.get_and_clear_pending_msg_events();
+
+	let stale_commitment_tx = get_local_commitment_txn!(nodes[0], chan_a_b.2)[0].clone();
+
+	*nodes[0].network_payment_count.borrow_mut() -= 1;
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
+	nodes[0]
+		.node
+		.send_payment(
+			payment_hash,
+			RecipientOnionFields::secret_only(payment_secret),
+			PaymentId([2; 32]),
+			route_params.clone(),
+			Retry::Attempts(0),
+		)
+		.unwrap();
+
+	let ev2 = remove_first_msg_event_to_node(
+		&nodes[1].node.get_our_node_id(),
+		&mut nodes[0].node.get_and_clear_pending_msg_events(),
+	);
+	let mut send_ev2 = SendEvent::from_event(ev2);
+
+	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &send_ev2.msgs[0]);
+	nodes[1].node.handle_commitment_signed_batch_test(
+		nodes[0].node.get_our_node_id(),
+		&send_ev2.commitment_msg,
+	);
+
+	let mut b2_events = nodes[1].node.get_and_clear_pending_msg_events();
+	for ev in b2_events.drain(..) {
+		match ev {
+			MessageSendEvent::SendRevokeAndACK { node_id, msg } => {
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+				nodes[0].node.handle_revoke_and_ack(nodes[1].node.get_our_node_id(), &msg);
+			},
+			MessageSendEvent::UpdateHTLCs { node_id, updates, .. } => {
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+				nodes[0].node.handle_commitment_signed_batch_test(
+					nodes[1].node.get_our_node_id(),
+					&updates.commitment_signed,
+				);
+			},
+			_ => {},
+		}
+	}
+
+	nodes[1].node.process_pending_htlc_forwards();
+	let _ = nodes[1].node.get_and_clear_pending_msg_events();
+
+	mine_transaction(&nodes[1], &stale_commitment_tx);
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY);
+
+	let events = nodes[1].node.get_and_clear_pending_events();
+	let failed_count =
+		events.iter().filter(|e| matches!(e, Event::HTLCHandlingFailed { .. })).count();
+	assert_eq!(failed_count, 2);
+
+	check_added_monitors!(&nodes[1], 1);
+	nodes[1].node.get_and_clear_pending_msg_events();
+}
