Onchain claim replay fixes

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(),
@@ -4946,7 +4946,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 								confirmation_height,
 							),
 						);
-						Some(PackageTemplate::build_package(
+						Some(ClaimRequest::new(
 							commitment_txid,
 							transaction_output_index,
 							htlc_data,
@@ -4962,13 +4962,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			.collect()
 	}
 
-	/// Returns the HTLC claim package templates and the counterparty output info
+	/// 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;
 
@@ -5039,7 +5039,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 +5057,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 +5088,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,
 				);
@@ -5140,13 +5140,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 +5166,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 +5202,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 +5713,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,15 @@ 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() {
+		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])
+		);
+	}
 }