[Custom Transactions] Define the TxBuilder trait

lightning/src/chain/onchaintx.rs

@@ -1363,16 +1363,15 @@ mod tests {
 		for i in 0..3 {
 			let preimage = PaymentPreimage([i; 32]);
 			let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array());
-			htlcs.push((
+			htlcs.push(
 				HTLCOutputInCommitment {
 					offered: true,
 					amount_msat: 10000,
 					cltv_expiry: i as u32,
 					payment_hash: hash,
 					transaction_output_index: Some(i as u32),
 				},
-				(),
-			));
+			);
 		}
 		let holder_commit = HolderCommitmentTransaction::dummy(&mut htlcs);
 		let mut tx_handler = OnchainTxHandler::new(
@@ -1400,7 +1399,7 @@ mod tests {
 		// Request claiming of each HTLC on the holder's commitment, with current block height 1.
 		let holder_commit_txid = tx_handler.get_unsigned_holder_commitment_tx().compute_txid();
 		let mut requests = Vec::new();
-		for (htlc, _) in htlcs {
+		for htlc in htlcs {
 			requests.push(PackageTemplate::build_package(
 				holder_commit_txid,
 				htlc.transaction_output_index.unwrap(),

lightning/src/ln/functional_tests.rs

@@ -731,45 +731,29 @@ fn test_update_fee_that_funder_cannot_afford() {
 
 	// Get the TestChannelSigner for each channel, which will be used to (1) get the keys
 	// needed to sign the new commitment tx and (2) sign the new commitment tx.
-	let (local_revocation_basepoint, local_htlc_basepoint, local_funding) = {
-		let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
-		let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
-		let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
-		let chan_signer = local_chan.get_signer();
-		let pubkeys = chan_signer.as_ref().pubkeys();
-		(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
-		 pubkeys.funding_pubkey)
-	};
-	let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
+	let remote_point = {
 		let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
 		let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
 		let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
 		let chan_signer = remote_chan.get_signer();
-		let pubkeys = chan_signer.as_ref().pubkeys();
-		(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
-		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
-		 pubkeys.funding_pubkey)
+		chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()
 	};
 
-	// Assemble the set of keys we can use for signatures for our commitment_signed message.
-	let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint,
-		&remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint);
-
 	let res = {
 		let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
 		let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
 		let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
 		let local_chan_signer = local_chan.get_signer();
-		let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
-		let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
+		let mut htlcs: Vec<HTLCOutputInCommitment> = vec![];
+		let commitment_tx = CommitmentTransaction::new(
 			INITIAL_COMMITMENT_NUMBER - 1,
+			&remote_point,
 			push_sats,
 			channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000,
-			local_funding, remote_funding,
-			commit_tx_keys.clone(),
 			non_buffer_feerate + 4,
-			&mut htlcs,
-			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable()
+			htlcs.iter_mut(),
+			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable(),
+			&secp_ctx,
 		);
 		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap()
 	};
@@ -1456,35 +1440,25 @@ fn test_fee_spike_violation_fails_htlc() {
 
 	// Get the TestChannelSigner for each channel, which will be used to (1) get the keys
 	// needed to sign the new commitment tx and (2) sign the new commitment tx.
-	let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point, local_funding) = {
+	let (local_secret, next_local_point) = {
 		let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
 		let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
 		let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
 		let chan_signer = local_chan.get_signer();
 		// Make the signer believe we validated another commitment, so we can release the secret
 		chan_signer.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1;
 
-		let pubkeys = chan_signer.as_ref().pubkeys();
-		(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
-		 chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
-		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(),
-		 chan_signer.as_ref().pubkeys().funding_pubkey)
+		 (chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
+		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap())
 	};
-	let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
+	let remote_point = {
 		let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
 		let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
 		let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
 		let chan_signer = remote_chan.get_signer();
-		let pubkeys = chan_signer.as_ref().pubkeys();
-		(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
-		 chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(),
-		 chan_signer.as_ref().pubkeys().funding_pubkey)
+		chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap()
 	};
 
-	// Assemble the set of keys we can use for signatures for our commitment_signed message.
-	let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint,
-		&remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint);
-
 	// Build the remote commitment transaction so we can sign it, and then later use the
 	// signature for the commitment_signed message.
 	let local_chan_balance = 1313;
@@ -1504,15 +1478,15 @@ fn test_fee_spike_violation_fails_htlc() {
 		let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
 		let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
 		let local_chan_signer = local_chan.get_signer();
-		let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
+		let commitment_tx = CommitmentTransaction::new(
 			commitment_number,
+			&remote_point,
 			95000,
 			local_chan_balance,
-			local_funding, remote_funding,
-			commit_tx_keys.clone(),
 			feerate_per_kw,
-			&mut vec![(accepted_htlc_info, ())],
-			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable()
+			vec![accepted_htlc_info].iter_mut(),
+			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable(),
+			&secp_ctx,
 		);
 		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), Vec::new(), &secp_ctx).unwrap()
 	};

lightning/src/sign/mod.rs

@@ -81,6 +81,7 @@ pub(crate) mod type_resolver;
 pub mod ecdsa;
 #[cfg(taproot)]
 pub mod taproot;
+pub(crate) mod tx_builder;
 
 /// Information about a spendable output to a P2WSH script.
 ///

lightning/src/sign/tx_builder.rs

@@ -0,0 +1,195 @@
+//! Defines the `TxBuilder` trait, and the `SpecTxBuilder` type
+#![allow(dead_code)]
+#![allow(unused_variables)]
+
+use core::ops::Deref;
+
+use bitcoin::secp256k1::{self, PublicKey, Secp256k1};
+
+use crate::ln::chan_utils::{
+	self, ChannelTransactionParameters, CommitmentTransaction, HTLCOutputInCommitment,
+};
+use crate::ln::channel::{self, CommitmentStats};
+use crate::prelude::*;
+use crate::util::logger::Logger;
+
+/// A trait for types that can build commitment transactions, both for the holder, and the counterparty.
+pub(crate) trait TxBuilder {
+	/// Build a commitment transaction, and populate the elements of `htlcs` with their output indices.
+	fn build_commitment_transaction<'a, L: Deref>(
+		&self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey,
+		channel_transaction_parameters: &ChannelTransactionParameters,
+		secp_ctx: &Secp256k1<secp256k1::All>, channel_value_satoshis: u64,
+		value_to_self_with_offset_msat: u64,
+		htlcs_in_tx: impl Iterator<Item = &'a mut HTLCOutputInCommitment>, feerate_per_kw: u32,
+		broadcaster_dust_limit_satoshis: u64, logger: &L,
+	) -> CommitmentStats
+	where
+		L::Target: Logger;
+}
+
+/// A type that builds commitment transactions according to the Lightning Specification.
+#[derive(Clone, Debug, Default)]
+pub(crate) struct SpecTxBuilder {}
+
+impl TxBuilder for SpecTxBuilder {
+	fn build_commitment_transaction<'a, L: Deref>(
+		&self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey,
+		channel_transaction_parameters: &ChannelTransactionParameters,
+		secp_ctx: &Secp256k1<secp256k1::All>, channel_value_satoshis: u64,
+		value_to_self_with_offset_msat: u64,
+		htlcs_in_tx: impl Iterator<Item = &'a mut HTLCOutputInCommitment>, feerate_per_kw: u32,
+		broadcaster_dust_limit_satoshis: u64, logger: &L,
+	) -> CommitmentStats
+	where
+		L::Target: Logger,
+	{
+		// We allocate this vector because we need to count the number of non-dust htlcs and calculate the total fee of the transaction
+		// before calling `CommitmentTransaction::new`.
+		// We could drop this vector and create two iterators: one to count the number of non-dust htlcs, and another to pass to `CommitmentTransaction::new`
+		let mut included_non_dust_htlcs: Vec<&mut HTLCOutputInCommitment> =
+			Vec::with_capacity(htlcs_in_tx.size_hint().0);
+
+		let mut remote_htlc_total_msat = 0;
+		let mut local_htlc_total_msat = 0;
+
+		let channel_parameters = if local {
+			channel_transaction_parameters.as_holder_broadcastable()
+		} else {
+			channel_transaction_parameters.as_counterparty_broadcastable()
+		};
+		let channel_type = channel_parameters.channel_type_features();
+
+		// Trim dust htlcs
+		for htlc in htlcs_in_tx {
+			let outbound = local == htlc.offered;
+			if outbound {
+				local_htlc_total_msat += htlc.amount_msat;
+			} else {
+				remote_htlc_total_msat += htlc.amount_msat;
+			}
+			let htlc_tx_fee = if channel_type.supports_anchors_zero_fee_htlc_tx() {
+				0
+			} else {
+				feerate_per_kw as u64
+					* if htlc.offered {
+						chan_utils::htlc_timeout_tx_weight(&channel_type)
+					} else {
+						chan_utils::htlc_success_tx_weight(&channel_type)
+					} / 1000
+			};
+			if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
+				log_trace!(
+					logger,
+					"   ...creating output for {} non-dust HTLC (hash {}) with value {}",
+					if outbound { "outbound" } else { "inbound" },
+					htlc.payment_hash,
+					htlc.amount_msat
+				);
+				included_non_dust_htlcs.push(htlc);
+			} else {
+				log_trace!(
+					logger,
+					"   ...trimming {} HTLC (hash {}) with value {} due to dust limit",
+					if outbound { "outbound" } else { "inbound" },
+					htlc.payment_hash,
+					htlc.amount_msat
+				);
+			}
+		}
+
+		// Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
+		// AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to
+		// "violate" their reserve value by couting those against it. Thus, we have to do checked subtraction
+		// as otherwise we can overflow.
+		let mut value_to_remote_msat =
+			u64::checked_sub(channel_value_satoshis * 1000, value_to_self_with_offset_msat)
+				.unwrap();
+		let value_to_self_msat =
+			u64::checked_sub(value_to_self_with_offset_msat, local_htlc_total_msat).unwrap();
+		value_to_remote_msat =
+			u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap();
+
+		let total_fee_sat = chan_utils::commit_tx_fee_sat(
+			feerate_per_kw,
+			included_non_dust_htlcs.len(),
+			&channel_type,
+		);
+		let anchors_val = if channel_type.supports_anchors_zero_fee_htlc_tx() {
+			channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2
+		} else {
+			0
+		};
+		let (value_to_self, value_to_remote) =
+			if channel_transaction_parameters.is_outbound_from_holder {
+				(
+					(value_to_self_msat / 1000)
+						.saturating_sub(anchors_val)
+						.saturating_sub(total_fee_sat),
+					value_to_remote_msat / 1000,
+				)
+			} else {
+				(
+					value_to_self_msat / 1000,
+					(value_to_remote_msat / 1000)
+						.saturating_sub(anchors_val)
+						.saturating_sub(total_fee_sat),
+				)
+			};
+
+		let mut value_to_a = if local { value_to_self } else { value_to_remote };
+		let mut value_to_b = if local { value_to_remote } else { value_to_self };
+
+		if value_to_a >= broadcaster_dust_limit_satoshis {
+			log_trace!(
+				logger,
+				"   ...creating {} output with value {}",
+				if local { "to_local" } else { "to_remote" },
+				value_to_a
+			);
+		} else {
+			log_trace!(
+				logger,
+				"   ...trimming {} output with value {} due to dust limit",
+				if local { "to_local" } else { "to_remote" },
+				value_to_a
+			);
+			value_to_a = 0;
+		}
+
+		if value_to_b >= broadcaster_dust_limit_satoshis {
+			log_trace!(
+				logger,
+				"   ...creating {} output with value {}",
+				if local { "to_remote" } else { "to_local" },
+				value_to_b
+			);
+		} else {
+			log_trace!(
+				logger,
+				"   ...trimming {} output with value {} due to dust limit",
+				if local { "to_remote" } else { "to_local" },
+				value_to_b
+			);
+			value_to_b = 0;
+		}
+
+		let tx = CommitmentTransaction::new(
+			commitment_number,
+			&per_commitment_point,
+			value_to_a,
+			value_to_b,
+			feerate_per_kw,
+			included_non_dust_htlcs.into_iter(),
+			&channel_parameters,
+			&secp_ctx,
+		);
+
+		CommitmentStats {
+			tx,
+			total_fee_sat,
+			local_balance_msat: value_to_self_msat,
+			remote_balance_msat: value_to_remote_msat,
+		}
+	}
+}

lightning/src/util/test_channel_signer.rs

@@ -548,8 +548,6 @@ impl TestChannelSigner {
 		commitment_tx
 			.verify(
 				&self.inner.get_channel_parameters().unwrap().as_counterparty_broadcastable(),
-				self.inner.counterparty_pubkeys().unwrap(),
-				self.inner.pubkeys(),
 				secp_ctx,
 			)
 			.expect("derived different per-tx keys or built transaction")
@@ -561,8 +559,6 @@ impl TestChannelSigner {
 		commitment_tx
 			.verify(
 				&self.inner.get_channel_parameters().unwrap().as_holder_broadcastable(),
-				self.inner.pubkeys(),
-				self.inner.counterparty_pubkeys().unwrap(),
 				secp_ctx,
 			)
 			.expect("derived different per-tx keys or built transaction")