From ab7a0a54318cdd55bedc3a02604af200b16e2e2c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 12 Feb 2020 15:47:04 -0500 Subject: [PATCH] Drop Clone from ChannelMonitor. This removes the somewhat-easy-to-misuse Clone from ChannelMonitors, opening us up to being able to track Events in ChannelMonitors with less risk of misuse. Sadly it doesn't remove the Clone requirement for ChannelKeys, though gets us much closer - we now just need to request a second copy once when we go to create the ChannelMonitors. --- lightning/src/chain/keysinterface.rs | 3 +- lightning/src/ln/channel.rs | 60 ++++++++++++++++++---------- lightning/src/ln/channelmonitor.rs | 2 - lightning/src/ln/functional_tests.rs | 2 +- lightning/src/util/test_utils.rs | 16 ++++---- 5 files changed, 51 insertions(+), 32 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 158f71dba..e2e4403b5 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -135,7 +135,8 @@ pub trait KeysInterface: Send + Sync { /// (TODO: We shouldn't require that, and should have an API to get them at deser time, due mostly /// to the possibility of reentrancy issues by calling the user's code during our deserialization /// routine). -/// TODO: remove Clone once we start returning ChannelUpdate objects instead of copying ChannelMonitor +/// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create +/// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors. pub trait ChannelKeys : Send+Clone { /// Gets the private key for the anchor tx fn funding_key<'a>(&'a self) -> &'a SecretKey; diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d0db1d856..c503aaea6 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1488,16 +1488,25 @@ impl Channel { let their_pubkeys = self.their_pubkeys.as_ref().unwrap(); let funding_redeemscript = self.get_funding_redeemscript(); let funding_txo_script = funding_redeemscript.to_v0_p2wsh(); - self.channel_monitor = Some(ChannelMonitor::new(self.local_keys.clone(), - &self.shutdown_pubkey, self.our_to_self_delay, - &self.destination_script, (funding_txo, funding_txo_script), - &their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint, - self.their_to_self_delay, funding_redeemscript, self.channel_value_satoshis, - self.get_commitment_transaction_number_obscure_factor(), - self.logger.clone())); - - self.channel_monitor.as_mut().unwrap().provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap()); - self.channel_monitor.as_mut().unwrap().provide_latest_local_commitment_tx_info(local_initial_commitment_tx, local_keys, self.feerate_per_kw, Vec::new()).unwrap(); + macro_rules! create_monitor { + () => { { + let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(), + &self.shutdown_pubkey, self.our_to_self_delay, + &self.destination_script, (funding_txo, funding_txo_script.clone()), + &their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint, + self.their_to_self_delay, funding_redeemscript.clone(), self.channel_value_satoshis, + self.get_commitment_transaction_number_obscure_factor(), + self.logger.clone()); + + channel_monitor.provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap()); + channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx.clone(), local_keys.clone(), self.feerate_per_kw, Vec::new()).unwrap(); + channel_monitor + } } + } + + self.channel_monitor = Some(create_monitor!()); + let channel_monitor = create_monitor!(); + self.channel_state = ChannelState::FundingSent as u32; self.channel_id = funding_txo.to_channel_id(); self.cur_remote_commitment_transaction_number -= 1; @@ -1506,7 +1515,7 @@ impl Channel { Ok((msgs::FundingSigned { channel_id: self.channel_id, signature: our_signature - }, self.channel_monitor.as_ref().unwrap().clone())) + }, channel_monitor)) } /// Handles a funding_signed message from the remote end. @@ -3330,15 +3339,24 @@ impl Channel { let their_pubkeys = self.their_pubkeys.as_ref().unwrap(); let funding_redeemscript = self.get_funding_redeemscript(); let funding_txo_script = funding_redeemscript.to_v0_p2wsh(); - self.channel_monitor = Some(ChannelMonitor::new(self.local_keys.clone(), - &self.shutdown_pubkey, self.our_to_self_delay, - &self.destination_script, (funding_txo, funding_txo_script), - &their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint, - self.their_to_self_delay, funding_redeemscript, self.channel_value_satoshis, - self.get_commitment_transaction_number_obscure_factor(), - self.logger.clone())); - - self.channel_monitor.as_mut().unwrap().provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap()); + macro_rules! create_monitor { + () => { { + let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(), + &self.shutdown_pubkey, self.our_to_self_delay, + &self.destination_script, (funding_txo, funding_txo_script.clone()), + &their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint, + self.their_to_self_delay, funding_redeemscript.clone(), self.channel_value_satoshis, + self.get_commitment_transaction_number_obscure_factor(), + self.logger.clone()); + + channel_monitor.provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap()); + channel_monitor + } } + } + + self.channel_monitor = Some(create_monitor!()); + let channel_monitor = create_monitor!(); + self.channel_state = ChannelState::FundingCreated as u32; self.channel_id = funding_txo.to_channel_id(); self.cur_remote_commitment_transaction_number -= 1; @@ -3348,7 +3366,7 @@ impl Channel { funding_txid: funding_txo.txid, funding_output_index: funding_txo.index, signature: our_signature - }, self.channel_monitor.as_ref().unwrap().clone())) + }, channel_monitor)) } /// Gets an UnsignedChannelAnnouncement, as well as a signature covering it using our diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 113e007ab..bbcbe75c6 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -390,7 +390,6 @@ pub(crate) const LATENCY_GRACE_PERIOD_BLOCKS: u32 = 3; /// keeping bumping another claim tx to solve the outpoint. pub(crate) const ANTI_REORG_DELAY: u32 = 6; -#[derive(Clone)] enum Storage { Local { keys: ChanSigner, @@ -785,7 +784,6 @@ impl Readable for ChannelMonitorUpdateStep { /// /// You MUST ensure that no ChannelMonitors for a given channel anywhere contain out-of-date /// information and are actively monitoring the chain. -#[derive(Clone)] pub struct ChannelMonitor { latest_update_id: u64, commitment_transaction_number_obscure_factor: u64, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index b9e6b5690..501203569 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -6577,7 +6577,7 @@ fn test_data_loss_protect() { }).unwrap().1 }; nodes[0].node = &node_state_0; - assert!(monitor.add_monitor(OutPoint { txid: chan.3.txid(), index: 0 }, chan_monitor.clone()).is_ok()); + assert!(monitor.add_monitor(OutPoint { txid: chan.3.txid(), index: 0 }, chan_monitor).is_ok()); nodes[0].chan_monitor = &monitor; nodes[0].chain_monitor = chain_monitor; diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index a38aea2d4..2fa7cef38 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -66,13 +66,14 @@ impl<'a> channelmonitor::ManyChannelMonitor for TestChanne // to a watchtower and disk... let mut w = TestVecWriter(Vec::new()); monitor.write_for_disk(&mut w).unwrap(); - assert!(<(Sha256dHash, channelmonitor::ChannelMonitor)>::read( - &mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1 == monitor); + let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor)>::read( + &mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1; + assert!(new_monitor == monitor); w.0.clear(); monitor.write_for_watchtower(&mut w).unwrap(); // This at least shouldn't crash... - self.added_monitors.lock().unwrap().push((funding_txo, monitor.clone())); self.latest_monitor_update_id.lock().unwrap().insert(funding_txo.to_channel_id(), (funding_txo, monitor.get_latest_update_id())); - assert!(self.simple_monitor.add_monitor(funding_txo, monitor).is_ok()); + self.added_monitors.lock().unwrap().push((funding_txo, monitor)); + assert!(self.simple_monitor.add_monitor(funding_txo, new_monitor).is_ok()); self.update_ret.lock().unwrap().clone() } @@ -91,11 +92,12 @@ impl<'a> channelmonitor::ManyChannelMonitor for TestChanne let monitor = monitors.get(&funding_txo).unwrap(); w.0.clear(); monitor.write_for_disk(&mut w).unwrap(); - assert!(<(Sha256dHash, channelmonitor::ChannelMonitor)>::read( - &mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1 == *monitor); + let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor)>::read( + &mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1; + assert!(new_monitor == *monitor); w.0.clear(); monitor.write_for_watchtower(&mut w).unwrap(); // This at least shouldn't crash... - self.added_monitors.lock().unwrap().push((funding_txo, monitor.clone())); + self.added_monitors.lock().unwrap().push((funding_txo, new_monitor)); self.update_ret.lock().unwrap().clone() } -- 2.39.5