Use sign_holder_htlc_transaction to sign non-anchors holder HTLCs

We want to ensure we use fresh random signatures to prevent certain
classes of transaction replacement attacks at the bitcoin P2P layer.
This was already covered for commitment transactions and zero fee holder
HTLC transactions, but was missing for holder HTLC transactions on
non-anchors channels.

We can easily do this by reusing the existing
`EcdsaChannelSigner::sign_holder_htlc_transaction` method and
circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs`
caches, which will be removed in a later commit anyway.

Author: wpaulino

lightning/src/chain/onchaintx.rs

@@ -23,6 +23,7 @@ use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
 use bitcoin::secp256k1;
 
 use crate::chain::chaininterface::compute_feerate_sat_per_1000_weight;
+use crate::events::bump_transaction::{ChannelDerivationParameters, HTLCDescriptor};
 use crate::sign::{ChannelSigner, EntropySource, SignerProvider};
 use crate::ln::msgs::DecodeError;
 use crate::ln::PaymentPreimage;
@@ -1157,35 +1158,51 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 	}
 
 	pub(crate) fn get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option<PaymentPreimage>) -> Option<Transaction> {
-		let mut htlc_tx = None;
+		let get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| {
+			let trusted_tx = holder_commitment.trust();
+			let (htlc_idx, htlc) = trusted_tx.htlcs().iter().enumerate()
+				.find(|(_, htlc)| htlc.transaction_output_index.unwrap() == outp.vout)
+				.unwrap();
+			let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx];
+			let mut htlc_tx = trusted_tx.get_unsigned_htlc_tx(
+				&self.channel_transaction_parameters.as_holder_broadcastable(), htlc_idx, preimage,
+			);
+
+			let htlc_descriptor = HTLCDescriptor {
+				channel_derivation_parameters: ChannelDerivationParameters {
+					value_satoshis: self.channel_value_satoshis,
+					keys_id: self.channel_keys_id,
+					transaction_parameters: self.channel_transaction_parameters.clone(),
+				},
+				commitment_txid: trusted_tx.txid(),
+				per_commitment_number: trusted_tx.commitment_number(),
+				per_commitment_point: trusted_tx.per_commitment_point(),
+				feerate_per_kw: trusted_tx.feerate_per_kw(),
+				htlc: htlc.clone(),
+				preimage: preimage.clone(),
+				counterparty_sig: counterparty_htlc_sig.clone(),
+			};
+			let htlc_sig = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx).unwrap();
+			htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness(
+				htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage,
+			);
+			htlc_tx
+		};
+
 		let commitment_txid = self.holder_commitment.trust().txid();
 		// Check if the HTLC spends from the current holder commitment
 		if commitment_txid == outp.txid {
-			self.sign_latest_holder_htlcs();
-			if let &Some(ref htlc_sigs) = &self.holder_htlc_sigs {
-				let &(ref htlc_idx, ref htlc_sig) = htlc_sigs[outp.vout as usize].as_ref().unwrap();
-				let trusted_tx = self.holder_commitment.trust();
-				let counterparty_htlc_sig = self.holder_commitment.counterparty_htlc_sigs[*htlc_idx];
-				htlc_tx = Some(trusted_tx
-					.get_signed_htlc_tx(&self.channel_transaction_parameters.as_holder_broadcastable(), *htlc_idx, &counterparty_htlc_sig, htlc_sig, preimage));
-			}
+			return Some(get_signed_htlc_tx(&self.holder_commitment));
 		}
 		// If the HTLC doesn't spend the current holder commitment, check if it spends the previous one
-		if htlc_tx.is_none() && self.prev_holder_commitment.is_some() {
+		if self.prev_holder_commitment.is_some() {
 			let commitment_txid = self.prev_holder_commitment.as_ref().unwrap().trust().txid();
 			if commitment_txid == outp.txid {
-				self.sign_prev_holder_htlcs();
-				if let &Some(ref htlc_sigs) = &self.prev_holder_htlc_sigs {
-					let &(ref htlc_idx, ref htlc_sig) = htlc_sigs[outp.vout as usize].as_ref().unwrap();
-					let holder_commitment = self.prev_holder_commitment.as_ref().unwrap();
-					let trusted_tx = holder_commitment.trust();
-					let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[*htlc_idx];
-					htlc_tx = Some(trusted_tx
-						.get_signed_htlc_tx(&self.channel_transaction_parameters.as_holder_broadcastable(), *htlc_idx, &counterparty_htlc_sig, htlc_sig, preimage));
-				}
+				let holder_commitment = self.prev_holder_commitment.as_ref().unwrap();
+				return Some(get_signed_htlc_tx(holder_commitment));
 			}
 		}
-		htlc_tx
+		None
 	}
 
 	pub(crate) fn generate_external_htlc_claim(

lightning/src/ln/chan_utils.rs

@@ -1599,6 +1599,11 @@ impl CommitmentTransaction {
 		self.commitment_number
 	}
 
+	/// The per commitment point used by the broadcaster.
+	pub fn per_commitment_point(&self) -> PublicKey {
+		self.keys.per_commitment_point.clone()
+	}
+
 	/// The value to be sent to the broadcaster
 	pub fn to_broadcaster_value_sat(&self) -> u64 {
 		self.to_broadcaster_value_sat
@@ -1721,7 +1726,10 @@ impl<'a> TrustedCommitmentTransaction<'a> {
 	}
 
 	/// Gets a signed HTLC transaction given a preimage (for !htlc.offered) and the holder HTLC transaction signature.
-	pub(crate) fn get_signed_htlc_tx(&self, channel_parameters: &DirectedChannelTransactionParameters, htlc_index: usize, counterparty_signature: &Signature, signature: &Signature, preimage: &Option<PaymentPreimage>) -> Transaction {
+	pub(crate) fn get_unsigned_htlc_tx(
+		&self, channel_parameters: &DirectedChannelTransactionParameters, htlc_index: usize,
+		preimage: &Option<PaymentPreimage>,
+	) -> Transaction {
 		let inner = self.inner;
 		let keys = &inner.keys;
 		let txid = inner.built.txid;
@@ -1732,14 +1740,26 @@ impl<'a> TrustedCommitmentTransaction<'a> {
 		// Further, we should never be provided the preimage for an HTLC-Timeout transaction.
 		if  this_htlc.offered && preimage.is_some() { unreachable!(); }
 
-		let mut htlc_tx = build_htlc_transaction(&txid, inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc, &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
+		build_htlc_transaction(
+			&txid, inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc,
+			&self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key
+		)
+	}
 
-		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc, &self.channel_type_features, &keys.broadcaster_htlc_key, &keys.countersignatory_htlc_key, &keys.revocation_key);
 
-		htlc_tx.input[0].witness = chan_utils::build_htlc_input_witness(
-			signature, counterparty_signature, preimage, &htlc_redeemscript, &self.channel_type_features,
+	/// Gets a signed HTLC transaction given a preimage (for !htlc.offered) and the holder HTLC transaction signature.
+	pub(crate) fn build_htlc_input_witness(&self, htlc_index: usize, counterparty_signature: &Signature, signature: &Signature, preimage: &Option<PaymentPreimage>) -> Witness {
+		let inner = self.inner;
+		let keys = &inner.keys;
+		let this_htlc = &inner.htlcs[htlc_index];
+
+		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(
+			&this_htlc, &self.channel_type_features, &keys.broadcaster_htlc_key,
+			&keys.countersignatory_htlc_key, &keys.revocation_key
 		);
-		htlc_tx
+		chan_utils::build_htlc_input_witness(
+			signature, counterparty_signature, preimage, &htlc_redeemscript, &self.channel_type_features,
+		)
 	}
 
 	/// Returns the index of the revokeable output, i.e. the `to_local` output sending funds to

lightning/src/ln/monitor_tests.rs

@@ -2715,3 +2715,102 @@ fn test_anchors_monitor_fixes_counterparty_payment_script_on_reload() {
 	do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(false);
 	do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(true);
 }
+
+#[cfg(not(feature = "_test_vectors"))]
+fn do_test_monitor_claims_with_random_signatures(anchors: bool) {
+	// Tests that our monitor claims will always use fresh random signatures (ensuring a unique
+	// wtxid) to prevent certain classes of transaction replacement at the bitcoin P2P layer.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let mut user_config = test_default_channel_config();
+	if anchors {
+		user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+		user_config.manually_accept_inbound_channels = true;
+	}
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config), Some(user_config)]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let coinbase_tx = Transaction {
+		version: 2,
+		lock_time: PackedLockTime::ZERO,
+		input: vec![TxIn { ..Default::default() }],
+		output: vec![
+			TxOut {
+				value: Amount::ONE_BTC.to_sat(),
+				script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
+			},
+		],
+	};
+	if anchors {
+		nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value);
+	}
+
+	// Open a channel and route a payment. We'll let it timeout to claim it.
+	let (_, _, _, funding_tx) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
+	route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+	connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1);
+
+	// The commitment transaction comes first.
+	let commitment_tx = {
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		assert_eq!(txn.len(), if anchors { 1 } else { 2 });
+		check_spends!(txn[0], funding_tx);
+		txn.remove(0)
+	};
+
+	// Check we rebroadcast it with a different wtxid.
+	nodes[0].chain_monitor.chain_monitor.rebroadcast_pending_claims();
+	{
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		assert_eq!(txn.len(), if anchors { 1 } else { 2 });
+		let new_commitment_tx = if txn[0].input[0].previous_output.txid == funding_tx.txid() {
+			&txn[0]
+		} else {
+			&txn[1]
+		};
+		assert_eq!(new_commitment_tx.txid(), commitment_tx.txid());
+		assert_ne!(new_commitment_tx.wtxid(), commitment_tx.wtxid());
+	};
+
+	mine_transaction(&nodes[0], &commitment_tx);
+	check_added_monitors!(nodes[0], 1);
+	check_closed_broadcast!(nodes[0], true);
+	check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed, [nodes[1].node.get_our_node_id()], 1_000_000);
+
+	// Then comes the HTLC timeout transaction.
+	if anchors {
+		handle_bump_htlc_event(&nodes[0], 1);
+	}
+	let htlc_timeout_tx = {
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		// If we update the best block to the new height before providing the confirmed
+		// transactions, we'll see another broadcast of the commitment transaction.
+		assert_eq!(txn.len(), if nodes[0].connect_style.borrow().updates_best_block_first() { 2 } else { 1 });
+		let tx = if txn[0].input[0].previous_output.txid == commitment_tx.txid() {
+			txn[0].clone()
+		} else {
+			txn[1].clone()
+		};
+		check_spends!(tx, commitment_tx, coinbase_tx);
+		tx
+	};
+
+	// Check we rebroadcast it with a different wtxid.
+	nodes[0].chain_monitor.chain_monitor.rebroadcast_pending_claims();
+	if anchors {
+		handle_bump_htlc_event(&nodes[0], 1);
+	}
+	{
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		assert_eq!(txn.len(), 1);
+		assert_eq!(txn[0].txid(), htlc_timeout_tx.txid());
+		assert_ne!(txn[0].wtxid(), htlc_timeout_tx.wtxid());
+	}
+}
+
+#[cfg(not(feature = "_test_vectors"))]
+#[test]
+fn test_monitor_claims_with_random_signatures() {
+	do_test_monitor_claims_with_random_signatures(false);
+	do_test_monitor_claims_with_random_signatures(true);
+}