f - pass MonitorName as parameter

Author: jkczyz

fuzz/src/utils/test_persister.rs

@@ -10,13 +10,14 @@ pub struct TestPersister {
 }
 impl chainmonitor::Persist<TestChannelSigner> for TestPersister {
 	fn persist_new_channel(
-		&self, _data: &channelmonitor::ChannelMonitor<TestChannelSigner>,
+		&self, _monitor_name: MonitorName,
+		_data: &channelmonitor::ChannelMonitor<TestChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		self.update_ret.lock().unwrap().clone()
 	}
 
 	fn update_persisted_channel(
-		&self, _update: Option<&channelmonitor::ChannelMonitorUpdate>,
+		&self, _monitor_name: MonitorName, _update: Option<&channelmonitor::ChannelMonitorUpdate>,
 		_data: &channelmonitor::ChannelMonitor<TestChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		self.update_ret.lock().unwrap().clone()

lightning-persister/src/fs_store.rs

@@ -618,7 +618,8 @@ mod tests {
 		perms.set_readonly(true);
 		fs::set_permissions(path, perms).unwrap();
 
-		match store.persist_new_channel(&added_monitors[0].1) {
+		let monitor_name = added_monitors[0].1.persistence_key();
+		match store.persist_new_channel(monitor_name, &added_monitors[0].1) {
 			ChannelMonitorUpdateStatus::UnrecoverableError => {},
 			_ => panic!("unexpected result from persisting new channel"),
 		}
@@ -665,7 +666,8 @@ mod tests {
 		// handle, hence why the test is Windows-only.
 		let store = FilesystemStore::new(":<>/".into());
 
-		match store.persist_new_channel(&added_monitors[0].1) {
+		let monitor_name = added_monitors[0].1.persistence_key();
+		match store.persist_new_channel(monitor_name, &added_monitors[0].1) {
 			ChannelMonitorUpdateStatus::UnrecoverableError => {},
 			_ => panic!("unexpected result from persisting new channel"),
 		}

lightning/src/chain/chainmonitor.rs

@@ -105,9 +105,9 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> {
 	/// Persist a new channel's data in response to a [`chain::Watch::watch_channel`] call. This is
 	/// called by [`ChannelManager`] for new channels, or may be called directly, e.g. on startup.
 	///
-	/// The data can be stored any way you want, so long as [`ChannelMonitor::persistence_key`] is
-	/// used to maintain a correct mapping with the stored channel data. Note that you **must**
-	/// persist every new monitor to disk.
+	/// The data can be stored any way you want, so long as `monitor_name` is used to maintain a
+	/// correct mapping with the stored channel data. Note that you **must** persist every new
+	/// monitor to disk.
 	///
 	/// The [`ChannelMonitor::get_latest_update_id`] uniquely links this call to [`ChainMonitor::channel_monitor_updated`].
 	/// For [`Persist::persist_new_channel`], it is only necessary to call [`ChainMonitor::channel_monitor_updated`]
@@ -118,7 +118,7 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> {
 	///
 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	/// [`Writeable::write`]: crate::util::ser::Writeable::write
-	fn persist_new_channel(&self, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
+	fn persist_new_channel(&self, monitor_name: MonitorName, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
 
 	/// Update one channel's data. The provided [`ChannelMonitor`] has already applied the given
 	/// update.
@@ -157,7 +157,7 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> {
 	/// [`ChannelMonitorUpdateStatus`] for requirements when returning errors.
 	///
 	/// [`Writeable::write`]: crate::util::ser::Writeable::write
-	fn update_persisted_channel(&self, monitor_update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
+	fn update_persisted_channel(&self, monitor_name: MonitorName, monitor_update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
 	/// Prevents the channel monitor from being loaded on startup.
 	///
 	/// Archiving the data in a backup location (rather than deleting it fully) is useful for
@@ -343,7 +343,7 @@ where C::Target: chain::Filter,
 			// `ChannelMonitorUpdate` after a channel persist for a channel with the same
 			// `latest_update_id`.
 			let _pending_monitor_updates = monitor_state.pending_monitor_updates.lock().unwrap();
-			match self.persister.update_persisted_channel(None, monitor) {
+			match self.persister.update_persisted_channel(monitor.persistence_key(), None, monitor) {
 				ChannelMonitorUpdateStatus::Completed =>
 					log_trace!(logger, "Finished syncing Channel Monitor for channel {} for block-data",
 						log_funding_info!(monitor)
@@ -642,7 +642,7 @@ where C::Target: chain::Filter,
 				have_monitors_to_prune = true;
 			}
 			if needs_persistence {
-				self.persister.update_persisted_channel(None, &monitor_holder.monitor);
+				self.persister.update_persisted_channel(monitor_holder.monitor.persistence_key(), None, &monitor_holder.monitor);
 			}
 		}
 		if have_monitors_to_prune {
@@ -769,7 +769,7 @@ where C::Target: chain::Filter,
 		log_trace!(logger, "Got new ChannelMonitor for channel {}", log_funding_info!(monitor));
 		let update_id = monitor.get_latest_update_id();
 		let mut pending_monitor_updates = Vec::new();
-		let persist_res = self.persister.persist_new_channel(&monitor);
+		let persist_res = self.persister.persist_new_channel(monitor.persistence_key(), &monitor);
 		match persist_res {
 			ChannelMonitorUpdateStatus::InProgress => {
 				log_info!(logger, "Persistence of new ChannelMonitor for channel {} in progress", log_funding_info!(monitor));
@@ -832,9 +832,9 @@ where C::Target: chain::Filter,
 					// while reading `channel_monitor` with updates from storage. Instead, we should persist
 					// the entire `channel_monitor` here.
 					log_warn!(logger, "Failed to update ChannelMonitor for channel {}. Going ahead and persisting the entire ChannelMonitor", log_funding_info!(monitor));
-					self.persister.update_persisted_channel(None, monitor)
+					self.persister.update_persisted_channel(monitor.persistence_key(), None, monitor)
 				} else {
-					self.persister.update_persisted_channel(Some(update), monitor)
+					self.persister.update_persisted_channel(monitor.persistence_key(), Some(update), monitor)
 				};
 				match persist_res {
 					ChannelMonitorUpdateStatus::InProgress => {

lightning/src/chain/channelmonitor.rs

@@ -1466,13 +1466,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		})
 	}
 
-	/// Returns a unique id for persisting the [`ChannelMonitor`], which may be useful as a key in a
+	/// Returns a unique id for persisting the [`ChannelMonitor`], which is used as a key in a
 	/// key-value store.
 	///
 	/// Note: Previously, the funding outpoint was used in the [`Persist`] trait. However, since the
-	/// outpoint may change during splicing, using this method to obtain a unique key is recommended
-	/// instead. For v1 channels, the funding outpoint is still used for backwards compatibility,
-	/// whereas v2 channels use the channel id since it is fixed.
+	/// outpoint may change during splicing, this method is used to obtain a unique key instead. For
+	/// v1 channels, the funding outpoint is still used for backwards compatibility, whereas v2
+	/// channels use the channel id since it is fixed.
 	///
 	/// [`Persist`]: crate::chain::chainmonitor::Persist
 	pub fn persistence_key(&self) -> MonitorName {

lightning/src/util/persist.rs

@@ -261,9 +261,8 @@ impl<ChannelSigner: EcdsaChannelSigner, K: KVStore + ?Sized> Persist<ChannelSign
 	// just shut down the node since we're not retrying persistence!
 
 	fn persist_new_channel(
-		&self, monitor: &ChannelMonitor<ChannelSigner>,
+		&self, monitor_name: MonitorName, monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
-		let monitor_name = monitor.persistence_key();
 		match self.write(
 			CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 			CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
@@ -276,9 +275,9 @@ impl<ChannelSigner: EcdsaChannelSigner, K: KVStore + ?Sized> Persist<ChannelSign
 	}
 
 	fn update_persisted_channel(
-		&self, _update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>,
+		&self, monitor_name: MonitorName, _update: Option<&ChannelMonitorUpdate>,
+		monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
-		let monitor_name = monitor.persistence_key();
 		match self.write(
 			CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 			CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
@@ -706,10 +705,9 @@ where
 	/// Persists a new channel. This means writing the entire monitor to the
 	/// parametrized [`KVStore`].
 	fn persist_new_channel(
-		&self, monitor: &ChannelMonitor<ChannelSigner>,
+		&self, monitor_name: MonitorName, monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		// Determine the proper key for this monitor
-		let monitor_name = monitor.persistence_key();
 		let monitor_key = monitor_name.to_string();
 		// Serialize and write the new monitor
 		let mut monitor_bytes = Vec::with_capacity(
@@ -748,15 +746,15 @@ where
 	///	    `update` is `None`.
 	///   - The update is at [`u64::MAX`], indicating an update generated by pre-0.1 LDK.
 	fn update_persisted_channel(
-		&self, update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>,
+		&self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>,
+		monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX;
 		if let Some(update) = update {
-			let monitor_name = monitor.persistence_key();
+			let monitor_key = monitor_name.to_string();
 			let persist_update = update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID
 				&& update.update_id % self.maximum_pending_updates != 0;
 			if persist_update {
-				let monitor_key = monitor_name.to_string();
 				let update_name = UpdateName::from(update.update_id);
 				match self.kv_store.write(
 					CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
@@ -778,7 +776,6 @@ where
 					},
 				}
 			} else {
-				let monitor_key = monitor_name.to_string();
 				// In case of channel-close monitor update, we need to read old monitor before persisting
 				// the new one in order to determine the cleanup range.
 				let maybe_old_monitor = match monitor.get_latest_update_id() {
@@ -789,7 +786,7 @@ where
 				};
 
 				// We could write this update, but it meets criteria of our design that calls for a full monitor write.
-				let monitor_update_status = self.persist_new_channel(monitor);
+				let monitor_update_status = self.persist_new_channel(monitor_name, monitor);
 
 				if let chain::ChannelMonitorUpdateStatus::Completed = monitor_update_status {
 					let channel_closed_legacy =
@@ -820,7 +817,7 @@ where
 			}
 		} else {
 			// There is no update given, so we must persist a new monitor.
-			self.persist_new_channel(monitor)
+			self.persist_new_channel(monitor_name, monitor)
 		}
 	}
 
@@ -923,7 +920,7 @@ where
 /// // Using MonitorName to generate a storage key
 /// let storage_key = format!("channel_monitors/{}", monitor_name);
 /// ```
-#[derive(Clone, Debug, Eq, Hash, PartialEq)]
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
 pub enum MonitorName {
 	/// The outpoint of the channel's funding transaction.
 	V1Channel(OutPoint),
@@ -1358,7 +1355,8 @@ mod tests {
 				broadcaster: node_cfgs[0].tx_broadcaster,
 				fee_estimator: node_cfgs[0].fee_estimator,
 			};
-			match ro_persister.persist_new_channel(&added_monitors[0].1) {
+			let monitor_name = added_monitors[0].1.persistence_key();
+			match ro_persister.persist_new_channel(monitor_name, &added_monitors[0].1) {
 				ChannelMonitorUpdateStatus::UnrecoverableError => {
 					// correct result
 				},
@@ -1369,7 +1367,11 @@ mod tests {
 					panic!("Returned InProgress when shouldn't have")
 				},
 			}
-			match ro_persister.update_persisted_channel(Some(cmu), &added_monitors[0].1) {
+			match ro_persister.update_persisted_channel(
+				monitor_name,
+				Some(cmu),
+				&added_monitors[0].1,
+			) {
 				ChannelMonitorUpdateStatus::UnrecoverableError => {
 					// correct result
 				},

lightning/src/util/test_utils.rs

@@ -588,9 +588,9 @@ impl WatchtowerPersister {
 #[cfg(test)]
 impl<Signer: sign::ecdsa::EcdsaChannelSigner> Persist<Signer> for WatchtowerPersister {
 	fn persist_new_channel(
-		&self, data: &ChannelMonitor<Signer>,
+		&self, monitor_name: MonitorName, data: &ChannelMonitor<Signer>,
 	) -> chain::ChannelMonitorUpdateStatus {
-		let res = self.persister.persist_new_channel(data);
+		let res = self.persister.persist_new_channel(monitor_name, data);
 
 		assert!(self
 			.unsigned_justice_tx_data
@@ -621,9 +621,10 @@ impl<Signer: sign::ecdsa::EcdsaChannelSigner> Persist<Signer> for WatchtowerPers
 	}
 
 	fn update_persisted_channel(
-		&self, update: Option<&ChannelMonitorUpdate>, data: &ChannelMonitor<Signer>,
+		&self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>,
+		data: &ChannelMonitor<Signer>,
 	) -> chain::ChannelMonitorUpdateStatus {
-		let res = self.persister.update_persisted_channel(update, data);
+		let res = self.persister.update_persisted_channel(monitor_name, update, data);
 
 		if let Some(update) = update {
 			let commitment_txs = data.counterparty_commitment_txs_from_update(update);
@@ -697,7 +698,7 @@ impl TestPersister {
 }
 impl<Signer: sign::ecdsa::EcdsaChannelSigner> Persist<Signer> for TestPersister {
 	fn persist_new_channel(
-		&self, _data: &ChannelMonitor<Signer>,
+		&self, _monitor_name: MonitorName, _data: &ChannelMonitor<Signer>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		if let Some(update_ret) = self.update_rets.lock().unwrap().pop_front() {
 			return update_ret;
@@ -706,14 +707,14 @@ impl<Signer: sign::ecdsa::EcdsaChannelSigner> Persist<Signer> for TestPersister
 	}
 
 	fn update_persisted_channel(
-		&self, update: Option<&ChannelMonitorUpdate>, data: &ChannelMonitor<Signer>,
+		&self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>,
+		_data: &ChannelMonitor<Signer>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		let mut ret = chain::ChannelMonitorUpdateStatus::Completed;
 		if let Some(update_ret) = self.update_rets.lock().unwrap().pop_front() {
 			ret = update_ret;
 		}
 
-		let monitor_name = data.persistence_key();
 		if let Some(update) = update {
 			self.offchain_monitor_updates
 				.lock()