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,43 @@ 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 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));
-			}
-		}
-		// 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() {
-			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 get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| {
+			let trusted_tx = holder_commitment.trust();
+			if trusted_tx.txid() != outp.txid {
+				return None;
 			}
-		}
-		htlc_tx
+			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.build_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,
+			);
+			Some(htlc_tx)
+		};
+
+		// Check if the HTLC spends from the current holder commitment first, or the previous.
+		get_signed_htlc_tx(&self.holder_commitment)
+			.or_else(|| self.prev_holder_commitment.as_ref().and_then(|prev_holder_commitment| get_signed_htlc_tx(prev_holder_commitment)))
 	}
 
 	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
+	}
+
 	/// The value to be sent to the broadcaster
 	pub fn to_broadcaster_value_sat(&self) -> u64 {
 		self.to_broadcaster_value_sat
@@ -1720,26 +1725,40 @@ impl<'a> TrustedCommitmentTransaction<'a> {
 		Ok(ret)
 	}
 
-	/// 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 {
-		let inner = self.inner;
-		let keys = &inner.keys;
-		let txid = inner.built.txid;
-		let this_htlc = &inner.htlcs[htlc_index];
+	/// Builds the second-level holder HTLC transaction for the HTLC with index `htlc_index`.
+	pub(crate) fn build_unsigned_htlc_tx(
+		&self, channel_parameters: &DirectedChannelTransactionParameters, htlc_index: usize,
+		preimage: &Option<PaymentPreimage>,
+	) -> Transaction {
+		let keys = &self.inner.keys;
+		let this_htlc = &self.inner.htlcs[htlc_index];
 		assert!(this_htlc.transaction_output_index.is_some());
 		// if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction.
 		if !this_htlc.offered && preimage.is_none() { unreachable!(); }
 		// 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(
+			&self.inner.built.txid, self.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,
+	/// Builds the witness required to spend the input for the HTLC with index `htlc_index` in a
+	/// second-level holder HTLC transaction.
+	pub(crate) fn build_htlc_input_witness(
+		&self, htlc_index: usize, counterparty_signature: &Signature, signature: &Signature,
+		preimage: &Option<PaymentPreimage>
+	) -> Witness {
+		let keys = &self.inner.keys;
+		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(
+			&self.inner.htlcs[htlc_index], &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,113 @@ 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, confirm_counterparty_commitment: 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 (_, _, chan_id, 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);
+
+	let (closing_node, other_node) = if confirm_counterparty_commitment {
+		(&nodes[1], &nodes[0])
+	} else {
+		(&nodes[0], &nodes[1])
+	};
+
+	closing_node.node.force_close_broadcasting_latest_txn(&chan_id, &other_node.node.get_our_node_id()).unwrap();
+
+	// The commitment transaction comes first.
+	let commitment_tx = {
+		let mut txn = closing_node.tx_broadcaster.unique_txn_broadcast();
+		assert_eq!(txn.len(), 1);
+		check_spends!(txn[0], funding_tx);
+		txn.pop().unwrap()
+	};
+
+	mine_transaction(closing_node, &commitment_tx);
+	check_added_monitors!(closing_node, 1);
+	check_closed_broadcast!(closing_node, true);
+	check_closed_event!(closing_node, 1, ClosureReason::HolderForceClosed, [other_node.node.get_our_node_id()], 1_000_000);
+
+	mine_transaction(other_node, &commitment_tx);
+	check_added_monitors!(other_node, 1);
+	check_closed_broadcast!(other_node, true);
+	check_closed_event!(other_node, 1, ClosureReason::CommitmentTxConfirmed, [closing_node.node.get_our_node_id()], 1_000_000);
+
+	// If we update the best block to the new height before providing the confirmed transactions,
+	// we'll see another broadcast of the commitment transaction.
+	if anchors && !confirm_counterparty_commitment && nodes[0].connect_style.borrow().updates_best_block_first() {
+		let _ = nodes[0].tx_broadcaster.txn_broadcast();
+	}
+
+	// Then comes the HTLC timeout transaction.
+	if confirm_counterparty_commitment {
+		connect_blocks(&nodes[0], 5);
+		test_spendable_output(&nodes[0], &commitment_tx, false);
+		connect_blocks(&nodes[0], TEST_FINAL_CLTV - 5);
+	} else {
+		connect_blocks(&nodes[0], TEST_FINAL_CLTV);
+	}
+	if anchors && !confirm_counterparty_commitment {
+		handle_bump_htlc_event(&nodes[0], 1);
+	}
+	let htlc_timeout_tx = {
+		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+		assert_eq!(txn.len(), 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 && !confirm_counterparty_commitment {
+		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, false);
+	do_test_monitor_claims_with_random_signatures(false, true);
+	do_test_monitor_claims_with_random_signatures(true, false);
+	do_test_monitor_claims_with_random_signatures(true, true);
+}