From 2fbaf96a3c94e29225028f699e840f26e46e8fce Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 7 Oct 2025 10:58:20 -0700 Subject: [PATCH 1/2] Rustfmt get_counterparty_output_claim_info We plan to rework this method in the following commit, so we start by formatting it first. --- lightning/src/chain/channelmonitor.rs | 57 +++++++++++++++++++-------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5f2bb248cd1..c4bc61deed2 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -4733,7 +4733,6 @@ impl ChannelMonitorImpl { } /// Returns the HTLC claim package templates and the counterparty output info - #[rustfmt::skip] fn get_counterparty_output_claim_info( &self, funding_spent: &FundingScope, commitment_number: u64, commitment_txid: Txid, tx: Option<&Transaction>, @@ -4767,13 +4766,23 @@ impl ChannelMonitorImpl { if let Some(transaction) = tx { let revocation_pubkey = RevocationKey::from_basepoint( - &self.onchain_tx_handler.secp_ctx, &self.holder_revocation_basepoint, &per_commitment_point); + &self.onchain_tx_handler.secp_ctx, + &self.holder_revocation_basepoint, + &per_commitment_point, + ); - let delayed_key = DelayedPaymentKey::from_basepoint(&self.onchain_tx_handler.secp_ctx, &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, &per_commitment_point); + let delayed_key = DelayedPaymentKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, + &per_commitment_point, + ); - let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, + let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript( + &revocation_pubkey, self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key).to_p2wsh(); + &delayed_key, + ) + .to_p2wsh(); for (idx, outp) in transaction.output.iter().enumerate() { if outp.script_pubkey == revokeable_p2wsh { to_counterparty_output_info = @@ -4782,25 +4791,36 @@ impl ChannelMonitorImpl { } } - for &(ref htlc, _) in per_commitment_claimable_data.iter() { + for &(ref htlc, _) in per_commitment_claimable_data.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { if let Some(transaction) = tx { - if transaction_output_index as usize >= transaction.output.len() || - transaction.output[transaction_output_index as usize].value != htlc.to_bitcoin_amount() { - // per_commitment_data is corrupt or our commitment signing key leaked! - return (claimable_outpoints, to_counterparty_output_info); - } + if transaction_output_index as usize >= transaction.output.len() + || transaction.output[transaction_output_index as usize].value + != htlc.to_bitcoin_amount() + { + // per_commitment_data is corrupt or our commitment signing key leaked! + return (claimable_outpoints, to_counterparty_output_info); + } } - let preimage = if htlc.offered { if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None }; + let preimage = if htlc.offered { + if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { + Some(*p) + } else { + None + } + } else { + None + }; if preimage.is_some() || !htlc.offered { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( CounterpartyOfferedHTLCOutput::build( - *per_commitment_point, preimage.unwrap(), + *per_commitment_point, + preimage.unwrap(), htlc.clone(), funding_spent.channel_parameters.clone(), confirmation_height, - ) + ), ) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( @@ -4809,10 +4829,15 @@ impl ChannelMonitorImpl { htlc.clone(), funding_spent.channel_parameters.clone(), confirmation_height, - ) + ), ) }; - let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry); + let counterparty_package = PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + counterparty_htlc_outp, + htlc.cltv_expiry, + ); claimable_outpoints.push(counterparty_package); } } From 46f0d3148f6f4fa00c89b737db4a45282cc057b3 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 7 Oct 2025 10:57:43 -0700 Subject: [PATCH 2/2] Only claim HTLCs with matching payment hash upon preimage monitor update Previously, we'd attempt to claim all HTLCs that have expired or that we have the preimage for on each preimage monitor update. This happened due to reusing the code path (`get_counterparty_output_claim_info`) used when producing all claims for a newly confirmed counterparty commitment. Unfortunately, this can result in invalid claim transactions and ultimately in loss of funds (if the HTLC expires and the counterparty claims it via the timeout), as it didn't consider that some of those HTLCs may have already been claimed by a separate transaction. This commit changes the behavior when handling preimage monitor updates only. We will now only attempt to claim HTLCs for the specific preimage that we learned via the monitor update. This is safe to do, as even if a preimage HTLC claim transaction is reorged out, the `OnchainTxHandler` is responsible for continuous claiming attempts until we see a reorg of the corresponding commitment transaction. --- lightning/src/chain/channelmonitor.rs | 158 ++++++++++++++++---------- lightning/src/ln/monitor_tests.rs | 82 +++++++++++++ 2 files changed, 183 insertions(+), 57 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index c4bc61deed2..a171b831861 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3823,7 +3823,7 @@ impl ChannelMonitorImpl { // First check if a counterparty commitment transaction has been broadcasted: macro_rules! claim_htlcs { ($commitment_number: expr, $txid: expr, $htlcs: expr) => { - let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info(funding_spent, $commitment_number, $txid, None, $htlcs, confirmed_spend_height); + let htlc_claim_reqs = self.get_counterparty_output_claims_for_preimage(*payment_preimage, funding_spent, $commitment_number, $txid, $htlcs, confirmed_spend_height); let conf_target = self.closure_conf_target(); self.onchain_tx_handler.update_claims_view_from_requests( htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, @@ -4722,7 +4722,7 @@ impl ChannelMonitorImpl { (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) ), logger); let (htlc_claim_reqs, counterparty_output_info) = - self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, Some(commitment_tx), per_commitment_option, Some(height)); + self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, commitment_tx, per_commitment_claimable_data, Some(height)); to_counterparty_output_info = counterparty_output_info; for req in htlc_claim_reqs { claimable_outpoints.push(req); @@ -4732,75 +4732,119 @@ impl ChannelMonitorImpl { (claimable_outpoints, to_counterparty_output_info) } + fn get_point_for_commitment_number(&self, commitment_number: u64) -> Option { + let per_commitment_points = &self.their_cur_per_commitment_points?; + + // If the counterparty commitment tx is the latest valid state, use their latest + // per-commitment point + if per_commitment_points.0 == commitment_number { + Some(per_commitment_points.1) + } else if let Some(point) = per_commitment_points.2.as_ref() { + // If counterparty commitment tx is the state previous to the latest valid state, use + // their previous per-commitment point (non-atomicity of revocation means it's valid for + // them to temporarily have two valid commitment txns from our viewpoint) + if per_commitment_points.0 == commitment_number + 1 { + Some(*point) + } else { + None + } + } else { + None + } + } + + fn get_counterparty_output_claims_for_preimage( + &self, preimage: PaymentPreimage, funding_spent: &FundingScope, commitment_number: u64, + commitment_txid: Txid, + per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, + confirmation_height: Option, + ) -> Vec { + let per_commitment_claimable_data = match per_commitment_option { + Some(outputs) => outputs, + None => return Vec::new(), + }; + let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) { + Some(point) => point, + None => return Vec::new(), + }; + + let matching_payment_hash = PaymentHash::from(preimage); + per_commitment_claimable_data + .iter() + .filter_map(|(htlc, _)| { + if let Some(transaction_output_index) = htlc.transaction_output_index { + if htlc.offered && htlc.payment_hash == matching_payment_hash { + let htlc_data = PackageSolvingData::CounterpartyOfferedHTLCOutput( + CounterpartyOfferedHTLCOutput::build( + per_commitment_point, + preimage, + htlc.clone(), + funding_spent.channel_parameters.clone(), + confirmation_height, + ), + ); + Some(PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + htlc_data, + htlc.cltv_expiry, + )) + } else { + None + } + } else { + None + } + }) + .collect() + } + /// Returns the HTLC claim package templates and the counterparty output info fn get_counterparty_output_claim_info( &self, funding_spent: &FundingScope, commitment_number: u64, commitment_txid: Txid, - tx: Option<&Transaction>, - per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, + tx: &Transaction, + per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option>)], confirmation_height: Option, ) -> (Vec, CommitmentTxCounterpartyOutputInfo) { let mut claimable_outpoints = Vec::new(); let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None; - let per_commitment_claimable_data = match per_commitment_option { - Some(outputs) => outputs, - None => return (claimable_outpoints, to_counterparty_output_info), - }; - let per_commitment_points = match self.their_cur_per_commitment_points { - Some(points) => points, + let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) { + Some(point) => point, None => return (claimable_outpoints, to_counterparty_output_info), }; - let per_commitment_point = - // If the counterparty commitment tx is the latest valid state, use their latest - // per-commitment point - if per_commitment_points.0 == commitment_number { &per_commitment_points.1 } - else if let Some(point) = per_commitment_points.2.as_ref() { - // If counterparty commitment tx is the state previous to the latest valid state, use - // their previous per-commitment point (non-atomicity of revocation means it's valid for - // them to temporarily have two valid commitment txns from our viewpoint) - if per_commitment_points.0 == commitment_number + 1 { - point - } else { return (claimable_outpoints, to_counterparty_output_info); } - } else { return (claimable_outpoints, to_counterparty_output_info); }; - - if let Some(transaction) = tx { - let revocation_pubkey = RevocationKey::from_basepoint( - &self.onchain_tx_handler.secp_ctx, - &self.holder_revocation_basepoint, - &per_commitment_point, - ); - - let delayed_key = DelayedPaymentKey::from_basepoint( - &self.onchain_tx_handler.secp_ctx, - &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, - &per_commitment_point, - ); - - let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript( - &revocation_pubkey, - self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key, - ) - .to_p2wsh(); - for (idx, outp) in transaction.output.iter().enumerate() { - if outp.script_pubkey == revokeable_p2wsh { - to_counterparty_output_info = - Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); - } + let revocation_pubkey = RevocationKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.holder_revocation_basepoint, + &per_commitment_point, + ); + let delayed_key = DelayedPaymentKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, + &per_commitment_point, + ); + let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript( + &revocation_pubkey, + self.counterparty_commitment_params.on_counterparty_tx_csv, + &delayed_key, + ) + .to_p2wsh(); + for (idx, outp) in tx.output.iter().enumerate() { + if outp.script_pubkey == revokeable_p2wsh { + to_counterparty_output_info = + Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); } } for &(ref htlc, _) in per_commitment_claimable_data.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { - if let Some(transaction) = tx { - if transaction_output_index as usize >= transaction.output.len() - || transaction.output[transaction_output_index as usize].value - != htlc.to_bitcoin_amount() - { - // per_commitment_data is corrupt or our commitment signing key leaked! - return (claimable_outpoints, to_counterparty_output_info); - } + if transaction_output_index as usize >= tx.output.len() + || tx.output[transaction_output_index as usize].value + != htlc.to_bitcoin_amount() + { + // per_commitment_data is corrupt or our commitment signing key leaked! + return (claimable_outpoints, to_counterparty_output_info); } let preimage = if htlc.offered { if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { @@ -4815,7 +4859,7 @@ impl ChannelMonitorImpl { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( CounterpartyOfferedHTLCOutput::build( - *per_commitment_point, + per_commitment_point, preimage.unwrap(), htlc.clone(), funding_spent.channel_parameters.clone(), @@ -4825,7 +4869,7 @@ impl ChannelMonitorImpl { } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( CounterpartyReceivedHTLCOutput::build( - *per_commitment_point, + per_commitment_point, htlc.clone(), funding_spent.channel_parameters.clone(), confirmation_height, diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index b316381398e..354cd48c6be 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -3836,3 +3836,85 @@ fn test_lost_timeout_monitor_events() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, true); do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, true); } + +#[test] +fn test_ladder_preimage_htlc_claims() { + // Tests that when we learn of a preimage via a monitor update we only claim HTLCs with the + // corresponding payment hash. This test is a reproduction of a scenario that happened in + // production where the second HTLC claim also included the first HTLC (even though it was + // already claimed) resulting in an invalid claim transaction. + 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 mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + + let (_, _, channel_id, _) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0); + + let (payment_preimage1, payment_hash1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + let (payment_preimage2, payment_hash2, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &node_id_1, "test".to_string()).unwrap(); + check_added_monitors(&nodes[0], 1); + check_closed_broadcast(&nodes[0], 1, true); + let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message: "test".to_string() }; + check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 1_000_000); + + let commitment_tx = { + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + txn.remove(0) + }; + mine_transaction(&nodes[0], &commitment_tx); + mine_transaction(&nodes[1], &commitment_tx); + + check_closed_broadcast(&nodes[1], 1, true); + check_added_monitors(&nodes[1], 1); + check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[node_id_0], 1_000_000); + + nodes[1].node.claim_funds(payment_preimage1); + expect_payment_claimed!(&nodes[1], payment_hash1, 1_000_000); + check_added_monitors(&nodes[1], 1); + + let (htlc1, htlc_claim_tx1) = { + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + let htlc_claim_tx = txn.remove(0); + assert_eq!(htlc_claim_tx.input.len(), 1); + check_spends!(htlc_claim_tx, commitment_tx); + (htlc_claim_tx.input[0].previous_output, htlc_claim_tx) + }; + mine_transaction(&nodes[0], &htlc_claim_tx1); + mine_transaction(&nodes[1], &htlc_claim_tx1); + + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + expect_payment_sent(&nodes[0], payment_preimage1, None, true, false); + check_added_monitors(&nodes[0], 1); + + nodes[1].node.claim_funds(payment_preimage2); + expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000); + check_added_monitors(&nodes[1], 1); + + let (htlc2, htlc_claim_tx2) = { + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1, "{:?}", txn.iter().map(|tx| tx.compute_txid()).collect::>()); + let htlc_claim_tx = txn.remove(0); + assert_eq!(htlc_claim_tx.input.len(), 1); + check_spends!(htlc_claim_tx, commitment_tx); + (htlc_claim_tx.input[0].previous_output, htlc_claim_tx) + }; + assert_ne!(htlc1, htlc2); + + mine_transaction(&nodes[0], &htlc_claim_tx2); + mine_transaction(&nodes[1], &htlc_claim_tx2); + + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + expect_payment_sent(&nodes[0], payment_preimage2, None, true, false); + check_added_monitors(&nodes[0], 1); +}