From: Matt Corallo Date: Wed, 12 Feb 2020 20:47:04 +0000 (-0500) Subject: Drop Clone from ChannelMonitor. X-Git-Tag: v0.0.12~119^2~1 X-Git-Url: http://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commitdiff_plain;h=ab7a0a54318cdd55bedc3a02604af200b16e2e2c 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. --- diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 158f71db..e2e4403b 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 d0db1d85..c503aaea 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 113e007a..bbcbe75c 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 b9e6b569..50120356 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 a38aea2d..2fa7cef3 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() }