Support persisting `ChannelMonitor`s after splicing #3569

jkczyz · 2025-01-28T23:26:17Z

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.

The funding outpoint is thus removed from the Persist methods in favor of calling ChannelMonitor::persistence_key.

Based on #3554.

Fixes #3325.

tnull

Did a pass out of interest, LGTM.

CI is failing as lightning-persister tests need some adjustments though.

jkczyz · 2025-01-29T15:45:11Z

lightning/src/util/persist.rs

+	fn archive_persisted_channel(&self, monitor: &ChannelMonitor<ChannelSigner>) {
+		let monitor_name = monitor.persistence_key();
 		let monitor = match self.read(


Wondering if we should take the MonitorName instead here given Monitor is re-read below? Reading prevents us from writing a different monitor. Could that ever be the case?

By the time we archive, the monitor taken and the one being read should be the same since there can't be any updates. I'd favor taking the MonitorName instead.

Updated accordingly.

jkczyz · 2025-01-29T15:46:12Z

lightning/src/util/persist.rs

+	fn archive_persisted_channel(&self, monitor: &ChannelMonitor<ChannelSigner>) {
+		let monitor_name = monitor.persistence_key();
 		let monitor_key = monitor_name.as_str().to_string();
 		let monitor = match self.read_channel_monitor_with_updates(monitor_key) {


Similarly here.

codecov · 2025-01-29T15:49:36Z

Codecov Report

Attention: Patch coverage is 78.75000% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.99%. Comparing base (717db82) to head (6267610).
Report is 55 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/persist.rs	72.56%	20 Missing and 11 partials ⚠️
lightning/src/util/test_utils.rs	90.00%	2 Missing ⚠️
lightning/src/chain/channelmonitor.rs	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3569      +/-   ##
==========================================
+ Coverage   88.49%   88.99%   +0.49%     
==========================================
  Files         149      149              
  Lines      114709   119147    +4438     
  Branches   114709   119147    +4438     
==========================================
+ Hits       101517   106029    +4512     
+ Misses      10691    10656      -35     
+ Partials     2501     2462      -39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wpaulino · 2025-01-30T19:52:48Z

lightning/src/chain/channelmonitor.rs

@@ -1398,6 +1400,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		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 original_funding_txo = funding_info.0;


For v2 channels, since monitors are created before transaction confirmation, this may not point to the one that actually confirmed if it changes due to an RBF. We should probably clarify what we're tracking here. Ultimately, I do think it should track the first confirmed outpoint of the channel.

Hmm... right, this will affect splicing a v1 channel then. IIUC, a v1 channel can be spliced but it will keep its channel id, which was derived from the original funding outpoint. Thus, we need the original_funding_txo to determine if the ChannelMonitor is for a v1 channel and continue to use it for the persistence key.

Seems we need to pass in a separate first_confirmed_funding_txo to ChannelMonitor::new as an Option<OutPoint>. For v1 or v2 channel establishment, we would pass None and set on ChannelMonitor as first_confirmed_funding_txo.unwrap_or(funding_info.0). An RBF attempt on v2 channel establishment would need to reset first_confirmed_funding_txo. And a splice attempt would need to pass in the appropriate Some(first_confirmed_funding_txo).

Does that check out? Presumably, the "scoped" work on ChannelMonitor for outstanding splices/RBFs would affect the exact mechanism for this.

We can keep things as is for v1 channels since we can't RBF them, so original_funding_txo will always be the intended value. For v2 channels is where things change, and I think we could just mutate the existing original_funding_txo to point to the newly confirmed one if a funding RBF does happen. We just need to rename the field to first_confirmed_funding_txo.

Right, make sense. Presumably the code will be refactored a bit when adding RBF support to allow updating or not updating depending on whether the RBF is for channel establishment (i.e., for v2 only) or splicing (i.e. for either v1 or v2).

wpaulino · 2025-01-30T20:13:15Z

lightning/src/util/persist.rs

+	fn archive_persisted_channel(&self, monitor: &ChannelMonitor<ChannelSigner>) {
+		let monitor_name = monitor.persistence_key();
 		let monitor = match self.read(


By the time we archive, the monitor taken and the one being read should be the same since there can't be any updates. I'd favor taking the MonitorName instead.

wpaulino

LGTM after squash. I'm not sure how we want to track the changes needed (as a TODO or issue?) for first_confirmed_funding_txo once we implement RBF support, they could easily be forgotten.

jkczyz · 2025-01-31T17:32:47Z

LGTM after squash. I'm not sure how we want to track the changes needed (as a TODO or issue?) for first_confirmed_funding_txo once we implement RBF support, they could easily be forgotten.

Add a TODO at the ChannelMonitor::new call site and squashed.

jkczyz · 2025-01-31T22:16:37Z

Rebased on latest changes to #3554.

TheBlueMatt · 2025-02-04T16:57:46Z

lightning/src/util/persist.rs

+	///
+	/// This is useful when you have a `MonitorName` (perhaps retrieved from storage)
+	/// and need to reconstruct the original data it represents.
+	fn try_from(name: &str) -> Result<Self, io::Error> {


Can you explain a bit more why we want to add this - what is the anticipated use-case? That someone would do, like, list_keys then iterate the keys, calling this, and compare against some desired monitor (instead of calculating the two possible keys and doing a lookup by the known key)?

I'm not familiar with the exact use case, but this was exposed by #3376. See MonitorName docs for some examples. FWIW, the latest push removes this now that MonitorName and MonitorNameSource are merged.

TheBlueMatt · 2025-02-04T17:01:41Z

lightning/src/util/persist.rs

 /// // Using MonitorName to generate a storage key
 /// let storage_key = format!("channel_monitors/{}", monitor_name.as_str());
 /// ```
-#[derive(Debug)]
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub struct MonitorName(String);


It would be nice to make MonitorName's internals just be MonitorNameSource, allocating the string when we actually go to use it (and ideally with the ability to write to a io::Write so that we can append it to the path in FilesystemPersister without allocating the string up front, since this is a very nontrivial portion of the total allocations I see on my node).

Right, consolidated it all as a MonitorName enum. Though FilesystemStore implements KVStore, which Persist has a blanket implementation for, so the conversion is done there (i.e., KVStore::write takes each path component as a &str). Therefore, I don't think io::Write is relevant here.

TheBlueMatt · 2025-02-04T17:02:34Z

lightning/src/chain/chainmonitor.rs

@@ -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, channel_funding_outpoint: OutPoint, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
+	fn persist_new_channel(&self, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;


Hmm, its a bit awkward to remove the key here but keep it in archive_persisted_channel. I guess maybe I'd prefer we keep the key since I hadn't considered archive_persisted_channel's semantics, sorry. Not really a huge deal either way, tho, if you feel strongly otherwise.

Makes sense. Added the parameter back to the other methods.

jkczyz · 2025-02-05T15:30:45Z

Added a couple fixups to remove some extra string allocations that I noticed. Please see if anything else could be improved. MonitorUpdatingPersister's methods probably could use a closer look.

TheBlueMatt

LGTM, feel free to squash.

TheBlueMatt · 2025-02-06T00:34:58Z

lightning/src/chain/chainmonitor.rs

-	/// 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


nit: I mean they don't have to use monitor_name, but also is it worth mentioning that the monitor_name here will match [ChannelMonitor::persistence_key]?

Updated the comment to reflect the relationship between monitor_name used here and in update_persisted_channel. Also, add reference to ChannelMonitor::persistence_key earlier in the docs.

jkczyz · 2025-02-06T19:40:00Z

In addition to the doc change, added a fix to a broken dual funding test introduced in another PR.

diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index fa903d388..eec56ac36 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -103,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<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.
+       /// 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, 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.
+       /// 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`]
diff --git a/lightning/src/ln/dual_funding_tests.rs b/lightning/src/ln/dual_funding_tests.rs
index 8cd47ad17..f5ba664b8 100644
--- a/lightning/src/ln/dual_funding_tests.rs
+++ b/lightning/src/ln/dual_funding_tests.rs
@@ -229,14 +229,14 @@ 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();

TheBlueMatt · 2025-02-06T21:24:09Z

lightning/src/util/persist.rs

-	type Error = io::Error;
-
-	/// Attempts to convert a `MonitorName` back into an `OutPoint`.
+impl From<OutPoint> for MonitorName {


nit: cant say I'm a fan of the blanket Froms here - it makes it seem like if you have a ChannelId for a channel you can get the single canonical MonitorName for it, which isn't true.

Ah, yeah, good point. Removed in a new commit. It can still be created incorrectly using the variant directly, but at least the variant names make it explicit and the docs state to use ChannelMonitor::persistence_key.

TheBlueMatt · 2025-02-07T00:44:50Z

rustfmt is sad:

Diff in /home/runner/work/rust-lightning/rust-lightning/lightning/src/ln/dual_funding_tests.rs at line 236:
 			.unwrap()
 			.get(&channel_id)
 			.unwrap();
-		nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(channel_id, 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();

An earlier commit broke test compilation under --cfg=dual_funding.

Instead repeating the MonitorName formatting when persisting monitors, DRY up the code to use MonitorName::from instead.

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.

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.

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.

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.

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.

jkczyz · 2025-02-07T17:13:16Z

rustfmt is sad:

Diff in /home/runner/work/rust-lightning/rust-lightning/lightning/src/ln/dual_funding_tests.rs at line 236:
 			.unwrap()
 			.get(&channel_id)
 			.unwrap();
-		nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(channel_id, 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();

Fixed.

jkczyz requested review from TheBlueMatt and wpaulino January 28, 2025 23:26

jkczyz added the weekly goal Someone wants to land this this week label Jan 28, 2025

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch from 001463b to fb7dcde Compare January 28, 2025 23:35

tnull reviewed Jan 29, 2025

View reviewed changes

jkczyz commented Jan 29, 2025

View reviewed changes

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch from f33c2af to 331c296 Compare January 29, 2025 16:21

wpaulino reviewed Jan 30, 2025

View reviewed changes

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch from 331c296 to 8edede0 Compare January 30, 2025 23:34

wpaulino reviewed Jan 31, 2025

View reviewed changes

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch 2 times, most recently from 30ae6bd to 260341b Compare January 31, 2025 17:32

wpaulino previously approved these changes Jan 31, 2025

View reviewed changes

jkczyz dismissed wpaulino’s stale review via 9e7be36 January 31, 2025 22:15

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch from 260341b to 9e7be36 Compare January 31, 2025 22:15

jkczyz assigned TheBlueMatt Jan 31, 2025

TheBlueMatt reviewed Feb 4, 2025

View reviewed changes

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch 2 times, most recently from 6267610 to 99fb6f1 Compare February 5, 2025 15:30

TheBlueMatt reviewed Feb 6, 2025

View reviewed changes

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch from 99fb6f1 to b6b7c20 Compare February 6, 2025 19:36

wpaulino previously approved these changes Feb 6, 2025

View reviewed changes

TheBlueMatt previously approved these changes Feb 6, 2025

View reviewed changes

Fix broken dual_funding_tests

d2564a9

An earlier commit broke test compilation under --cfg=dual_funding.

jkczyz added 6 commits February 7, 2025 10:49

Use MonitorName when persisting ChannelMonitors

220dfc9

Instead repeating the MonitorName formatting when persisting monitors, DRY up the code to use MonitorName::from instead.

jkczyz dismissed stale reviews from TheBlueMatt and wpaulino via 372e7f0 February 7, 2025 17:10

jkczyz force-pushed the 2025-01-v2-channel-monitor-persist branch from b6b7c20 to 372e7f0 Compare February 7, 2025 17:10

wpaulino approved these changes Feb 7, 2025

View reviewed changes

TheBlueMatt approved these changes Feb 7, 2025

View reviewed changes

TheBlueMatt merged commit 59739ef into lightningdevkit:main Feb 7, 2025
24 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support persisting `ChannelMonitor`s after splicing #3569

Support persisting `ChannelMonitor`s after splicing #3569

jkczyz commented Jan 28, 2025 •

edited

Loading

tnull left a comment

jkczyz Jan 29, 2025

wpaulino Jan 30, 2025

jkczyz Jan 30, 2025

jkczyz Jan 29, 2025

codecov bot commented Jan 29, 2025 •

edited

Loading

wpaulino Jan 30, 2025

jkczyz Jan 30, 2025

wpaulino Jan 30, 2025

jkczyz Jan 30, 2025

jkczyz Jan 30, 2025

wpaulino Jan 30, 2025

wpaulino left a comment

jkczyz commented Jan 31, 2025

jkczyz commented Jan 31, 2025

TheBlueMatt Feb 4, 2025

jkczyz Feb 4, 2025

TheBlueMatt Feb 4, 2025

jkczyz Feb 4, 2025

TheBlueMatt Feb 4, 2025

jkczyz Feb 4, 2025

jkczyz commented Feb 5, 2025

TheBlueMatt left a comment

TheBlueMatt Feb 6, 2025

jkczyz Feb 6, 2025

jkczyz commented Feb 6, 2025

TheBlueMatt Feb 6, 2025

jkczyz Feb 7, 2025

TheBlueMatt commented Feb 7, 2025

jkczyz commented Feb 7, 2025

Support persisting ChannelMonitors after splicing #3569

Support persisting ChannelMonitors after splicing #3569

Conversation

jkczyz commented Jan 28, 2025 • edited Loading

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino left a comment

Choose a reason for hiding this comment

jkczyz commented Jan 31, 2025

jkczyz commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Feb 5, 2025

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 7, 2025

jkczyz commented Feb 7, 2025

Support persisting `ChannelMonitor`s after splicing #3569

Support persisting `ChannelMonitor`s after splicing #3569

jkczyz commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading