Only claim HTLCs with matching payment hash upon preimage monitor update

wpaulino · wpaulino · commit 46f0d3148f6f · 2025-10-07T11:58:04.000-07:00
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.
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
@@ -3823,7 +3823,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		// 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<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					(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<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		(claimable_outpoints, to_counterparty_output_info)
 	}
 
+	fn get_point_for_commitment_number(&self, commitment_number: u64) -> Option<PublicKey> {
+		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<Box<HTLCSource>>)>>,
+		confirmation_height: Option<u32>,
+	) -> Vec<PackageTemplate> {
+		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<Box<HTLCSource>>)>>,
+		tx: &Transaction,
+		per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option<Box<HTLCSource>>)],
 		confirmation_height: Option<u32>,
 	) -> (Vec<PackageTemplate>, 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<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					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<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					} 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
@@ -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::<Vec<_>>());
+		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);
+}
