From 6267610e29030e188cc15abea498844aa7a8f776 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 4 Feb 2025 16:43:35 -0600 Subject: [PATCH] f - pass MonitorName as parameter --- fuzz/src/utils/test_persister.rs | 5 +++-- lightning-persister/src/fs_store.rs | 6 +++-- lightning/src/chain/chainmonitor.rs | 20 ++++++++--------- lightning/src/chain/channelmonitor.rs | 8 +++---- lightning/src/util/persist.rs | 32 ++++++++++++++------------- lightning/src/util/test_utils.rs | 15 +++++++------ 6 files changed, 46 insertions(+), 40 deletions(-) diff --git a/fuzz/src/utils/test_persister.rs b/fuzz/src/utils/test_persister.rs index e7296563e2d..5838a961c0f 100644 --- a/fuzz/src/utils/test_persister.rs +++ b/fuzz/src/utils/test_persister.rs @@ -10,13 +10,14 @@ pub struct TestPersister { } impl chainmonitor::Persist for TestPersister { fn persist_new_channel( - &self, _data: &channelmonitor::ChannelMonitor, + &self, _monitor_name: MonitorName, + _data: &channelmonitor::ChannelMonitor, ) -> 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, ) -> chain::ChannelMonitorUpdateStatus { self.update_ret.lock().unwrap().clone() diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs index 7d6d9a44533..e396b1fcec7 100644 --- a/lightning-persister/src/fs_store.rs +++ b/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"), } diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 19fc8f857e2..fa903d388b9 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -105,9 +105,9 @@ pub trait Persist { /// 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 { /// /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager /// [`Writeable::write`]: crate::util::ser::Writeable::write - fn persist_new_channel(&self, monitor: &ChannelMonitor) -> ChannelMonitorUpdateStatus; + fn persist_new_channel(&self, monitor_name: MonitorName, monitor: &ChannelMonitor) -> ChannelMonitorUpdateStatus; /// Update one channel's data. The provided [`ChannelMonitor`] has already applied the given /// update. @@ -157,7 +157,7 @@ pub trait Persist { /// [`ChannelMonitorUpdateStatus`] for requirements when returning errors. /// /// [`Writeable::write`]: crate::util::ser::Writeable::write - fn update_persisted_channel(&self, monitor_update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor) -> ChannelMonitorUpdateStatus; + fn update_persisted_channel(&self, monitor_name: MonitorName, monitor_update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor) -> 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 => { diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 7ae7f4f8264..55d168542ac 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1466,13 +1466,13 @@ impl ChannelMonitor { }) } - /// 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 { diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs index 3676c3600cf..88899fd05a4 100644 --- a/lightning/src/util/persist.rs +++ b/lightning/src/util/persist.rs @@ -261,9 +261,8 @@ impl Persist, + &self, monitor_name: MonitorName, monitor: &ChannelMonitor, ) -> 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 Persist, monitor: &ChannelMonitor, + &self, monitor_name: MonitorName, _update: Option<&ChannelMonitorUpdate>, + monitor: &ChannelMonitor, ) -> 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, + &self, monitor_name: MonitorName, monitor: &ChannelMonitor, ) -> 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, + &self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>, + monitor: &ChannelMonitor, ) -> 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 }, diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 68ade38bd83..f17185ea5ad 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -588,9 +588,9 @@ impl WatchtowerPersister { #[cfg(test)] impl Persist for WatchtowerPersister { fn persist_new_channel( - &self, data: &ChannelMonitor, + &self, monitor_name: MonitorName, data: &ChannelMonitor, ) -> 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 Persist for WatchtowerPers } fn update_persisted_channel( - &self, update: Option<&ChannelMonitorUpdate>, data: &ChannelMonitor, + &self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>, + data: &ChannelMonitor, ) -> 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 Persist for TestPersister { fn persist_new_channel( - &self, _data: &ChannelMonitor, + &self, _monitor_name: MonitorName, _data: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { if let Some(update_ret) = self.update_rets.lock().unwrap().pop_front() { return update_ret; @@ -706,14 +707,14 @@ impl Persist for TestPersister } fn update_persisted_channel( - &self, update: Option<&ChannelMonitorUpdate>, data: &ChannelMonitor, + &self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>, + _data: &ChannelMonitor, ) -> 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()