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 158f71dba28bed6a1b67c2a0a9e8d640c7458a8a..e2e4403b528997e7a4ce56fb2a4591af85631efa 100644 (file)
--- 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 d0db1d85613f217cbca040191207a60fcb5d2bfa..c503aaea6c78ca2a3dccb8c45efac01d629cedfe 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1488,16 +1488,25 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                        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 113e007abead2b6c647cab3db68ffe7363cf0217..bbcbe75c67bf43bc78c0069e734c11ecff3ee447 100644 (file)
--- 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<ChanSigner: ChannelKeys> {
        Local {
                keys: ChanSigner,
@@ -785,7 +784,6 @@ impl<R: ::std::io::Read> Readable<R> 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<ChanSigner: ChannelKeys> {
        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 b9e6b569019123ee131d871a65d4e965683199fe..50120356910bcdb6fe62e9679fdad75beaa2c956 100644 (file)
--- 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 a38aea2d4374a4a118709ee86d4c51e9580d006e..2fa7cef389e51326388d81ecd855d4fe28726b25 100644 (file)
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -66,13 +66,14 @@ impl<'a> channelmonitor::ManyChannelMonitor<EnforcingChannelKeys> for TestChanne
                // to a watchtower and disk...
                let mut w = TestVecWriter(Vec::new());
                monitor.write_for_disk(&mut w).unwrap();
-               assert!(<(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(
-                               &mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1 == monitor);
+               let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::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<EnforcingChannelKeys> for TestChanne
                let monitor = monitors.get(&funding_txo).unwrap();
                w.0.clear();
                monitor.write_for_disk(&mut w).unwrap();
-               assert!(<(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(
-                               &mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1 == *monitor);
+               let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::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()
        }