fix duplicate HTLC fail-back on stale force-close

Author: Anyitechs

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.
@@ -5502,6 +5502,81 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			}
 		}
 
+		// Immediate fail-back on stale force-close, regardless of expiry or whether we're allowed to send further updates.
+		let current_counterparty_htlcs = 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 prev_counterparty_htlcs = if let Some(txid) = self.funding.prev_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 htlcs = holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES)
+			.chain(current_counterparty_htlcs)
+			.chain(prev_counterparty_htlcs);
+
+		// To correctly handle duplicate HTLCs, we first count all expected instances from
+		// the commitment transactions.
+		let mut expected_htlc_counts: HashMap<SentHTLCId, u64> = new_hash_map();
+		for (_, source_opt) in htlcs.clone() {
+			if let Some(source) = source_opt {
+				*expected_htlc_counts.entry(SentHTLCId::from_source(source)).or_default() += 1;
+			}
+		}
+
+		// Get a lookup of all HTLCs the monitor is currently tracking on-chain.
+		let monitor_htlc_sources: HashSet<&HTLCSource> = onchain_events_reaching_threshold_conf
+			.iter()
+			.filter_map(|event_entry| match &event_entry.event {
+				OnchainEvent::HTLCUpdate { source, .. } => Some(source),
+				_ => None,
+			})
+			.collect();
+
+		// Group all in-flight HTLCs by payment hash to handle duplicates correctly.
+		let mut htlcs_by_hash: HashMap<PaymentHash, Vec<(&HTLCOutputInCommitment, &HTLCSource)>> = new_hash_map();
+		for (htlc, source_opt) in htlcs {
+			if let Some(source) = source_opt {
+				htlcs_by_hash.entry(htlc.payment_hash).or_default().push((htlc, source));
+			}
+		}
+
+		for (payment_hash, htlc_group) in htlcs_by_hash {
+			// If any HTLC in this group is missing from the monitor's on-chain view,
+			// it indicates a stale state was used. We must fail back the entire group.
+			let is_any_htlc_missing = htlc_group
+				.iter()
+				.any(|(_, source)| !monitor_htlc_sources.contains(source));
+			if is_any_htlc_missing {
+				log_info!(logger,
+					"Detected stale force-close. Failing back HTLCs for hash {}.",
+					&payment_hash);
+				for (htlc, source) in htlc_group {
+					let htlc_id = SentHTLCId::from_source(source);
+					let already_failed_count = *self.failed_back_htlc_ids.get(&htlc_id).unwrap_or(&0);
+					let expected_count = *expected_htlc_counts.get(&htlc_id).unwrap_or(&0);
+
+					// Only fail back if we haven't already failed all expected instances.
+					if already_failed_count < expected_count {
+						log_error!(logger,
+							"Failing back HTLC for payment {} due to stale close.",
+							log_bytes!(payment_hash.0));
+						self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
+							source: source.clone(),
+							payment_preimage: None,
+							payment_hash,
+							htlc_value_satoshis: Some(htlc.amount_msat / 1000),
+						}));
+						*self.failed_back_htlc_ids.entry(htlc_id).or_default() += 1;
+					}
+				}
+			}
+		}
+
 		if self.no_further_updates_allowed() {
 			// Fail back HTLCs on backwards channels if they expire within
 			// `LATENCY_GRACE_PERIOD_BLOCKS` blocks and the channel is closed (i.e. we're at a
@@ -5547,7 +5622,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 +5636,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 +6623,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,

lightning/src/ln/functional_tests.rs

@@ -9680,3 +9680,40 @@ 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_b_d = create_announced_chan_between_nodes(&nodes, 1, 3);
+
+	// Capture a stale state snapshot before adding any HTLCs
+	let stale_tx = get_local_commitment_txn!(nodes[0], chan_a_b.2)[0].clone();
+
+	// Create two identical HTLCs
+	let (payment_preimage, payment_hash, ..) =
+		route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 10_000);
+
+	*nodes[0].network_payment_count.borrow_mut() -= 1;
+	let (payment_preimage_2, payment_hash_2, ..) = route_payment(&nodes[1], &[&nodes[3]], 10_000);
+
+	assert_eq!(payment_hash, payment_hash_2);
+	assert_eq!(payment_preimage, payment_preimage_2);
+
+	mine_transaction(&nodes[1], &stale_tx);
+
+	let events = nodes[1].node.get_and_clear_pending_events();
+	let failed_events_count =
+		events.iter().filter(|e| matches!(e, Event::HTLCHandlingFailed { .. })).count();
+	assert_eq!(failed_events_count, 2);
+
+	check_added_monitors!(&nodes[1], 1);
+	nodes[1].node.get_and_clear_pending_msg_events();
+}