Remove redundant parameters from the CommitmentTransaction constructor

The `DirectedChannelTransactionParameters` argument of the
`CommitmentTransaction` constructor already contains the broadcaster,
and the countersignatory public keys, which include the respective
funding keys.

It is therefore not necessary to ask for these funding keys in
additional arguments.

Author: tankyleo

lightning/src/chain/channelmonitor.rs

@@ -3506,20 +3506,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32,
 		mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>
 	) -> CommitmentTransaction {
-		let broadcaster_keys =
-			&self.funding.channel_parameters.counterparty_parameters.as_ref().unwrap().pubkeys;
-		let countersignatory_keys = &self.funding.channel_parameters.holder_pubkeys;
-
-		let broadcaster_funding_key = broadcaster_keys.funding_pubkey;
-		let countersignatory_funding_key = countersignatory_keys.funding_pubkey;
-		let keys = TxCreationKeys::from_channel_static_keys(&their_per_commitment_point,
-			&broadcaster_keys, &countersignatory_keys, &self.onchain_tx_handler.secp_ctx);
 		let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable();
+		let keys = TxCreationKeys::from_channel_static_keys(their_per_commitment_point,
+			channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), &self.onchain_tx_handler.secp_ctx);
 
 		CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
-			to_broadcaster_value, to_countersignatory_value, broadcaster_funding_key,
-			countersignatory_funding_key, keys, feerate_per_kw, &mut nondust_htlcs,
-			channel_parameters)
+			to_broadcaster_value, to_countersignatory_value, keys, feerate_per_kw, &mut nondust_htlcs, channel_parameters)
 	}
 
 	fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec<CommitmentTransaction> {

lightning/src/ln/chan_utils.rs

@@ -1208,7 +1208,7 @@ impl HolderCommitmentTransaction {
 		for _ in 0..htlcs.len() {
 			counterparty_htlc_sigs.push(dummy_sig);
 		}
-		let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
+		let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
 		htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index);
 		HolderCommitmentTransaction {
 			inner,
@@ -1518,12 +1518,12 @@ impl CommitmentTransaction {
 	/// Only include HTLCs that are above the dust limit for the channel.
 	///
 	/// This is not exported to bindings users due to the generic though we likely should expose a version without
-	pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, broadcaster_funding_key: PublicKey, countersignatory_funding_key: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction {
+	pub fn new_with_auxiliary_htlc_data<T>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction {
 		let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
 		let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
 
 		// Sort outputs and populate output indices while keeping track of the auxiliary data
-		let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap();
+		let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters).unwrap();
 
 		let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters);
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
@@ -1552,11 +1552,11 @@ impl CommitmentTransaction {
 		self
 	}
 
-	fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<BuiltCommitmentTransaction, ()> {
+	fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> Result<BuiltCommitmentTransaction, ()> {
 		let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters);
 
 		let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect();
-		let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?;
+		let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters)?;
 
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
 		let txid = transaction.compute_txid();
@@ -1580,17 +1580,20 @@ impl CommitmentTransaction {
 	// - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the
 	//   caller needs to have sorted together with the HTLCs so it can keep track of the output index
 	// - building of a bitcoin transaction during a verify() call, in which case T is just ()
-	fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
-		let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys();
+	fn internal_build_outputs<T>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
+		let countersignatory_payment_point = &channel_parameters.countersignatory_pubkeys().payment_point;
+		let countersignatory_funding_key = &channel_parameters.countersignatory_pubkeys().funding_pubkey;
+		let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey;
+		let channel_type = channel_parameters.channel_type_features();
 		let contest_delay = channel_parameters.contest_delay();
 
 		let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new();
 
 		if to_countersignatory_value_sat > Amount::ZERO {
-			let script = if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
-				get_to_countersigner_keyed_anchor_redeemscript(&countersignatory_pubkeys.payment_point).to_p2wsh()
+			let script = if channel_type.supports_anchors_zero_fee_htlc_tx() {
+				get_to_countersigner_keyed_anchor_redeemscript(countersignatory_payment_point).to_p2wsh()
 			} else {
-				ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_pubkeys.payment_point.serialize()).into())
+				ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_payment_point.serialize()).into())
 			};
 			txouts.push((
 				TxOut {
@@ -1616,7 +1619,7 @@ impl CommitmentTransaction {
 			));
 		}
 
-		if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
+		if channel_type.supports_anchors_zero_fee_htlc_tx() {
 			if to_broadcaster_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() {
 				let anchor_script = get_keyed_anchor_redeemscript(broadcaster_funding_key);
 				txouts.push((
@@ -1642,7 +1645,7 @@ impl CommitmentTransaction {
 
 		let mut htlcs = Vec::with_capacity(htlcs_with_aux.len());
 		for (htlc, _) in htlcs_with_aux {
-			let script = get_htlc_redeemscript(&htlc, &channel_parameters.channel_type_features(), &keys);
+			let script = get_htlc_redeemscript(htlc, channel_type, keys);
 			let txout = TxOut {
 				script_pubkey: script.to_p2wsh(),
 				value: htlc.to_bitcoin_amount(),
@@ -1753,14 +1756,14 @@ impl CommitmentTransaction {
 	///
 	/// An external validating signer must call this method before signing
 	/// or using the built transaction.
-	pub fn verify<T: secp256k1::Signing + secp256k1::Verification>(&self, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1<T>) -> Result<TrustedCommitmentTransaction, ()> {
+	pub fn verify<T: secp256k1::Signing + secp256k1::Verification>(&self, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<T>) -> Result<TrustedCommitmentTransaction, ()> {
 		// This is the only field of the key cache that we trust
-		let per_commitment_point = self.keys.per_commitment_point;
-		let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, broadcaster_keys, countersignatory_keys, secp_ctx);
+		let per_commitment_point = &self.keys.per_commitment_point;
+		let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
 		if keys != self.keys {
 			return Err(());
 		}
-		let tx = self.internal_rebuild_transaction(&keys, channel_parameters, &broadcaster_keys.funding_pubkey, &countersignatory_keys.funding_pubkey)?;
+		let tx = self.internal_rebuild_transaction(&keys, channel_parameters)?;
 		if self.built.transaction != tx.transaction || self.built.txid != tx.txid {
 			return Err(());
 		}
@@ -1983,8 +1986,6 @@ mod tests {
 
 	struct TestCommitmentTxBuilder {
 		commitment_number: u64,
-		holder_funding_pubkey: PublicKey,
-		counterparty_funding_pubkey: PublicKey,
 		keys: TxCreationKeys,
 		feerate_per_kw: u32,
 		htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>,
@@ -2023,8 +2024,6 @@ mod tests {
 
 			Self {
 				commitment_number: 0,
-				holder_funding_pubkey: holder_pubkeys.funding_pubkey,
-				counterparty_funding_pubkey: counterparty_pubkeys.funding_pubkey,
 				keys,
 				feerate_per_kw: 1,
 				htlcs_with_aux,
@@ -2036,8 +2035,6 @@ mod tests {
 		fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction {
 			CommitmentTransaction::new_with_auxiliary_htlc_data(
 				self.commitment_number, to_broadcaster_sats, to_countersignatory_sats,
-				self.holder_funding_pubkey.clone(),
-				self.counterparty_funding_pubkey.clone(),
 				self.keys.clone(), self.feerate_per_kw,
 				&mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable()
 			)

lightning/src/ln/channel.rs

@@ -3795,11 +3795,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 
 		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 };
-		let (funding_pubkey_a, funding_pubkey_b) = if local {
-			(funding.get_holder_pubkeys().funding_pubkey, funding.get_counterparty_pubkeys().funding_pubkey)
-		} else {
-			(funding.get_counterparty_pubkeys().funding_pubkey, funding.get_holder_pubkeys().funding_pubkey)
-		};
 
 		if value_to_a >= (broadcaster_dust_limit_satoshis as i64) {
 			log_trace!(logger, "   ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
@@ -3821,8 +3816,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
 		                                                             value_to_a as u64,
 		                                                             value_to_b as u64,
-		                                                             funding_pubkey_a,
-		                                                             funding_pubkey_b,
 		                                                             keys.clone(),
 		                                                             feerate_per_kw,
 		                                                             &mut included_non_dust_htlcs,

lightning/src/ln/functional_tests.rs

@@ -734,24 +734,23 @@ pub 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 (local_revocation_basepoint, local_htlc_basepoint) = {
 		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(None, &secp_ctx);
-		(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
-		 pubkeys.funding_pubkey)
+		(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint)
 	};
-	let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
+	let (remote_delayed_payment_basepoint, remote_htlc_basepoint, 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(None, &secp_ctx);
 		(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)
+		 )
 	};
 
 	// Assemble the set of keys we can use for signatures for our commitment_signed message.
@@ -768,7 +767,6 @@ pub fn test_update_fee_that_funder_cannot_afford() {
 			INITIAL_COMMITMENT_NUMBER - 1,
 			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,
@@ -1462,7 +1460,7 @@ pub 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_revocation_basepoint, local_htlc_basepoint, 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();
@@ -1473,18 +1471,16 @@ pub fn test_fee_spike_violation_fails_htlc() {
 		let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx);
 		(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(None, &secp_ctx).funding_pubkey)
+		 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_delayed_payment_basepoint, remote_htlc_basepoint, 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(None, &secp_ctx);
 		(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(None, &secp_ctx).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.
@@ -1514,7 +1510,6 @@ pub fn test_fee_spike_violation_fails_htlc() {
 			commitment_number,
 			95000,
 			local_chan_balance,
-			local_funding, remote_funding,
 			commit_tx_keys.clone(),
 			feerate_per_kw,
 			&mut vec![(accepted_htlc_info, ())],

lightning/src/util/test_channel_signer.rs

@@ -549,12 +549,7 @@ impl TestChannelSigner {
 		commitment_tx: &'a CommitmentTransaction, secp_ctx: &Secp256k1<T>,
 	) -> TrustedCommitmentTransaction<'a> {
 		commitment_tx
-			.verify(
-				&channel_parameters.as_counterparty_broadcastable(),
-				channel_parameters.counterparty_pubkeys().unwrap(),
-				&channel_parameters.holder_pubkeys,
-				secp_ctx,
-			)
+			.verify(&channel_parameters.as_counterparty_broadcastable(), secp_ctx)
 			.expect("derived different per-tx keys or built transaction")
 	}
 
@@ -563,12 +558,7 @@ impl TestChannelSigner {
 		commitment_tx: &'a CommitmentTransaction, secp_ctx: &Secp256k1<T>,
 	) -> TrustedCommitmentTransaction<'a> {
 		commitment_tx
-			.verify(
-				&channel_parameters.as_holder_broadcastable(),
-				&channel_parameters.holder_pubkeys,
-				channel_parameters.counterparty_pubkeys().unwrap(),
-				secp_ctx,
-			)
+			.verify(&channel_parameters.as_holder_broadcastable(), secp_ctx)
 			.expect("derived different per-tx keys or built transaction")
 	}
 }