Drop redundant parameters in sign_local_commitment_tx

TheBlueMatt · TheBlueMatt · commit f4fef00ef642 · 2020-04-19T14:55:15.000-04:00
The ChanKeys is created with knowledge of the Channel's value and
funding redeemscript up-front, so we should not be providing it
when making signing requests.
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
@@ -222,15 +222,15 @@ pub trait ChannelKeys : Send+Clone {
 	/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
 	/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
 	/// making the callee generate it via some util function we expose)!
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Create a signature for a local commitment transaction without enforcing one-time signing.
 	///
 	/// Testing revocation logic by our test framework needs to sign multiple local commitment
 	/// transactions. This unsafe test-only version doesn't enforce one-time signing security
 	/// requirement.
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
 
 	/// Signs a transaction created by build_htlc_transaction. If the transaction is an
 	/// HTLC-Success transaction, preimage must be set!
@@ -363,13 +363,21 @@ impl ChannelKeys for InMemoryChannelKeys {
 		Ok((commitment_sig, htlc_sigs))
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
-		local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+		let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+		let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+		local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
-		local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
+		let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
+		let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
+
+		local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
@@ -4418,7 +4418,7 @@ mod tests {
 		let logger : Arc<Logger> = Arc::new(test_utils::TestLogger::new());
 		let secp_ctx = Secp256k1::new();
 
-		let chan_keys = InMemoryChannelKeys::new(
+		let mut chan_keys = InMemoryChannelKeys::new(
 			&secp_ctx,
 			SecretKey::from_slice(&hex::decode("30ff4956bbdd3222d44cc5e8a1261dab1e07957bdac5ae88fe3261ef321f3749").unwrap()[..]).unwrap(),
 			SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
@@ -4428,7 +4428,7 @@ mod tests {
 
 			// These aren't set in the test vectors:
 			[0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff],
-			7000000000,
+			10_000_000,
 		);
 
 		assert_eq!(PublicKey::from_secret_key(&secp_ctx, chan_keys.funding_key()).serialize()[..],
@@ -4438,7 +4438,7 @@ mod tests {
 		let their_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let mut config = UserConfig::default();
 		config.channel_options.announced_channel = false;
-		let mut chan = Channel::<InMemoryChannelKeys>::new_outbound(&&feeest, &&keys_provider, their_node_id, 10000000, 100000, 42, Arc::clone(&logger), &config).unwrap(); // Nothing uses their network key in this test
+		let mut chan = Channel::<InMemoryChannelKeys>::new_outbound(&&feeest, &&keys_provider, their_node_id, 10_000_000, 100000, 42, Arc::clone(&logger), &config).unwrap(); // Nothing uses their network key in this test
 		chan.their_to_self_delay = 144;
 		chan.our_dust_limit_satoshis = 546;
 
@@ -4452,6 +4452,7 @@ mod tests {
 			delayed_payment_basepoint: public_from_secret_hex(&secp_ctx, "1552dfba4f6cf29a62a0af13c8d6981d36d0ef8d61ba10fb0fe90da7634d7e13"),
 			htlc_basepoint: public_from_secret_hex(&secp_ctx, "4444444444444444444444444444444444444444444444444444444444444444")
 		};
+		chan_keys.set_remote_channel_pubkeys(&their_pubkeys);
 
 		assert_eq!(their_pubkeys.payment_basepoint.serialize()[..],
 		           hex::decode("032c0b7cf95324a07d05398b240174dc0c2be444d96b159aa6c7f7b1e668680991").unwrap()[..]);
@@ -4491,7 +4492,7 @@ mod tests {
 				secp_ctx.verify(&sighash, &their_signature, chan.their_funding_pubkey()).unwrap();
 
 				localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey());
-				chan_keys.sign_local_commitment(&mut localtx, &redeemscript, chan.channel_value_satoshis, &chan.secp_ctx);
+				chan_keys.sign_local_commitment(&mut localtx, &chan.secp_ctx);
 
 				assert_eq!(serialize(localtx.with_valid_witness())[..],
 						hex::decode($tx_hex).unwrap()[..]);
diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs
@@ -1086,7 +1086,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			onchain_events_waiting_threshold_conf: HashMap::new(),
 			outputs_to_watch: HashMap::new(),
 
-			onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, funding_redeemscript, their_to_self_delay, logger.clone()),
+			onchain_tx_handler: OnchainTxHandler::new(destination_script.clone(), keys, their_to_self_delay, logger.clone()),
 
 			lockdown_from_offchain: false,
 
@@ -1743,7 +1743,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	/// In any-case, choice is up to the user.
 	pub fn get_latest_local_commitment_txn(&mut self) -> Vec<Transaction> {
 		log_trace!(self, "Getting signed latest local commitment transaction!");
-		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis) {
+		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() {
 			let txid = commitment_tx.txid();
 			let mut res = vec![commitment_tx];
 			if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx {
@@ -1769,7 +1769,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	#[cfg(test)]
 	pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec<Transaction> {
 		log_trace!(self, "Getting signed copy of latest local commitment transaction!");
-		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx(self.channel_value_satoshis) {
+		if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx() {
 			let txid = commitment_tx.txid();
 			let mut res = vec![commitment_tx];
 			if let &Some(ref local_tx) = &self.current_local_signed_commitment_tx {
@@ -1855,7 +1855,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		}
 		if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
 			if should_broadcast {
-				if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis) {
+				if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() {
 					let (mut new_outpoints, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, cur_local_tx);
 					if !new_outputs.is_empty() {
 						watch_outputs.push((cur_local_tx.txid.clone(), new_outputs));
diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs
@@ -142,7 +142,6 @@ macro_rules! subtract_high_prio_fee {
 /// do RBF bumping if possible.
 pub struct OnchainTxHandler<ChanSigner: ChannelKeys> {
 	destination_script: Script,
-	funding_redeemscript: Script,
 	local_commitment: Option<LocalCommitmentTransaction>,
 	prev_local_commitment: Option<LocalCommitmentTransaction>,
 	local_csv: u16,
@@ -185,7 +184,6 @@ pub struct OnchainTxHandler<ChanSigner: ChannelKeys> {
 impl<ChanSigner: ChannelKeys + Writeable> OnchainTxHandler<ChanSigner> {
 	pub(crate) fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		self.destination_script.write(writer)?;
-		self.funding_redeemscript.write(writer)?;
 		self.local_commitment.write(writer)?;
 		self.prev_local_commitment.write(writer)?;
 
@@ -231,7 +229,6 @@ impl<ChanSigner: ChannelKeys + Writeable> OnchainTxHandler<ChanSigner> {
 impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for OnchainTxHandler<ChanSigner> {
 	fn read<R: ::std::io::Read>(reader: &mut R, logger: Arc<Logger>) -> Result<Self, DecodeError> {
 		let destination_script = Readable::read(reader)?;
-		let funding_redeemscript = Readable::read(reader)?;
 
 		let local_commitment = Readable::read(reader)?;
 		let prev_local_commitment = Readable::read(reader)?;
@@ -285,7 +282,6 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for OnchainTx
 
 		Ok(OnchainTxHandler {
 			destination_script,
-			funding_redeemscript,
 			local_commitment,
 			prev_local_commitment,
 			local_csv,
@@ -300,13 +296,12 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for OnchainTx
 }
 
 impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
-	pub(super) fn new(destination_script: Script, keys: ChanSigner, funding_redeemscript: Script, local_csv: u16, logger: Arc<Logger>) -> Self {
+	pub(super) fn new(destination_script: Script, keys: ChanSigner, local_csv: u16, logger: Arc<Logger>) -> Self {
 
 		let key_storage = keys;
 
 		OnchainTxHandler {
 			destination_script,
-			funding_redeemscript,
 			local_commitment: None,
 			prev_local_commitment: None,
 			local_csv,
@@ -537,7 +532,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 						return None;
 					},
 					&InputMaterial::Funding { ref channel_value } => {
-						let signed_tx = self.get_fully_signed_local_tx(*channel_value).unwrap();
+						let signed_tx = self.get_fully_signed_local_tx().unwrap();
 						let mut amt_outputs = 0;
 						for outp in signed_tx.output.iter() {
 							amt_outputs += outp.value;
@@ -790,19 +785,19 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 	// have empty local commitment transaction if a ChannelMonitor is asked to force-close just after Channel::get_outbound_funding_created,
 	// before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing
 	// to monitor before.
-	pub(super) fn get_fully_signed_local_tx(&mut self, channel_value_satoshis: u64) -> Option<Transaction> {
+	pub(super) fn get_fully_signed_local_tx(&mut self) -> Option<Transaction> {
 		if let Some(ref mut local_commitment) = self.local_commitment {
-			self.key_storage.sign_local_commitment(local_commitment, &self.funding_redeemscript, channel_value_satoshis, &self.secp_ctx);
+			self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx);
 			return Some(local_commitment.with_valid_witness().clone());
 		}
 		None
 	}
 
 	#[cfg(test)]
-	pub(super) fn get_fully_signed_copy_local_tx(&mut self, channel_value_satoshis: u64) -> Option<Transaction> {
+	pub(super) fn get_fully_signed_copy_local_tx(&mut self) -> Option<Transaction> {
 		if let Some(ref mut local_commitment) = self.local_commitment {
 			let mut local_commitment = local_commitment.clone();
-			self.key_storage.unsafe_sign_local_commitment(&mut local_commitment, &self.funding_redeemscript, channel_value_satoshis, &self.secp_ctx);
+			self.key_storage.unsafe_sign_local_commitment(&mut local_commitment, &self.secp_ctx);
 			return Some(local_commitment.with_valid_witness().clone());
 		}
 		None
diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs
@@ -7,7 +7,6 @@ use std::cmp;
 use std::sync::{Mutex, Arc};
 
 use bitcoin::blockdata::transaction::Transaction;
-use bitcoin::blockdata::script::Script;
 
 use secp256k1;
 use secp256k1::key::{SecretKey, PublicKey};
@@ -80,13 +79,13 @@ impl ChannelKeys for EnforcingChannelKeys {
 		Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap())
 	}
 
-	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
-		self.inner.sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx)
+	fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+		self.inner.sign_local_commitment(local_commitment_tx, secp_ctx)
 	}
 
 	#[cfg(test)]
-	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
-		self.inner.unsafe_sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx);
+	fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
+		self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx);
 	}
 
 	fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {
