From ba75b3ecd7f88a79ff6392a5229c4ab6c14a8591 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 17 Apr 2020 21:26:38 -0400 Subject: [PATCH] Drop redundant parameters in sign_local_commitment_tx 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. --- lightning/src/chain/keysinterface.rs | 20 ++++++++++++++------ lightning/src/ln/channel.rs | 9 +++++---- lightning/src/ln/channelmonitor.rs | 8 ++++---- lightning/src/ln/onchaintx.rs | 17 ++++++----------- lightning/src/util/enforcing_trait_impls.rs | 9 ++++----- 5 files changed, 33 insertions(+), 30 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index fb163966..ec76262b 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -222,7 +222,7 @@ 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(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1); /// Create a signature for a local commitment transaction without enforcing one-time signing. /// @@ -230,7 +230,7 @@ pub trait ChannelKeys : Send+Clone { /// transactions. This unsafe test-only version doesn't enforce one-time signing security /// requirement. #[cfg(test)] - fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1); + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1); /// 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(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { - local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + 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(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { - local_commitment_tx.add_local_sig(&self.funding_key, funding_redeemscript, channel_value_satoshis, secp_ctx); + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + 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(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 47936ea6..879df8cd 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4418,7 +4418,7 @@ mod tests { let logger : Arc = 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::::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::::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 index 1dd12539..d24a5f52 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1086,7 +1086,7 @@ impl ChannelMonitor { 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 ChannelMonitor { /// In any-case, choice is up to the user. pub fn get_latest_local_commitment_txn(&mut self) -> Vec { 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 ChannelMonitor { #[cfg(test)] pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec { 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 ChannelMonitor { } 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 index 102a688d..cb76dcb2 100644 --- 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 { destination_script: Script, - funding_redeemscript: Script, local_commitment: Option, prev_local_commitment: Option, local_csv: u16, @@ -185,7 +184,6 @@ pub struct OnchainTxHandler { impl OnchainTxHandler { pub(crate) fn write(&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 OnchainTxHandler { impl ReadableArgs> for OnchainTxHandler { fn read(reader: &mut R, logger: Arc) -> Result { 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 ReadableArgs> for OnchainTx Ok(OnchainTxHandler { destination_script, - funding_redeemscript, local_commitment, prev_local_commitment, local_csv, @@ -300,13 +296,12 @@ impl ReadableArgs> for OnchainTx } impl OnchainTxHandler { - pub(super) fn new(destination_script: Script, keys: ChanSigner, funding_redeemscript: Script, local_csv: u16, logger: Arc) -> Self { + pub(super) fn new(destination_script: Script, keys: ChanSigner, local_csv: u16, logger: Arc) -> Self { let key_storage = keys; OnchainTxHandler { destination_script, - funding_redeemscript, local_commitment: None, prev_local_commitment: None, local_csv, @@ -537,7 +532,7 @@ impl OnchainTxHandler { 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 OnchainTxHandler { // 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 { + pub(super) fn get_fully_signed_local_tx(&mut self) -> Option { 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 { + pub(super) fn get_fully_signed_copy_local_tx(&mut self) -> Option { 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 index c48424f9..1e5dc707 100644 --- 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(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { - self.inner.sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx) + fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + self.inner.sign_local_commitment(local_commitment_tx, secp_ctx) } #[cfg(test)] - fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { - self.inner.unsafe_sign_local_commitment(local_commitment_tx, funding_redeemscript, channel_value_satoshis, secp_ctx); + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx); } fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { -- 2.30.2