Start tracking ChannelMonitors by channel ID in ChainMonitor and ChannelManager #3554

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

We no longer need the funding outpoints as we can fully rely on the channel IDs instead to track them. This acts as a breaking change, as we will no longer write the legacy version of in-flight monitor updates, preventing downgrades to versions that only supported such. Along the way, we clean up `handle_new_monitor_update` and `MonitorUpdateRegeneratedOnStartup` now that their `funding_txo` parameter is unused.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 94.70899% with 20 lines in your changes missing coverage. Please review.

Project coverage is 88.38%. Comparing base (3d2b4de) to head (1c7a0a1).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	83.56%	8 Missing and 4 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.36%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3554      +/-   ##
==========================================
- Coverage   88.39%   88.38%   -0.01%     
==========================================
  Files         149      149              
  Lines      113850   113953     +103     
  Branches   113850   113953     +103     
==========================================
+ Hits       100639   100720      +81     
- Misses      10695    10721      +26     
+ Partials     2516     2512       -4

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

jkczyz

Concept ACK

jkczyz · 2025-01-21T21:36:34Z

lightning/src/ln/channelmanager.rs

-				BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((funding_txo, _channel_id, update)) => {
+				BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((_funding_txo, channel_id, update)) => {
 					// The channel has already been closed, so no use bothering to care about the
 					// monitor updating completing.
-					let _ = self.chain_monitor.update_channel(funding_txo, &update);
+					let _ = self.chain_monitor.update_channel(channel_id, &update);


IIUC, this will never be a temporary channel id since we have a ChannelMonitor and therefore also a funded channel?

jkczyz · 2025-01-21T22:09:56Z

lightning/src/chain/chainmonitor.rs

-			(*outpoint, channel_id)
-		}).collect()
+	pub fn list_monitors(&self) -> Vec<ChannelId> {
+		self.monitors.read().unwrap().iter().map(|(channel_id, _)| *channel_id).collect()


Consider using keys().copied() instead of iter().map().

jkczyz · 2025-01-21T22:17:00Z

lightning/src/chain/chainmonitor.rs

@@ -640,27 +635,27 @@ where C::Target: chain::Filter,
 	/// data could be moved to an archive location or removed entirely.
 	pub fn archive_fully_resolved_channel_monitors(&self) {
 		let mut have_monitors_to_prune = false;
-		for (funding_txo, monitor_holder) in self.monitors.read().unwrap().iter() {
+		for (_, monitor_holder) in self.monitors.read().unwrap().iter() {


Likewise with values()

jkczyz · 2025-01-21T22:18:20Z

lightning/src/chain/chainmonitor.rs

@@ -294,9 +293,9 @@ where C::Target: chain::Filter,

 		// do some followup cleanup if any funding outpoints were added in between iterations


Update comment.

jkczyz · 2025-01-21T22:24:54Z

lightning/src/chain/mod.rs

@@ -276,7 +276,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
 	/// [`get_outputs_to_watch`]: channelmonitor::ChannelMonitor::get_outputs_to_watch
 	/// [`block_connected`]: channelmonitor::ChannelMonitor::block_connected
 	/// [`block_disconnected`]: channelmonitor::ChannelMonitor::block_disconnected
-	fn watch_channel(&self, funding_txo: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> Result<ChannelMonitorUpdateStatus, ()>;
+	fn watch_channel(&self, channel_id: ChannelId, monitor: ChannelMonitor<ChannelSigner>) -> Result<ChannelMonitorUpdateStatus, ()>;


Need to update the docs accordingly and mention how to obtain the funding_txo from the ChannelMonitor, if necessary.

jkczyz · 2025-01-21T22:25:00Z

lightning/src/chain/mod.rs

@@ -293,7 +293,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
 	/// [`ChannelMonitorUpdateStatus::UnrecoverableError`], see its documentation for more info.
 	///
 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
-	fn update_channel(&self, funding_txo: OutPoint, update: &ChannelMonitorUpdate) -> ChannelMonitorUpdateStatus;
+	fn update_channel(&self, channel_id: ChannelId, update: &ChannelMonitorUpdate) -> ChannelMonitorUpdateStatus;


jkczyz · 2025-01-21T22:28:52Z

lightning/src/ln/channelmanager.rs

 		for _ in 0..channel_count {
 			let mut channel: FundedChannel<SP> = FundedChannel::read(reader, (
 				&args.entropy_source, &args.signer_provider, best_block_height, &provided_channel_type_features(&args.default_config)
 			))?;
 			let logger = WithChannelContext::from(&args.logger, &channel.context, None);
+			let channel_id = channel.context.channel_id();
+			channel_id_set.insert(channel_id.clone());


Clone not needed?

jkczyz · 2025-01-21T22:32:15Z

lightning/src/ln/channelmanager.rs

-	in_flight_monitor_updates: BTreeMap<OutPoint, Vec<ChannelMonitorUpdate>>,
+	in_flight_monitor_updates: BTreeMap<(OutPoint, ChannelId), Vec<ChannelMonitorUpdate>>,


Wondering if it would simplify anything if either the ChannelId or OutPoint? were tupled with the value instead of the key.

wpaulino added the weekly goal Someone wants to land this this week label Jan 21, 2025

wpaulino requested review from TheBlueMatt and jkczyz January 21, 2025 19:11

wpaulino force-pushed the channel-monitor-by-channel-id branch from 70ad719 to 40aeb66 Compare January 21, 2025 20:53

wpaulino added 3 commits January 21, 2025 13:04

wpaulino force-pushed the channel-monitor-by-channel-id branch from 40aeb66 to 1c7a0a1 Compare January 21, 2025 21:04

jkczyz reviewed Jan 21, 2025

View reviewed changes

wpaulino mentioned this pull request Jan 21, 2025

Encapsulate Channel enum variants inside a struct #3550

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start tracking ChannelMonitors by channel ID in ChainMonitor and ChannelManager #3554

Start tracking ChannelMonitors by channel ID in ChainMonitor and ChannelManager #3554

wpaulino commented Jan 21, 2025

codecov bot commented Jan 21, 2025

jkczyz left a comment

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

jkczyz Jan 21, 2025

		@@ -294,9 +293,9 @@ where C::Target: chain::Filter,

		// do some followup cleanup if any funding outpoints were added in between iterations

		in_flight_monitor_updates: BTreeMap<OutPoint, Vec<ChannelMonitorUpdate>>,
		in_flight_monitor_updates: BTreeMap<(OutPoint, ChannelId), Vec<ChannelMonitorUpdate>>,

Start tracking ChannelMonitors by channel ID in ChainMonitor and ChannelManager #3554

Are you sure you want to change the base?

Start tracking ChannelMonitors by channel ID in ChainMonitor and ChannelManager #3554

Conversation

wpaulino commented Jan 21, 2025

codecov bot commented Jan 21, 2025

Codecov Report

jkczyz 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

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