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.

Author: TheBlueMatt

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;

lightning/src/ln/channel.rs

@@ -1481,16 +1481,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;
@@ -1499,7 +1508,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.
@@ -3323,15 +3332,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;
@@ -3341,7 +3359,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

lightning/src/ln/channelmonitor.rs

@@ -381,7 +381,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,
@@ -773,7 +772,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,

lightning/src/ln/functional_tests.rs

@@ -6494,7 +6494,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;
 

lightning/src/util/test_utils.rs

@@ -66,13 +66,14 @@ impl channelmonitor::ManyChannelMonitor<EnforcingChannelKeys> for TestChannelMon
 		// 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 channelmonitor::ManyChannelMonitor<EnforcingChannelKeys> for TestChannelMon
 		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()
 	}