From 2a5ac7de2ebe8cc7f419b51958532638ff292e38 Mon Sep 17 00:00:00 2001 From: Anyitechs Date: Tue, 9 Sep 2025 01:12:11 +0100 Subject: [PATCH] fix duplicate HTLC fail-back on stale force-close --- lightning/src/chain/channelmonitor.rs | 91 +++++++++++++++- lightning/src/ln/functional_tests.rs | 148 +++++++++++++++++++++++++- 2 files changed, 232 insertions(+), 7 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5f2bb248cd1..f38899817bf 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1356,7 +1356,7 @@ pub(crate) struct ChannelMonitorImpl { /// 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, + failed_back_htlc_ids: HashMap, // 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 @@ -1956,7 +1956,7 @@ impl ChannelMonitor { 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(), @@ -5523,7 +5523,7 @@ impl ChannelMonitorImpl { 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. @@ -5587,6 +5587,85 @@ impl ChannelMonitorImpl { }); } } + + // 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 = 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 = 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 = 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 @@ -5646,7 +5725,8 @@ impl ChannelMonitorImpl { 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 { @@ -5659,6 +5739,7 @@ impl ChannelMonitorImpl { payment_hash: htlc.payment_hash, htlc_value_satoshis: Some(htlc.amount_msat / 1000), })); + *self.failed_back_htlc_ids.entry(htlc_id).or_default() += 1; } } } @@ -6650,7 +6731,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 index d1c0ac8f12b..185b688637b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -33,8 +33,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::{ @@ -9703,3 +9703,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(); +}