Avoid duplicate on-chain HTLC claims after replay

fuzz/src/chanmon_consistency.rs

@@ -208,9 +208,7 @@ impl ChainState {
 
 	fn is_outpoint_spent(&self, outpoint: &bitcoin::OutPoint) -> bool {
 		self.blocks.iter().any(|(_, txs)| {
-			txs.iter().any(|tx| {
-				tx.input.iter().any(|input| input.previous_output == *outpoint)
-			})
+			txs.iter().any(|tx| tx.input.iter().any(|input| input.previous_output == *outpoint))
 		})
 	}
 
@@ -1027,7 +1025,8 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], out: Out) {
 			}
 			let network = Network::Bitcoin;
 			let best_block_timestamp = genesis_block(network).header.time;
-			let params = ChainParameters { network, best_block: BlockLocator::from_network(network) };
+			let params =
+				ChainParameters { network, best_block: BlockLocator::from_network(network) };
 			(
 				ChannelManager::new(
 					$fee_estimator.clone(),
@@ -1142,8 +1141,8 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], out: Out) {
 			channel_monitors: monitor_refs,
 		};
 
-		let manager =
-			<(BlockLocator, ChanMan)>::read(&mut &ser[..], read_args).expect("Failed to read manager");
+		let manager = <(BlockLocator, ChanMan)>::read(&mut &ser[..], read_args)
+			.expect("Failed to read manager");
 		let res = (manager.1, chain_monitor.clone());
 		for (channel_id, mon) in monitors.drain() {
 			assert_eq!(
@@ -2106,7 +2105,8 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], out: Out) {
 						},
 						events::Event::SpliceFailed { .. } => {},
 						events::Event::DiscardFunding {
-							funding_info: events::FundingInfo::Contribution { .. }
+							funding_info:
+								events::FundingInfo::Contribution { .. }
 								| events::FundingInfo::Tx { .. },
 							..
 						} => {},

lightning/src/chain/channelmonitor.rs

@@ -38,8 +38,8 @@ use crate::chain::chaininterface::{
 };
 use crate::chain::onchaintx::{ClaimEvent, FeerateStrategy, OnchainTxHandler};
 use crate::chain::package::{
-	CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput,
-	HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedHTLCOutput, RevokedOutput,
+	ClaimRequest, CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput,
+	HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, RevokedHTLCOutput, RevokedOutput,
 };
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::chain::{BlockLocator, WatchedOutput};
@@ -3861,15 +3861,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	fn generate_claimable_outpoints_and_watch_outputs(
 		&mut self, generate_monitor_event_with_reason: Option<ClosureReason>,
 		require_funding_seen: bool,
-	) -> (Vec<PackageTemplate>, Vec<TransactionOutputs>) {
+	) -> (Vec<ClaimRequest>, Vec<TransactionOutputs>) {
 		let funding = get_confirmed_funding_scope!(self);
 		let holder_commitment_tx = &funding.current_holder_commitment_tx;
 		let funding_outp = HolderFundingOutput::build(
 			holder_commitment_tx.clone(),
 			funding.channel_parameters.clone(),
 		);
 		let funding_outpoint = funding.funding_outpoint();
-		let commitment_package = PackageTemplate::build_package(
+		let commitment_package = ClaimRequest::new(
 			funding_outpoint.txid.clone(), funding_outpoint.index as u32,
 			PackageSolvingData::HolderFundingOutput(funding_outp),
 			self.best_block.height,
@@ -3908,9 +3908,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		let zero_fee_commitments =
 			self.channel_type_features().supports_anchor_zero_fee_commitments();
 		if !zero_fee_htlcs && !zero_fee_commitments {
-			// Because we're broadcasting a commitment transaction, we should construct the package
-			// assuming it gets confirmed in the next block. Sadly, we have code which considers
-			// "not yet confirmed" things as discardable, so we cannot do that here.
+			// Because we're broadcasting a commitment transaction, we should construct claim
+			// requests assuming it gets confirmed in the next block. Sadly, we have code which
+			// considers "not yet confirmed" things as discardable, so we cannot do that here.
 			let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
 				funding, holder_commitment_tx, self.best_block.height,
 			);
@@ -4759,11 +4759,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	/// height > height + CLTV_SHARED_CLAIM_BUFFER. In any case, will install monitoring for
 	/// HTLC-Success/HTLC-Timeout transactions.
 	///
-	/// Returns packages to claim the revoked output(s) and general information about the output that
-	/// is to the counterparty in the commitment transaction.
+	/// Returns claim requests for the revoked output(s) and general information about the output
+	/// that is to the counterparty in the commitment transaction.
 	#[rustfmt::skip]
 	fn check_spend_counterparty_transaction<L: Logger>(&mut self, commitment_txid: Txid, commitment_tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &L)
-		-> (Vec<PackageTemplate>, CommitmentTxCounterpartyOutputInfo)
+		-> (Vec<ClaimRequest>, CommitmentTxCounterpartyOutputInfo)
 	{
 		// Most secp and related errors trying to create keys means we have no hope of constructing
 		// a spend transaction...so we return no transactions to broadcast
@@ -4803,7 +4803,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						per_commitment_point, per_commitment_key, outp.value,
 						funding_spent.channel_parameters.clone(), height,
 					);
-					let justice_package = PackageTemplate::build_package(
+					let justice_package = ClaimRequest::new(
 						commitment_txid, idx as u32,
 						PackageSolvingData::RevokedOutput(revk_outp),
 						height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32,
@@ -4832,7 +4832,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						} else {
 							height
 						};
-						let justice_package = PackageTemplate::build_package(
+						let justice_package = ClaimRequest::new(
 							commitment_txid,
 							transaction_output_index,
 							PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
@@ -4921,7 +4921,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		commitment_txid: Txid,
 		per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
 		confirmation_height: Option<u32>,
-	) -> Vec<PackageTemplate> {
+	) -> Vec<ClaimRequest> {
 		let per_commitment_claimable_data = match per_commitment_option {
 			Some(outputs) => outputs,
 			None => return Vec::new(),
@@ -4936,7 +4936,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			.iter()
 			.filter_map(|(htlc, _)| {
 				if let Some(transaction_output_index) = htlc.transaction_output_index {
-					if htlc.offered && htlc.payment_hash == matching_payment_hash {
+					if htlc.offered
+						&& htlc.payment_hash == matching_payment_hash
+						&& !self.is_htlc_output_spent_on_chain(htlc)
+					{
 						let htlc_data = PackageSolvingData::CounterpartyOfferedHTLCOutput(
 							CounterpartyOfferedHTLCOutput::build(
 								per_commitment_point,
@@ -4946,7 +4949,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 								confirmation_height,
 							),
 						);
-						Some(PackageTemplate::build_package(
+						Some(ClaimRequest::new(
 							commitment_txid,
 							transaction_output_index,
 							htlc_data,
@@ -4962,13 +4965,28 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			.collect()
 	}
 
-	/// Returns the HTLC claim package templates and the counterparty output info
+	fn is_htlc_output_spent_on_chain(&self, htlc: &HTLCOutputInCommitment) -> bool {
+		if let Some(transaction_output_index) = htlc.transaction_output_index {
+			// Only suppress claims once the monitor has persisted final HTLC
+			// resolution. While a conflicting spend is still awaiting anti-reorg
+			// confirmation, a replayed preimage may create a live conflicting
+			// claim; keeping that claim in OnchainTxHandler preserves retry state
+			// if the spend reorgs out.
+			self.htlcs_resolved_on_chain.iter().any(|resolved_htlc| {
+				resolved_htlc.commitment_tx_output_idx == Some(transaction_output_index)
+			})
+		} else {
+			false
+		}
+	}
+
+	/// Returns the HTLC claim requests and the counterparty output info.
 	fn get_counterparty_output_claim_info(
 		&self, funding_spent: &FundingScope, commitment_number: u64, commitment_txid: Txid,
 		tx: &Transaction,
 		per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option<Box<HTLCSource>>)],
 		confirmation_height: Option<u32>,
-	) -> (Vec<PackageTemplate>, CommitmentTxCounterpartyOutputInfo) {
+	) -> (Vec<ClaimRequest>, CommitmentTxCounterpartyOutputInfo) {
 		let mut claimable_outpoints = Vec::new();
 		let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None;
 
@@ -5009,6 +5027,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					// per_commitment_data is corrupt or our commitment signing key leaked!
 					return (claimable_outpoints, to_counterparty_output_info);
 				}
+				if self.is_htlc_output_spent_on_chain(htlc) {
+					continue;
+				}
 				let preimage = if htlc.offered {
 					if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) {
 						Some(*p)
@@ -5039,7 +5060,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							),
 						)
 					};
-					let counterparty_package = PackageTemplate::build_package(
+					let counterparty_package = ClaimRequest::new(
 						commitment_txid,
 						transaction_output_index,
 						counterparty_htlc_outp,
@@ -5057,7 +5078,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	#[rustfmt::skip]
 	fn check_spend_counterparty_htlc<L: Logger>(
 		&mut self, tx: &Transaction, commitment_number: u64, commitment_txid: &Txid, height: u32, logger: &L
-	) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) {
+	) -> (Vec<ClaimRequest>, Option<TransactionOutputs>) {
 		let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
 		let per_commitment_key = match SecretKey::from_slice(&secret) {
 			Ok(key) => key,
@@ -5088,7 +5109,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					per_commitment_point, per_commitment_key, tx.output[idx].value,
 					self.funding.channel_parameters.clone(), height,
 				);
-				let justice_package = PackageTemplate::build_package(
+				let justice_package = ClaimRequest::new(
 					htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp),
 					height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32,
 				);
@@ -5110,6 +5131,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		let mut htlcs = Vec::with_capacity(holder_tx.nondust_htlcs().len());
 		debug_assert_eq!(holder_tx.nondust_htlcs().len(), holder_tx.counterparty_htlc_sigs.len());
 		for (htlc, counterparty_sig) in holder_tx.nondust_htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) {
+			if self.is_htlc_output_spent_on_chain(htlc) {
+				continue;
+			}
 			assert!(htlc.transaction_output_index.is_some(), "Expected transaction output index for non-dust HTLC");
 
 			let preimage = if htlc.offered {
@@ -5140,13 +5164,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		htlcs
 	}
 
-	// Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can
-	// broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable
-	// script so we can detect whether a holder transaction has been seen on-chain.
+	// Returns (1) `ClaimRequest`s that can be given to the OnchainTxHandler, so that the
+	// handler can broadcast transactions claiming holder HTLC commitment outputs and (2) a
+	// holder revokable script so we can detect whether a holder transaction has been seen
+	// on-chain.
 	#[rustfmt::skip]
 	fn get_broadcasted_holder_claims(
 		&self, funding: &FundingScope, holder_tx: &HolderCommitmentTransaction, conf_height: u32,
-	) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
+	) -> (Vec<ClaimRequest>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
 		let tx = holder_tx.trust();
 		let keys = tx.keys();
 		let redeem_script = chan_utils::get_revokeable_redeemscript(
@@ -5165,7 +5190,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				};
 				let transaction_output_index = htlc_descriptor.htlc.transaction_output_index
 					.expect("Expected transaction output index for non-dust HTLC");
-				PackageTemplate::build_package(
+				ClaimRequest::new(
 					tx.txid(), transaction_output_index,
 					PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(htlc_descriptor, conf_height)),
 					counterparty_spendable_height,
@@ -5201,7 +5226,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	fn check_spend_holder_transaction<L: Logger>(
 		&mut self, commitment_txid: Txid, commitment_tx: &Transaction, height: u32,
 		block_hash: &BlockHash, logger: &L,
-	) -> Option<(Vec<PackageTemplate>, TransactionOutputs)> {
+	) -> Option<(Vec<ClaimRequest>, TransactionOutputs)> {
 		let funding_spent = get_confirmed_funding_scope!(self);
 
 		// HTLCs set may differ between last and previous holder commitment txn, in case of one them hitting chain, ensure we cancel all HTLCs backward
@@ -5712,7 +5737,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		conf_hash: BlockHash,
 		txn_matched: Vec<&Transaction>,
 		mut watch_outputs: Vec<TransactionOutputs>,
-		mut claimable_outpoints: Vec<PackageTemplate>,
+		mut claimable_outpoints: Vec<ClaimRequest>,
 		broadcaster: &B,
 		fee_estimator: &LowerBoundedFeeEstimator<F>,
 		logger: &WithContext<L>,

lightning/src/chain/mod.rs

@@ -563,10 +563,18 @@ pub struct ClaimId(pub [u8; 32]);
 
 impl ClaimId {
 	pub(crate) fn from_htlcs(htlcs: &[HTLCDescriptor]) -> ClaimId {
+		let mut htlc_outpoints = htlcs
+			.iter()
+			.map(|htlc| {
+				(htlc.commitment_txid.to_byte_array(), htlc.htlc.transaction_output_index.unwrap())
+			})
+			.collect::<Vec<_>>();
+		htlc_outpoints.sort_unstable();
+
 		let mut engine = Sha256::engine();
-		for htlc in htlcs {
-			engine.input(&htlc.commitment_txid.to_byte_array());
-			engine.input(&htlc.htlc.transaction_output_index.unwrap().to_be_bytes());
+		for (commitment_txid, transaction_output_index) in htlc_outpoints {
+			engine.input(&commitment_txid);
+			engine.input(&transaction_output_index.to_be_bytes());
 		}
 		ClaimId(Sha256::from_engine(engine).to_byte_array())
 	}
@@ -581,8 +589,45 @@ impl ClaimId {
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use crate::ln::chan_utils::{
+		ChannelTransactionParameters, HTLCOutputInCommitment, HolderCommitmentTransaction,
+	};
+	use crate::sign::ChannelDerivationParameters;
+	use crate::types::payment::{PaymentHash, PaymentPreimage};
 	use bitcoin::hashes::Hash;
 
+	fn dummy_htlc_descriptor(
+		commitment_txid: Txid, transaction_output_index: u32,
+	) -> HTLCDescriptor {
+		let channel_parameters = ChannelTransactionParameters::test_dummy(100_000);
+		let htlc = HTLCOutputInCommitment {
+			offered: true,
+			amount_msat: 1000,
+			cltv_expiry: 100,
+			payment_hash: PaymentHash::from(PaymentPreimage([1; 32])),
+			transaction_output_index: Some(transaction_output_index),
+		};
+		let funding_outpoint = channel_parameters.funding_outpoint.unwrap();
+		let commitment_tx =
+			HolderCommitmentTransaction::dummy(100_000, funding_outpoint, vec![htlc.clone()]);
+		let trusted_tx = commitment_tx.trust();
+
+		HTLCDescriptor {
+			channel_derivation_parameters: ChannelDerivationParameters {
+				value_satoshis: channel_parameters.channel_value_satoshis,
+				keys_id: [1; 32],
+				transaction_parameters: channel_parameters,
+			},
+			commitment_txid,
+			per_commitment_number: trusted_tx.commitment_number(),
+			per_commitment_point: trusted_tx.per_commitment_point(),
+			feerate_per_kw: trusted_tx.negotiated_feerate_per_kw(),
+			htlc,
+			preimage: None,
+			counterparty_sig: commitment_tx.counterparty_htlc_sigs[0],
+		}
+	}
+
 	#[test]
 	fn test_best_block() {
 		let hash1 = BlockHash::from_slice(&[1; 32]).unwrap();
@@ -618,4 +663,17 @@ mod tests {
 		let chain_c = BlockLocator::new(hash_other, 200);
 		assert_eq!(chain_a.find_common_ancestor(&chain_c), None);
 	}
+
+	#[test]
+	fn test_htlc_claim_id_is_descriptor_order_independent() {
+		// Use opposite txid and vout ordering so the assertion would fail if
+		// ClaimId still hashed descriptors in caller-provided order.
+		let first = dummy_htlc_descriptor(Txid::from_slice(&[1; 32]).unwrap(), 2);
+		let second = dummy_htlc_descriptor(Txid::from_slice(&[2; 32]).unwrap(), 1);
+
+		assert_eq!(
+			ClaimId::from_htlcs(&[first.clone(), second.clone()]),
+			ClaimId::from_htlcs(&[second, first])
+		);
+	}
 }