From d2564a9563e36fcb18f735f323c16b866aab816b Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 5 Feb 2025 13:21:36 -0600 Subject: [PATCH 1/7] Fix broken dual_funding_tests An earlier commit broke test compilation under --cfg=dual_funding. --- lightning/src/ln/dual_funding_tests.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/dual_funding_tests.rs b/lightning/src/ln/dual_funding_tests.rs index 8cd47ad1765..e73f1192222 100644 --- a/lightning/src/ln/dual_funding_tests.rs +++ b/lightning/src/ln/dual_funding_tests.rs @@ -229,14 +229,17 @@ fn do_test_v2_channel_establishment( chanmon_cfgs[1] .persister .set_update_ret(crate::chain::ChannelMonitorUpdateStatus::Completed); - let (outpoint, latest_update, _) = *nodes[1] + let (latest_update, _) = *nodes[1] .chain_monitor .latest_monitor_update_id .lock() .unwrap() .get(&channel_id) .unwrap(); - nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update); + nodes[1] + .chain_monitor + .chain_monitor + .force_channel_monitor_updated(channel_id, latest_update); } let events = nodes[1].node.get_and_clear_pending_events(); From 220dfc9e10410ef04b8acf47b6e8ed170cf1b305 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 24 Jan 2025 16:55:40 -0600 Subject: [PATCH 2/7] Use MonitorName when persisting ChannelMonitors Instead repeating the MonitorName formatting when persisting monitors, DRY up the code to use MonitorName::from instead. --- lightning/src/util/persist.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs index 52ba75a8c73..48150dd93f8 100644 --- a/lightning/src/util/persist.rs +++ b/lightning/src/util/persist.rs @@ -261,11 +261,11 @@ impl Persist, ) -> chain::ChannelMonitorUpdateStatus { - let key = format!("{}_{}", funding_txo.txid.to_string(), funding_txo.index); + let monitor_name = MonitorName::from(funding_txo); match self.write( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - &key, + monitor_name.as_str(), &monitor.encode(), ) { Ok(()) => chain::ChannelMonitorUpdateStatus::Completed, @@ -277,11 +277,11 @@ impl Persist, monitor: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { - let key = format!("{}_{}", funding_txo.txid.to_string(), funding_txo.index); + let monitor_name = MonitorName::from(funding_txo); match self.write( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - &key, + monitor_name.as_str(), &monitor.encode(), ) { Ok(()) => chain::ChannelMonitorUpdateStatus::Completed, From 321abbaa07173c1f3ee8ed58b21d79611a6f42e0 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 24 Jan 2025 16:17:34 -0600 Subject: [PATCH 3/7] Support parsing persisted v2 channel monitor names ChannelMonitors are persisted using the corresponding channel's funding outpoint for the key. This is fine for v1 channels since the funding output will never change. However, this is not the case for v2 channels since a splice will result in a new funding outpoint. Support parsing a MonitorName for v2 channels as a ChannelId, which still looks like a outpoint only without an index. This allows differentiating monitors for v1 channels from those of v2 channels. --- lightning/src/util/persist.rs | 262 ++++++++++++++++++++-------------- 1 file changed, 154 insertions(+), 108 deletions(-) diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs index 48150dd93f8..65fbe10d863 100644 --- a/lightning/src/util/persist.rs +++ b/lightning/src/util/persist.rs @@ -10,6 +10,7 @@ //! //! [`ChannelManager`]: crate::ln::channelmanager::ChannelManager +use bitcoin::hashes::hex::FromHex; use bitcoin::{BlockHash, Txid}; use core::cmp; use core::ops::Deref; @@ -24,6 +25,7 @@ use crate::chain::chainmonitor::Persist; use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate}; use crate::chain::transaction::OutPoint; use crate::ln::channelmanager::AChannelManager; +use crate::ln::types::ChannelId; use crate::routing::gossip::NetworkGraph; use crate::routing::scoring::WriteableScore; use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, SignerProvider}; @@ -265,7 +267,7 @@ impl Persist chain::ChannelMonitorUpdateStatus::Completed, @@ -281,7 +283,7 @@ impl Persist chain::ChannelMonitorUpdateStatus::Completed, @@ -291,10 +293,11 @@ impl Persist monitor, Err(_) => return, @@ -302,7 +305,7 @@ impl Persist {}, @@ -311,7 +314,7 @@ impl Persist Result<(BlockHash, ChannelMonitor<::EcdsaSigner>), io::Error> { - let monitor_name = MonitorName::new(monitor_key)?; - let (block_hash, monitor) = self.read_monitor(&monitor_name)?; + let monitor_name = MonitorName::from_str(monitor_key)?; + let (block_hash, monitor) = self.read_monitor(&monitor_name, monitor_key)?; let mut current_update_id = monitor.get_latest_update_id(); loop { current_update_id = match current_update_id.checked_add(1) { @@ -572,7 +575,7 @@ where None => break, }; let update_name = UpdateName::from(current_update_id); - let update = match self.read_monitor_update(&monitor_name, &update_name) { + let update = match self.read_monitor_update(monitor_key, &update_name) { Ok(update) => update, Err(err) if err.kind() == io::ErrorKind::NotFound => { // We can't find any more updates, so we are done. @@ -587,7 +590,7 @@ where log_error!( self.logger, "Monitor update failed. monitor: {} update: {} reason: {:?}", - monitor_name.as_str(), + monitor_key, update_name.as_str(), e ); @@ -599,14 +602,22 @@ where /// Read a channel monitor. fn read_monitor( - &self, monitor_name: &MonitorName, + &self, monitor_name: &MonitorName, monitor_key: &str, ) -> Result<(BlockHash, ChannelMonitor<::EcdsaSigner>), io::Error> { - let outpoint: OutPoint = monitor_name.try_into()?; + let outpoint = match monitor_name { + MonitorName::V1Channel(outpoint) => outpoint, + MonitorName::V2Channel(_) => { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Invalid outpoint in stored key", + )) + }, + }; let mut monitor_cursor = io::Cursor::new(self.kv_store.read( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - monitor_name.as_str(), + monitor_key, )?); // Discard the sentinel bytes if found. if monitor_cursor.get_ref().starts_with(MONITOR_UPDATING_PERSISTER_PREPEND_SENTINEL) { @@ -623,7 +634,7 @@ where log_error!( self.logger, "ChannelMonitor {} was stored under the wrong key!", - monitor_name.as_str() + monitor_key, ); Err(io::Error::new( io::ErrorKind::InvalidData, @@ -637,7 +648,7 @@ where log_error!( self.logger, "Failed to read ChannelMonitor {}, reason: {}", - monitor_name.as_str(), + monitor_key, e, ); Err(io::Error::new(io::ErrorKind::InvalidData, "Failed to read ChannelMonitor")) @@ -647,11 +658,11 @@ where /// Read a channel monitor update. fn read_monitor_update( - &self, monitor_name: &MonitorName, update_name: &UpdateName, + &self, monitor_key: &str, update_name: &UpdateName, ) -> Result { let update_bytes = self.kv_store.read( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + monitor_key, update_name.as_str(), )?; ChannelMonitorUpdate::read(&mut io::Cursor::new(update_bytes)).map_err(|e| { @@ -659,7 +670,7 @@ where self.logger, "Failed to read ChannelMonitorUpdate {}/{}/{}, reason: {}", CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + monitor_key, update_name.as_str(), e, ); @@ -679,19 +690,18 @@ where CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, )?; for monitor_key in monitor_keys { - let monitor_name = MonitorName::new(monitor_key)?; - let (_, current_monitor) = self.read_monitor(&monitor_name)?; - let updates = self.kv_store.list( - CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), - )?; + let monitor_name = MonitorName::from_str(&monitor_key)?; + let (_, current_monitor) = self.read_monitor(&monitor_name, &monitor_key)?; + let updates = self + .kv_store + .list(CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, monitor_key.as_str())?; for update in updates { let update_name = UpdateName::new(update)?; // if the update_id is lower than the stored monitor, delete if update_name.0 <= current_monitor.get_latest_update_id() { self.kv_store.remove( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), update_name.as_str(), lazy, )?; @@ -726,6 +736,7 @@ where ) -> chain::ChannelMonitorUpdateStatus { // Determine the proper key for this monitor let monitor_name = MonitorName::from(funding_txo); + let monitor_key = monitor_name.to_string(); // Serialize and write the new monitor let mut monitor_bytes = Vec::with_capacity( MONITOR_UPDATING_PERSISTER_PREPEND_SENTINEL.len() + monitor.serialized_length(), @@ -735,7 +746,7 @@ where match self.kv_store.write( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), &monitor_bytes, ) { Ok(_) => chain::ChannelMonitorUpdateStatus::Completed, @@ -745,7 +756,7 @@ where "Failed to write ChannelMonitor {}/{}/{} reason: {}", CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), e ); chain::ChannelMonitorUpdateStatus::UnrecoverableError @@ -772,10 +783,11 @@ where && update.update_id % self.maximum_pending_updates != 0; if persist_update { let monitor_name = MonitorName::from(funding_txo); + 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, - monitor_name.as_str(), + monitor_key.as_str(), update_name.as_str(), &update.encode(), ) { @@ -785,7 +797,7 @@ where self.logger, "Failed to write ChannelMonitorUpdate {}/{}/{} reason: {}", CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), update_name.as_str(), e ); @@ -794,10 +806,13 @@ where } } else { let monitor_name = MonitorName::from(funding_txo); + 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() { - LEGACY_CLOSED_CHANNEL_UPDATE_ID => self.read_monitor(&monitor_name).ok(), + LEGACY_CLOSED_CHANNEL_UPDATE_ID => { + self.read_monitor(&monitor_name, &monitor_key).ok() + }, _ => None, }; @@ -839,15 +854,15 @@ where fn archive_persisted_channel(&self, funding_txo: OutPoint) { let monitor_name = MonitorName::from(funding_txo); - let monitor_key = monitor_name.as_str().to_string(); - let monitor = match self.read_channel_monitor_with_updates(monitor_key) { + let monitor_key = monitor_name.to_string(); + let monitor = match self.read_channel_monitor_with_updates(&monitor_key) { Ok((_block_hash, monitor)) => monitor, Err(_) => return, }; match self.kv_store.write( ARCHIVED_CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, ARCHIVED_CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), &monitor.encode(), ) { Ok(()) => {}, @@ -856,7 +871,7 @@ where let _ = self.kv_store.remove( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), true, ); } @@ -874,18 +889,19 @@ where { // Cleans up monitor updates for given monitor in range `start..=end`. fn cleanup_in_range(&self, monitor_name: MonitorName, start: u64, end: u64) { + let monitor_key = monitor_name.to_string(); for update_id in start..=end { let update_name = UpdateName::from(update_id); if let Err(e) = self.kv_store.remove( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + monitor_key.as_str(), update_name.as_str(), true, ) { log_error!( self.logger, "Failed to clean up channel monitor updates for monitor {}, reason: {}", - monitor_name.as_str(), + monitor_key.as_str(), e ); }; @@ -896,9 +912,12 @@ where /// A struct representing a name for a channel monitor. /// /// `MonitorName` is primarily used within the [`MonitorUpdatingPersister`] -/// in functions that store or retrieve channel monitor snapshots. +/// in functions that store or retrieve [`ChannelMonitor`] snapshots. /// It provides a consistent way to generate a unique key for channel -/// monitors based on their funding outpoints. +/// monitors based on the channel's funding [`OutPoint`] for v1 channels or +/// [`ChannelId`] for v2 channels. The former uses the channel's funding +/// outpoint for historical reasons. The latter uses the channel's id because +/// the funding outpoint may change during splicing. /// /// While users of the Lightning Dev Kit library generally won't need /// to interact with [`MonitorName`] directly, it can be useful for: @@ -912,86 +931,95 @@ where /// use std::str::FromStr; /// /// use bitcoin::Txid; +/// use bitcoin::hashes::hex::FromHex; /// /// use lightning::util::persist::MonitorName; /// use lightning::chain::transaction::OutPoint; +/// use lightning::ln::types::ChannelId; /// +/// // v1 channel /// let outpoint = OutPoint { /// txid: Txid::from_str("deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef").unwrap(), /// index: 1, /// }; /// let monitor_name = MonitorName::from(outpoint); -/// assert_eq!(monitor_name.as_str(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_1"); +/// assert_eq!(&monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_1"); +/// +/// // v2 channel +/// let channel_id = ChannelId(<[u8; 32]>::from_hex("deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef").unwrap()); +/// let monitor_name = MonitorName::from(channel_id); +/// assert_eq!(&monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); /// /// // Using MonitorName to generate a storage key -/// let storage_key = format!("channel_monitors/{}", monitor_name.as_str()); +/// let storage_key = format!("channel_monitors/{}", monitor_name); /// ``` #[derive(Debug)] -pub struct MonitorName(String); - -impl MonitorName { - /// Constructs a [`MonitorName`], after verifying that an [`OutPoint`] can - /// be formed from the given `name`. - /// This method is useful if you have a String and you want to verify that - /// it's a valid storage key for a channel monitor. - pub fn new(name: String) -> Result { - MonitorName::do_try_into_outpoint(&name)?; - Ok(Self(name)) - } +pub enum MonitorName { + /// The outpoint of the channel's funding transaction. + V1Channel(OutPoint), - /// Convert this monitor name to a str. - /// This method is particularly useful when you need to use the monitor name - /// as a key in a key-value store or when logging. - pub fn as_str(&self) -> &str { - &self.0 - } + /// The id of the channel produced by [`ChannelId::v2_from_revocation_basepoints`]. + V2Channel(ChannelId), +} - /// Attempt to form a valid [`OutPoint`] from a given name string. - fn do_try_into_outpoint(name: &str) -> Result { - let mut parts = name.splitn(2, '_'); - let txid = if let Some(part) = parts.next() { - Txid::from_str(part).map_err(|_| { +impl MonitorName { + /// Attempts to construct a `MonitorName` from a storage key returned by [`KVStore::list`]. + /// + /// This is useful when you need to reconstruct the original data the key represents. + fn from_str(monitor_key: &str) -> Result { + let mut parts = monitor_key.splitn(2, '_'); + let id = parts + .next() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "Empty stored key"))?; + + if let Some(part) = parts.next() { + let txid = Txid::from_str(id).map_err(|_| { io::Error::new(io::ErrorKind::InvalidData, "Invalid tx ID in stored key") - })? - } else { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "Stored monitor key is not a splittable string", - )); - }; - let index = if let Some(part) = parts.next() { - part.parse().map_err(|_| { + })?; + let index: u16 = part.parse().map_err(|_| { io::Error::new(io::ErrorKind::InvalidData, "Invalid tx index in stored key") - })? + })?; + let outpoint = OutPoint { txid, index }; + Ok(MonitorName::V1Channel(outpoint)) } else { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "No tx index value found after underscore in stored key", - )); - }; - Ok(OutPoint { txid, index }) + let bytes = <[u8; 32]>::from_hex(id).map_err(|_| { + io::Error::new(io::ErrorKind::InvalidData, "Invalid channel ID in stored key") + })?; + Ok(MonitorName::V2Channel(ChannelId(bytes))) + } } } -impl TryFrom<&MonitorName> for OutPoint { - type Error = io::Error; - - /// Attempts to convert a `MonitorName` back into an `OutPoint`. +impl From for MonitorName { + /// Creates a `MonitorName` from an `OutPoint`. /// - /// This is useful when you have a `MonitorName` (perhaps retrieved from storage) - /// and need to reconstruct the original `OutPoint` it represents. - fn try_from(value: &MonitorName) -> Result { - MonitorName::do_try_into_outpoint(&value.0) + /// This is typically used when you need to generate a storage key or identifier + /// for a new or existing channel monitor for a v1 channel. + fn from(outpoint: OutPoint) -> Self { + MonitorName::V1Channel(outpoint) } } -impl From for MonitorName { - /// Creates a `MonitorName` from an `OutPoint`. +impl From for MonitorName { + /// Creates a `MonitorName` from a `ChannelId`. /// /// This is typically used when you need to generate a storage key or identifier - /// for a new or existing channel monitor. - fn from(value: OutPoint) -> Self { - MonitorName(format!("{}_{}", value.txid.to_string(), value.index)) + /// for a new or existing channel monitor for a v2 channel. + fn from(channel_id: ChannelId) -> Self { + MonitorName::V2Channel(channel_id) + } +} + +impl core::fmt::Display for MonitorName { + fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { + match self { + MonitorName::V1Channel(outpoint) => { + write!(f, "{}_{}", outpoint.txid, outpoint.index) + }, + MonitorName::V2Channel(channel_id) => { + write!(f, "{}", channel_id) + }, + } } } @@ -1092,6 +1120,7 @@ mod tests { use crate::util::test_channel_signer::TestChannelSigner; use crate::util::test_utils::{self, TestLogger, TestStore}; use crate::{check_added_monitors, check_closed_broadcast}; + use bitcoin::hashes::hex::FromHex; const EXPECTED_UPDATES_PER_PAYMENT: u64 = 5; @@ -1109,8 +1138,8 @@ mod tests { } #[test] - fn monitor_from_outpoint_works() { - let monitor_name1 = MonitorName::from(OutPoint { + fn creates_monitor_from_outpoint() { + let monitor_name = MonitorName::from(OutPoint { txid: Txid::from_str( "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", ) @@ -1118,11 +1147,12 @@ mod tests { index: 1, }); assert_eq!( - monitor_name1.as_str(), + &monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_1" ); + assert!(matches!(monitor_name, MonitorName::V1Channel(_))); - let monitor_name2 = MonitorName::from(OutPoint { + let monitor_name = MonitorName::from(OutPoint { txid: Txid::from_str( "f33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeef", ) @@ -1130,23 +1160,39 @@ mod tests { index: u16::MAX, }); assert_eq!( - monitor_name2.as_str(), + &monitor_name.to_string(), "f33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeef_65535" ); + assert!(matches!(monitor_name, MonitorName::V1Channel(_))); + } + + #[test] + fn creates_monitor_from_channel_id() { + let monitor_name = MonitorName::from(ChannelId( + <[u8; 32]>::from_hex( + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", + ) + .unwrap(), + )); + assert_eq!( + &monitor_name.to_string(), + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + ); + assert!(matches!(monitor_name, MonitorName::V2Channel(_))); } #[test] - fn bad_monitor_string_fails() { - assert!(MonitorName::new( - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef".to_string() + fn fails_parsing_monitor_name() { + assert!(MonitorName::from_str( + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_" ) .is_err()); - assert!(MonitorName::new( - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_65536".to_string() + assert!(MonitorName::from_str( + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_65536" ) .is_err()); - assert!(MonitorName::new( - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_21".to_string() + assert!(MonitorName::from_str( + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_21" ) .is_err()); } @@ -1225,7 +1271,7 @@ mod tests { .kv_store .list( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str() + &monitor_name.to_string() ) .unwrap() .len() as u64, @@ -1244,7 +1290,7 @@ mod tests { .kv_store .list( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str() + &monitor_name.to_string() ) .unwrap() .len() as u64, @@ -1438,7 +1484,7 @@ mod tests { .kv_store .write( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + &monitor_name.to_string(), UpdateName::from(1).as_str(), &[0u8; 1], ) @@ -1452,7 +1498,7 @@ mod tests { .kv_store .read( CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), + &monitor_name.to_string(), UpdateName::from(1).as_str() ) .is_err()); From afed8ad323da5775d8cd62b81384eb9975cb13c4 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 27 Jan 2025 16:24:56 -0600 Subject: [PATCH 4/7] Save first confirmed funding txo in ChannelMonitor Currently, when a ChannelMonitor is persisted, it's funding txo is used as the key. However, when a channel is spliced, there is a new funding txo. Store the first confirmed funding txo in ChannelMonitor such that the persistence does not change. This will be used for v1 channels for backward compatibility while v2 channels will use the channel ID, which for them is not derived from the funding txo. This prevents needing to migrate previously-persisted ChannelMonitors. --- lightning/src/chain/channelmonitor.rs | 8 ++++++++ lightning/src/ln/channel.rs | 2 ++ 2 files changed, 10 insertions(+) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 6da544655f3..3189856862a 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -879,6 +879,7 @@ pub(crate) struct ChannelMonitorImpl { holder_revocation_basepoint: RevocationBasepoint, channel_id: ChannelId, funding_info: (OutPoint, ScriptBuf), + first_confirmed_funding_txo: OutPoint, current_counterparty_commitment_txid: Option, prev_counterparty_commitment_txid: Option, @@ -1246,6 +1247,7 @@ impl Writeable for ChannelMonitorImpl { (21, self.balances_empty_height, option), (23, self.holder_pays_commitment_tx_fee, option), (25, self.payment_preimages, required), + (27, self.first_confirmed_funding_txo, required), }); Ok(()) @@ -1398,6 +1400,8 @@ impl ChannelMonitor { let mut outputs_to_watch = new_hash_map(); outputs_to_watch.insert(funding_info.0.txid, vec![(funding_info.0.index as u32, funding_info.1.clone())]); + let first_confirmed_funding_txo = funding_info.0; + Self::from_impl(ChannelMonitorImpl { latest_update_id: 0, commitment_transaction_number_obscure_factor, @@ -1411,6 +1415,7 @@ impl ChannelMonitor { holder_revocation_basepoint, channel_id, funding_info, + first_confirmed_funding_txo, current_counterparty_commitment_txid: None, prev_counterparty_commitment_txid: None, @@ -5042,6 +5047,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut channel_id = None; let mut holder_pays_commitment_tx_fee = None; let mut payment_preimages_with_info: Option> = None; + let mut first_confirmed_funding_txo = RequiredWrapper(None); read_tlv_fields!(reader, { (1, funding_spend_confirmed, option), (3, htlcs_resolved_on_chain, optional_vec), @@ -5056,6 +5062,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (21, balances_empty_height, option), (23, holder_pays_commitment_tx_fee, option), (25, payment_preimages_with_info, option), + (27, first_confirmed_funding_txo, (default_value, funding_info.0)), }); if let Some(payment_preimages_with_info) = payment_preimages_with_info { if payment_preimages_with_info.len() != payment_preimages.len() { @@ -5108,6 +5115,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP holder_revocation_basepoint, channel_id: channel_id.unwrap_or(ChannelId::v1_from_funding_outpoint(outpoint)), funding_info, + first_confirmed_funding_txo: first_confirmed_funding_txo.0.unwrap(), current_counterparty_commitment_txid, prev_counterparty_commitment_txid, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 1d64a396bfa..e76cc0deb1d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1965,6 +1965,8 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner()); let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id); monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters); + // TODO(RBF): When implementing RBF, the funding_txo passed here must only update + // ChannelMonitorImp::first_confirmed_funding_txo during channel establishment, not splicing let channel_monitor = ChannelMonitor::new(context.secp_ctx.clone(), monitor_signer, shutdown_script, context.get_holder_selected_contest_delay(), &context.destination_script, (funding_txo, funding_txo_script), From 9905e6062fb41fac9f8a49e696077aacd03ed070 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 27 Jan 2025 18:17:46 -0600 Subject: [PATCH 5/7] Update WatchtowerPersister to use channel_id The WatchtowerPersister test utility keys its maps using the channel's funding_outpoint. Since this is being removed from the Persist trait in the next commit, update the maps to be keyed by ChannelId instead. This is fine for testing where there isn't any previously persisted data. --- lightning/src/ln/functional_tests.rs | 5 ++--- lightning/src/util/test_utils.rs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index bb464389a61..34bc05b6d22 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2773,8 +2773,7 @@ fn do_test_forming_justice_tx_from_monitor_updates(broadcast_initial_commitment: let node_cfgs = create_node_cfgs_with_persisters(2, &chanmon_cfgs, persisters.iter().collect()); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let (_, _, channel_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1); - let funding_txo = OutPoint { txid: funding_tx.compute_txid(), index: 0 }; + let (_, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1); if !broadcast_initial_commitment { // Send a payment to move the channel forward @@ -2790,7 +2789,7 @@ fn do_test_forming_justice_tx_from_monitor_updates(broadcast_initial_commitment: // Send another payment, now revoking the previous commitment tx send_payment(&nodes[0], &vec!(&nodes[1])[..], 5_000_000); - let justice_tx = persisters[1].justice_tx(funding_txo, &revoked_commitment_tx.compute_txid()).unwrap(); + let justice_tx = persisters[1].justice_tx(channel_id, &revoked_commitment_tx.compute_txid()).unwrap(); check_spends!(justice_tx, revoked_commitment_tx); mine_transactions(&nodes[1], &[revoked_commitment_tx, &justice_tx]); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 7ebbf1c5734..dbeb41f9da8 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -533,10 +533,10 @@ pub(crate) struct WatchtowerPersister { /// ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTxInfo. We'll store the justice tx /// amount, and commitment number so we can build the justice tx after our counterparty /// revokes it. - unsigned_justice_tx_data: Mutex>>, + unsigned_justice_tx_data: Mutex>>, /// After receiving a revoke_and_ack for a commitment number, we'll form and store the justice /// tx which would be used to provide a watchtower with the data it needs. - watchtower_state: Mutex>>, + watchtower_state: Mutex>>, destination_script: ScriptBuf, } @@ -556,12 +556,12 @@ impl WatchtowerPersister { #[cfg(test)] pub(crate) fn justice_tx( - &self, funding_txo: OutPoint, commitment_txid: &Txid, + &self, channel_id: ChannelId, commitment_txid: &Txid, ) -> Option { self.watchtower_state .lock() .unwrap() - .get(&funding_txo) + .get(&channel_id) .unwrap() .get(commitment_txid) .cloned() @@ -596,13 +596,13 @@ impl Persist for WatchtowerPers .unsigned_justice_tx_data .lock() .unwrap() - .insert(funding_txo, VecDeque::new()) + .insert(data.channel_id(), VecDeque::new()) .is_none()); assert!(self .watchtower_state .lock() .unwrap() - .insert(funding_txo, new_hash_map()) + .insert(data.channel_id(), new_hash_map()) .is_none()); let initial_counterparty_commitment_tx = @@ -613,7 +613,7 @@ impl Persist for WatchtowerPers self.unsigned_justice_tx_data .lock() .unwrap() - .get_mut(&funding_txo) + .get_mut(&data.channel_id()) .unwrap() .push_back(justice_data); } @@ -632,7 +632,7 @@ impl Persist for WatchtowerPers .into_iter() .filter_map(|commitment_tx| self.form_justice_data_from_commitment(&commitment_tx)); let mut channels_justice_txs = self.unsigned_justice_tx_data.lock().unwrap(); - let channel_state = channels_justice_txs.get_mut(&funding_txo).unwrap(); + let channel_state = channels_justice_txs.get_mut(&data.channel_id()).unwrap(); channel_state.extend(justice_datas); while let Some(JusticeTxData { justice_tx, value, commitment_number }) = @@ -651,7 +651,7 @@ impl Persist for WatchtowerPers .watchtower_state .lock() .unwrap() - .get_mut(&funding_txo) + .get_mut(&data.channel_id()) .unwrap() .insert(commitment_txid, signed_justice_tx); assert!(dup.is_none()); From b8a1f84b5bc95879b150f6dace6f51ed863e7edb Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 28 Jan 2025 16:57:45 -0600 Subject: [PATCH 6/7] Support persisting ChannelMonitors after splicing ChannelMonitors are persisted using the corresponding channel's funding outpoint for the key. This is fine for v1 channels since the funding output will never change. However, this is not the case for v2 channels since a splice will result in a new funding outpoint. Instead, use the channel id as the persistence key for v2 channels since this is fixed. For v1 channels, continue to use the funding outpoint for backwards compatibility. Any v1 channel upgraded to a v2 channel via splicing will continue to use the original funding outpoint as the persistence key. --- fuzz/src/utils/test_persister.rs | 9 +-- lightning-persister/src/fs_store.rs | 24 ++----- lightning/src/chain/chainmonitor.rs | 31 ++++----- lightning/src/chain/channelmonitor.rs | 21 ++++++ lightning/src/util/persist.rs | 92 +++++++++------------------ lightning/src/util/test_utils.rs | 32 +++++----- 6 files changed, 93 insertions(+), 116 deletions(-) diff --git a/fuzz/src/utils/test_persister.rs b/fuzz/src/utils/test_persister.rs index 7de3cc6bebb..5838a961c0f 100644 --- a/fuzz/src/utils/test_persister.rs +++ b/fuzz/src/utils/test_persister.rs @@ -1,6 +1,6 @@ use lightning::chain; -use lightning::chain::transaction::OutPoint; use lightning::chain::{chainmonitor, channelmonitor}; +use lightning::util::persist::MonitorName; use lightning::util::test_channel_signer::TestChannelSigner; use std::sync::Mutex; @@ -10,17 +10,18 @@ pub struct TestPersister { } impl chainmonitor::Persist for TestPersister { fn persist_new_channel( - &self, _funding_txo: OutPoint, _data: &channelmonitor::ChannelMonitor, + &self, _monitor_name: MonitorName, + _data: &channelmonitor::ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { self.update_ret.lock().unwrap().clone() } fn update_persisted_channel( - &self, _funding_txo: OutPoint, _update: Option<&channelmonitor::ChannelMonitorUpdate>, + &self, _monitor_name: MonitorName, _update: Option<&channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { self.update_ret.lock().unwrap().clone() } - fn archive_persisted_channel(&self, _: OutPoint) {} + fn archive_persisted_channel(&self, _monitor_name: MonitorName) {} } diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs index 487e1a7ac14..e396b1fcec7 100644 --- a/lightning-persister/src/fs_store.rs +++ b/lightning-persister/src/fs_store.rs @@ -498,17 +498,13 @@ mod tests { do_read_write_remove_list_persist, do_test_data_migration, do_test_store, }; - use bitcoin::Txid; - use lightning::chain::chainmonitor::Persist; - use lightning::chain::transaction::OutPoint; use lightning::chain::ChannelMonitorUpdateStatus; use lightning::check_closed_event; use lightning::events::{ClosureReason, MessageSendEventsProvider}; use lightning::ln::functional_test_utils::*; use lightning::util::persist::read_channel_monitors; use lightning::util::test_utils; - use std::str::FromStr; impl Drop for FilesystemStore { fn drop(&mut self) { @@ -622,14 +618,8 @@ mod tests { perms.set_readonly(true); fs::set_permissions(path, perms).unwrap(); - let test_txo = OutPoint { - txid: Txid::from_str( - "8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be", - ) - .unwrap(), - index: 0, - }; - match store.persist_new_channel(test_txo, &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"), } @@ -676,14 +666,8 @@ mod tests { // handle, hence why the test is Windows-only. let store = FilesystemStore::new(":<>/".into()); - let test_txo = OutPoint { - txid: Txid::from_str( - "8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be", - ) - .unwrap(), - index: 0, - }; - match store.persist_new_channel(test_txo, &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 120420894e9..eec56ac3647 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -36,6 +36,7 @@ use crate::sign::ecdsa::EcdsaChannelSigner; use crate::events::{self, Event, EventHandler, ReplayEvent}; use crate::util::logger::{Logger, WithContext}; use crate::util::errors::APIError; +use crate::util::persist::MonitorName; use crate::util::wakers::{Future, Notifier}; use crate::ln::channel_state::ChannelDetails; @@ -102,11 +103,13 @@ use bitcoin::secp256k1::PublicKey; /// [`TrustedCommitmentTransaction::build_to_local_justice_tx`]: crate::ln::chan_utils::TrustedCommitmentTransaction::build_to_local_justice_tx 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. + /// called by [`ChannelManager`] for new channels, or may be called directly, e.g. on startup, + /// with the `monitor_name` returned by [`ChannelMonitor::persistence_key`]. /// - /// The data can be stored any way you want, but the identifier provided by LDK is the - /// channel's outpoint (and it is up to you to maintain a correct mapping between the outpoint - /// and 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 (i.e., calls to `update_persisted_channel` with + /// the same `monitor_name` must be applied to or overwrite this 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`] @@ -117,7 +120,7 @@ pub trait Persist { /// /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager /// [`Writeable::write`]: crate::util::ser::Writeable::write - fn persist_new_channel(&self, channel_funding_outpoint: OutPoint, 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. @@ -156,7 +159,7 @@ pub trait Persist { /// [`ChannelMonitorUpdateStatus`] for requirements when returning errors. /// /// [`Writeable::write`]: crate::util::ser::Writeable::write - fn update_persisted_channel(&self, channel_funding_outpoint: OutPoint, 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 @@ -168,7 +171,7 @@ pub trait Persist { /// the archive process. Additionally, because the archive operation could be retried on /// restart, this method must in that case be idempotent, ensuring it can handle scenarios where /// the monitor already exists in the archive. - fn archive_persisted_channel(&self, channel_funding_outpoint: OutPoint); + fn archive_persisted_channel(&self, monitor_name: MonitorName); } struct MonitorHolder { @@ -342,8 +345,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(); - let funding_txo = monitor.get_funding_txo(); - match self.persister.update_persisted_channel(funding_txo, 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 +644,7 @@ where C::Target: chain::Filter, have_monitors_to_prune = true; } if needs_persistence { - self.persister.update_persisted_channel(monitor_holder.monitor.get_funding_txo(), None, &monitor_holder.monitor); + self.persister.update_persisted_channel(monitor_holder.monitor.persistence_key(), None, &monitor_holder.monitor); } } if have_monitors_to_prune { @@ -655,7 +657,7 @@ where C::Target: chain::Filter, "Archiving fully resolved ChannelMonitor for channel ID {}", channel_id ); - self.persister.archive_persisted_channel(monitor_holder.monitor.get_funding_txo()); + self.persister.archive_persisted_channel(monitor_holder.monitor.persistence_key()); false } else { true @@ -769,7 +771,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.get_funding_txo(), &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)); @@ -825,7 +827,6 @@ where C::Target: chain::Filter, let update_res = monitor.update_monitor(update, &self.broadcaster, &self.fee_estimator, &self.logger); let update_id = update.update_id; - let funding_txo = monitor.get_funding_txo(); let persist_res = if update_res.is_err() { // Even if updating the monitor returns an error, the monitor's state will // still be changed. Therefore, we should persist the updated monitor despite the error. @@ -833,9 +834,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(funding_txo, None, monitor) + self.persister.update_persisted_channel(monitor.persistence_key(), None, monitor) } else { - self.persister.update_persisted_channel(funding_txo, 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 3189856862a..55d168542ac 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -48,6 +48,7 @@ use crate::chain::onchaintx::{ClaimEvent, FeerateStrategy, OnchainTxHandler}; use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput}; use crate::chain::Filter; use crate::util::logger::{Logger, Record}; +use crate::util::persist::MonitorName; use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, UpgradableRequired, Writer, Writeable, U48}; use crate::util::byte_utils; use crate::events::{ClosureReason, Event, EventHandler, ReplayEvent}; @@ -1465,6 +1466,26 @@ impl ChannelMonitor { }) } + /// 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, 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 { + let inner = self.inner.lock().unwrap(); + let funding_outpoint = inner.first_confirmed_funding_txo; + let channel_id = inner.channel_id; + if ChannelId::v1_from_funding_outpoint(funding_outpoint) == channel_id { + MonitorName::from(funding_outpoint) + } else { + MonitorName::from(channel_id) + } + } + #[cfg(test)] fn provide_secret(&self, idx: u64, secret: [u8; 32]) -> Result<(), &'static str> { self.inner.lock().unwrap().provide_secret(idx, secret) diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs index 65fbe10d863..05f0013f0ee 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 = MonitorName::from(funding_txo); match self.write( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, @@ -276,10 +275,9 @@ impl Persist, + &self, monitor_name: MonitorName, _update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { - let monitor_name = MonitorName::from(funding_txo); match self.write( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, @@ -291,8 +289,7 @@ impl Persist::EcdsaSigner>)>::read( &mut io::Cursor::new(kv_store.read( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, @@ -359,14 +341,14 @@ where (&*entropy_source, &*signer_provider), ) { Ok((block_hash, channel_monitor)) => { - if channel_monitor.get_funding_txo().txid != txid - || channel_monitor.get_funding_txo().index != index - { + let monitor_name = MonitorName::from_str(&stored_key)?; + if channel_monitor.persistence_key() != monitor_name { return Err(io::Error::new( io::ErrorKind::InvalidData, "ChannelMonitor was stored under the wrong key", )); } + res.push((block_hash, channel_monitor)); }, Err(_) => { @@ -407,12 +389,13 @@ where /// - [`Persist::update_persisted_channel`], which persists only a [`ChannelMonitorUpdate`] /// /// Whole [`ChannelMonitor`]s are stored in the [`CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE`], -/// using the familiar encoding of an [`OutPoint`] (for example, `[SOME-64-CHAR-HEX-STRING]_1`). +/// using the familiar encoding of an [`OutPoint`] (e.g., `[SOME-64-CHAR-HEX-STRING]_1`) for v1 +/// channels or a [`ChannelId`] (e.g., `[SOME-64-CHAR-HEX-STRING]`) for v2 channels. /// /// Each [`ChannelMonitorUpdate`] is stored in a dynamic secondary namespace, as follows: /// /// - primary namespace: [`CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE`] -/// - secondary namespace: [the monitor's encoded outpoint name] +/// - secondary namespace: [the monitor's encoded outpoint or channel id name] /// /// Under that secondary namespace, each update is stored with a number string, like `21`, which /// represents its `update_id` value. @@ -551,8 +534,8 @@ where /// [`io::ErrorKind::NotFound`] variant correctly. For more information, please see the /// documentation for [`MonitorUpdatingPersister`]. /// - /// For `monitor_key`, channel storage keys be the channel's transaction ID and index, or - /// [`OutPoint`], with an underscore `_` between them. For example, given: + /// For `monitor_key`, channel storage keys can be the channel's funding [`OutPoint`], with an + /// underscore `_` between txid and index for v1 channels. For example, given: /// /// - Transaction ID: `deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef` /// - Index: `1` @@ -560,6 +543,8 @@ where /// The correct `monitor_key` would be: /// `deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_1` /// + /// For v2 channels, the hex-encoded [`ChannelId`] is used directly for `monitor_key` instead. + /// /// Loading a large number of monitors will be faster if done in parallel. You can use this /// function to accomplish this. Take care to limit the number of parallel readers. pub fn read_channel_monitor_with_updates( @@ -605,15 +590,6 @@ where &self, monitor_name: &MonitorName, monitor_key: &str, ) -> Result<(BlockHash, ChannelMonitor<::EcdsaSigner>), io::Error> { - let outpoint = match monitor_name { - MonitorName::V1Channel(outpoint) => outpoint, - MonitorName::V2Channel(_) => { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "Invalid outpoint in stored key", - )) - }, - }; let mut monitor_cursor = io::Cursor::new(self.kv_store.read( CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE, CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE, @@ -628,9 +604,7 @@ where (&*self.entropy_source, &*self.signer_provider), ) { Ok((blockhash, channel_monitor)) => { - if channel_monitor.get_funding_txo().txid != outpoint.txid - || channel_monitor.get_funding_txo().index != outpoint.index - { + if channel_monitor.persistence_key() != *monitor_name { log_error!( self.logger, "ChannelMonitor {} was stored under the wrong key!", @@ -732,10 +706,9 @@ where /// Persists a new channel. This means writing the entire monitor to the /// parametrized [`KVStore`]. fn persist_new_channel( - &self, funding_txo: OutPoint, monitor: &ChannelMonitor, + &self, monitor_name: MonitorName, monitor: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { // Determine the proper key for this monitor - let monitor_name = MonitorName::from(funding_txo); let monitor_key = monitor_name.to_string(); // Serialize and write the new monitor let mut monitor_bytes = Vec::with_capacity( @@ -774,7 +747,7 @@ where /// `update` is `None`. /// - The update is at [`u64::MAX`], indicating an update generated by pre-0.1 LDK. fn update_persisted_channel( - &self, funding_txo: OutPoint, update: Option<&ChannelMonitorUpdate>, + &self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX; @@ -782,7 +755,6 @@ where let persist_update = update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID && update.update_id % self.maximum_pending_updates != 0; if persist_update { - let monitor_name = MonitorName::from(funding_txo); let monitor_key = monitor_name.to_string(); let update_name = UpdateName::from(update.update_id); match self.kv_store.write( @@ -805,19 +777,18 @@ where }, } } else { - let monitor_name = MonitorName::from(funding_txo); - 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() { LEGACY_CLOSED_CHANNEL_UPDATE_ID => { + let monitor_key = monitor_name.to_string(); self.read_monitor(&monitor_name, &monitor_key).ok() }, _ => None, }; // 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(funding_txo, monitor); + let monitor_update_status = self.persist_new_channel(monitor_name, monitor); if let chain::ChannelMonitorUpdateStatus::Completed = monitor_update_status { let channel_closed_legacy = @@ -848,12 +819,11 @@ where } } else { // There is no update given, so we must persist a new monitor. - self.persist_new_channel(funding_txo, monitor) + self.persist_new_channel(monitor_name, monitor) } } - fn archive_persisted_channel(&self, funding_txo: OutPoint) { - let monitor_name = MonitorName::from(funding_txo); + fn archive_persisted_channel(&self, monitor_name: MonitorName) { let monitor_key = monitor_name.to_string(); let monitor = match self.read_channel_monitor_with_updates(&monitor_key) { Ok((_block_hash, monitor)) => monitor, @@ -915,16 +885,15 @@ where /// in functions that store or retrieve [`ChannelMonitor`] snapshots. /// It provides a consistent way to generate a unique key for channel /// monitors based on the channel's funding [`OutPoint`] for v1 channels or -/// [`ChannelId`] for v2 channels. The former uses the channel's funding -/// outpoint for historical reasons. The latter uses the channel's id because -/// the funding outpoint may change during splicing. +/// [`ChannelId`] for v2 channels. Use [`ChannelMonitor::persistence_key`] to +/// obtain the correct `MonitorName`. /// /// While users of the Lightning Dev Kit library generally won't need /// to interact with [`MonitorName`] directly, it can be useful for: /// - Custom persistence implementations /// - Debugging or logging channel monitor operations /// - Extending the functionality of the `MonitorUpdatingPersister` -// +/// /// # Examples /// /// ``` @@ -953,7 +922,7 @@ where /// // Using MonitorName to generate a storage key /// let storage_key = format!("channel_monitors/{}", monitor_name); /// ``` -#[derive(Debug)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] pub enum MonitorName { /// The outpoint of the channel's funding transaction. V1Channel(OutPoint), @@ -1378,10 +1347,6 @@ mod tests { let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap(); let cmu_map = nodes[1].chain_monitor.monitor_updates.lock().unwrap(); let cmu = &cmu_map.get(&added_monitors[0].1.channel_id()).unwrap()[0]; - let txid = - Txid::from_str("8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be") - .unwrap(); - let test_txo = OutPoint { txid, index: 0 }; let ro_persister = MonitorUpdatingPersister { kv_store: &TestStore::new(true), @@ -1392,7 +1357,8 @@ mod tests { broadcaster: node_cfgs[0].tx_broadcaster, fee_estimator: node_cfgs[0].fee_estimator, }; - match ro_persister.persist_new_channel(test_txo, &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 }, @@ -1403,7 +1369,11 @@ mod tests { panic!("Returned InProgress when shouldn't have") }, } - match ro_persister.update_persisted_channel(test_txo, 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 dbeb41f9da8..f17185ea5ad 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -51,7 +51,7 @@ use crate::sync::RwLock; use crate::types::features::{ChannelFeatures, InitFeatures, NodeFeatures}; use crate::util::config::UserConfig; use crate::util::logger::{Logger, Record}; -use crate::util::persist::KVStore; +use crate::util::persist::{KVStore, MonitorName}; use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer}; use crate::util::test_channel_signer::{EnforcementState, TestChannelSigner}; @@ -588,9 +588,9 @@ impl WatchtowerPersister { #[cfg(test)] impl Persist for WatchtowerPersister { fn persist_new_channel( - &self, funding_txo: OutPoint, data: &ChannelMonitor, + &self, monitor_name: MonitorName, data: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { - let res = self.persister.persist_new_channel(funding_txo, data); + let res = self.persister.persist_new_channel(monitor_name, data); assert!(self .unsigned_justice_tx_data @@ -621,10 +621,10 @@ impl Persist for WatchtowerPers } fn update_persisted_channel( - &self, funding_txo: OutPoint, update: Option<&ChannelMonitorUpdate>, + &self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>, data: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { - let res = self.persister.update_persisted_channel(funding_txo, 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); @@ -664,10 +664,10 @@ impl Persist for WatchtowerPers res } - fn archive_persisted_channel(&self, funding_txo: OutPoint) { + fn archive_persisted_channel(&self, monitor_name: MonitorName) { >::archive_persisted_channel( &self.persister, - funding_txo, + monitor_name, ); } } @@ -678,10 +678,10 @@ pub struct TestPersister { pub update_rets: Mutex>, /// When we get an update_persisted_channel call *with* a ChannelMonitorUpdate, we insert the /// [`ChannelMonitor::get_latest_update_id`] here. - pub offchain_monitor_updates: Mutex>>, + pub offchain_monitor_updates: Mutex>>, /// When we get an update_persisted_channel call with no ChannelMonitorUpdate, we insert the /// monitor's funding outpoint here. - pub chain_sync_monitor_persistences: Mutex>, + pub chain_sync_monitor_persistences: Mutex>, } impl TestPersister { pub fn new() -> Self { @@ -698,7 +698,7 @@ impl TestPersister { } impl Persist for TestPersister { fn persist_new_channel( - &self, _funding_txo: OutPoint, _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; @@ -707,7 +707,7 @@ impl Persist for TestPersister } fn update_persisted_channel( - &self, funding_txo: OutPoint, update: Option<&ChannelMonitorUpdate>, + &self, monitor_name: MonitorName, update: Option<&ChannelMonitorUpdate>, _data: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { let mut ret = chain::ChannelMonitorUpdateStatus::Completed; @@ -719,19 +719,19 @@ impl Persist for TestPersister self.offchain_monitor_updates .lock() .unwrap() - .entry(funding_txo) + .entry(monitor_name) .or_insert(new_hash_set()) .insert(update.update_id); } else { - self.chain_sync_monitor_persistences.lock().unwrap().push_back(funding_txo); + self.chain_sync_monitor_persistences.lock().unwrap().push_back(monitor_name); } ret } - fn archive_persisted_channel(&self, funding_txo: OutPoint) { + fn archive_persisted_channel(&self, monitor_name: MonitorName) { // remove the channel from the offchain_monitor_updates and chain_sync_monitor_persistences. - self.offchain_monitor_updates.lock().unwrap().remove(&funding_txo); - self.chain_sync_monitor_persistences.lock().unwrap().retain(|x| x != &funding_txo); + self.offchain_monitor_updates.lock().unwrap().remove(&monitor_name); + self.chain_sync_monitor_persistences.lock().unwrap().retain(|x| x != &monitor_name); } } From 372e7f0c2dea7ffcf37b9fa8a392735702ec8887 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 7 Feb 2025 11:05:52 -0600 Subject: [PATCH 7/7] Remove From implementations for MonitorName Now that MonitorName can be represented by either an OutPoint for V1 established channels or a ChannelId for V2 established channels, having a From implementation converting from those types may be misleading. Remove these implementations since the enum variants are at least explicit about what version channels the MonitorName is applicable, even if still possible to create a MonitorName incorrectly. --- lightning/src/chain/channelmonitor.rs | 4 +-- lightning/src/util/persist.rs | 39 ++++++--------------------- 2 files changed, 10 insertions(+), 33 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 55d168542ac..8bbe8f51103 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1480,9 +1480,9 @@ impl ChannelMonitor { let funding_outpoint = inner.first_confirmed_funding_txo; let channel_id = inner.channel_id; if ChannelId::v1_from_funding_outpoint(funding_outpoint) == channel_id { - MonitorName::from(funding_outpoint) + MonitorName::V1Channel(funding_outpoint) } else { - MonitorName::from(channel_id) + MonitorName::V2Channel(channel_id) } } diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs index 05f0013f0ee..5d9f8b7a9dd 100644 --- a/lightning/src/util/persist.rs +++ b/lightning/src/util/persist.rs @@ -911,12 +911,12 @@ where /// txid: Txid::from_str("deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef").unwrap(), /// index: 1, /// }; -/// let monitor_name = MonitorName::from(outpoint); +/// let monitor_name = MonitorName::V1Channel(outpoint); /// assert_eq!(&monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_1"); /// /// // v2 channel /// let channel_id = ChannelId(<[u8; 32]>::from_hex("deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef").unwrap()); -/// let monitor_name = MonitorName::from(channel_id); +/// let monitor_name = MonitorName::V2Channel(channel_id); /// assert_eq!(&monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); /// /// // Using MonitorName to generate a storage key @@ -959,26 +959,6 @@ impl MonitorName { } } -impl From for MonitorName { - /// Creates a `MonitorName` from an `OutPoint`. - /// - /// This is typically used when you need to generate a storage key or identifier - /// for a new or existing channel monitor for a v1 channel. - fn from(outpoint: OutPoint) -> Self { - MonitorName::V1Channel(outpoint) - } -} - -impl From for MonitorName { - /// Creates a `MonitorName` from a `ChannelId`. - /// - /// This is typically used when you need to generate a storage key or identifier - /// for a new or existing channel monitor for a v2 channel. - fn from(channel_id: ChannelId) -> Self { - MonitorName::V2Channel(channel_id) - } -} - impl core::fmt::Display for MonitorName { fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { match self { @@ -1108,7 +1088,7 @@ mod tests { #[test] fn creates_monitor_from_outpoint() { - let monitor_name = MonitorName::from(OutPoint { + let monitor_name = MonitorName::V1Channel(OutPoint { txid: Txid::from_str( "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", ) @@ -1119,9 +1099,8 @@ mod tests { &monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_1" ); - assert!(matches!(monitor_name, MonitorName::V1Channel(_))); - let monitor_name = MonitorName::from(OutPoint { + let monitor_name = MonitorName::V1Channel(OutPoint { txid: Txid::from_str( "f33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeef", ) @@ -1132,12 +1111,11 @@ mod tests { &monitor_name.to_string(), "f33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeeff33dbeef_65535" ); - assert!(matches!(monitor_name, MonitorName::V1Channel(_))); } #[test] fn creates_monitor_from_channel_id() { - let monitor_name = MonitorName::from(ChannelId( + let monitor_name = MonitorName::V2Channel(ChannelId( <[u8; 32]>::from_hex( "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", ) @@ -1147,7 +1125,6 @@ mod tests { &monitor_name.to_string(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" ); - assert!(matches!(monitor_name, MonitorName::V2Channel(_))); } #[test] @@ -1234,7 +1211,7 @@ mod tests { // check that when we read it, we got the right update id assert_eq!(mon.get_latest_update_id(), $expected_update_id); - let monitor_name = MonitorName::from(mon.get_funding_txo()); + let monitor_name = mon.persistence_key(); assert_eq!( persister_0 .kv_store @@ -1253,7 +1230,7 @@ mod tests { assert_eq!(persisted_chan_data_1.len(), 1); for (_, mon) in persisted_chan_data_1.iter() { assert_eq!(mon.get_latest_update_id(), $expected_update_id); - let monitor_name = MonitorName::from(mon.get_funding_txo()); + let monitor_name = mon.persistence_key(); assert_eq!( persister_1 .kv_store @@ -1449,7 +1426,7 @@ mod tests { // Get the monitor and make a fake stale update at update_id=1 (lowest height of an update possible) let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates().unwrap(); let (_, monitor) = &persisted_chan_data[0]; - let monitor_name = MonitorName::from(monitor.get_funding_txo()); + let monitor_name = monitor.persistence_key(); persister_0 .kv_store .write(