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

@@ -3470,22 +3470,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32,
 		nondust_htlcs: impl Iterator<Item = &'a mut HTLCOutputInCommitment>,
 	) -> CommitmentTransaction {
-		let broadcaster_keys = &self.onchain_tx_handler.channel_transaction_parameters
-			.counterparty_parameters.as_ref().unwrap().pubkeys;
-		let countersignatory_keys =
-			&self.onchain_tx_handler.channel_transaction_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.onchain_tx_handler.channel_transaction_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(commitment_number,
-			to_broadcaster_value, to_countersignatory_value, broadcaster_funding_key,
-			countersignatory_funding_key, keys, feerate_per_kw, nondust_htlcs,
-			channel_parameters)
+			to_broadcaster_value, to_countersignatory_value, keys, feerate_per_kw, nondust_htlcs, channel_parameters)
 	}
 
 	fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec<CommitmentTransaction> {

lightning/src/ln/chan_utils.rs

@@ -1136,7 +1136,7 @@ impl HolderCommitmentTransaction {
 		for _ in 0..nondust_htlcs.len() {
 			counterparty_htlc_sigs.push(dummy_sig);
 		}
-		let inner = CommitmentTransaction::new(0, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable());
+		let inner = CommitmentTransaction::new(0, 0, 0, keys, 0, nondust_htlcs.iter_mut(), &channel_parameters.as_counterparty_broadcastable());
 		nondust_htlcs.sort_by_key(|htlc| htlc.transaction_output_index);
 		HolderCommitmentTransaction {
 			inner,
@@ -1443,12 +1443,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<'a>(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, nondust_htlcs: impl Iterator<Item = &'a mut HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction {
+	pub fn new<'a>(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, keys: TxCreationKeys, feerate_per_kw: u32, nondust_htlcs: impl Iterator<Item = &'a mut HTLCOutputInCommitment>, 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, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap();
+		let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, 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);
@@ -1477,11 +1477,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 nondust_htlcs = self.htlcs.clone();
-		let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, nondust_htlcs.iter_mut(), 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, nondust_htlcs.iter_mut(), channel_parameters)?;
 
 		let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs);
 		let txid = transaction.compute_txid();
@@ -1505,8 +1505,10 @@ 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<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: impl Iterator<Item = &'a mut HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
+	fn internal_build_outputs<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: impl Iterator<Item = &'a mut HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters) -> Result<(Vec<TxOut>, Vec<HTLCOutputInCommitment>), ()> {
 		let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys();
+		let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey;
+		let countersignatory_funding_key = &countersignatory_pubkeys.funding_pubkey;
 		let contest_delay = channel_parameters.contest_delay();
 
 		let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new();
@@ -1680,14 +1682,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 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(());
 		}
@@ -1910,8 +1912,6 @@ mod tests {
 
 	struct TestCommitmentTxBuilder {
 		commitment_number: u64,
-		holder_funding_pubkey: PublicKey,
-		counterparty_funding_pubkey: PublicKey,
 		keys: TxCreationKeys,
 		feerate_per_kw: u32,
 		nondust_htlcs: Vec<HTLCOutputInCommitment>,
@@ -1946,8 +1946,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,
 				nondust_htlcs,
@@ -1959,8 +1957,6 @@ mod tests {
 		fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction {
 			CommitmentTransaction::new(
 				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,
 				self.nondust_htlcs.iter_mut(), &self.channel_parameters.as_holder_broadcastable()
 			)

lightning/src/ln/channel.rs

@@ -3595,11 +3595,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 {
-			(self.get_holder_pubkeys().funding_pubkey, self.get_counterparty_pubkeys().funding_pubkey)
-		} else {
-			(self.get_counterparty_pubkeys().funding_pubkey, self.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);
@@ -3621,8 +3616,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		let tx = CommitmentTransaction::new(commitment_number,
 		                                    value_to_a as u64,
 		                                    value_to_b as u64,
-		                                    funding_pubkey_a,
-		                                    funding_pubkey_b,
 		                                    keys.clone(),
 		                                    feerate_per_kw,
 		                                    included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc),

lightning/src/ln/functional_tests.rs

@@ -731,24 +731,23 @@ 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();
-		(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();
 		(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.
@@ -765,7 +764,6 @@ 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,
 			htlcs.iter_mut(),
@@ -1456,7 +1454,7 @@ 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();
@@ -1467,18 +1465,16 @@ fn test_fee_spike_violation_fails_htlc() {
 		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().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();
 		(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.
@@ -1508,7 +1504,6 @@ fn test_fee_spike_violation_fails_htlc() {
 			commitment_number,
 			95000,
 			local_chan_balance,
-			local_funding, remote_funding,
 			commit_tx_keys.clone(),
 			feerate_per_kw,
 			vec![accepted_htlc_info].iter_mut(),

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")