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.

Author: jkczyz

lightning/src/chain/chainmonitor.rs

@@ -104,9 +104,9 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> {
 	/// Persist a new channel's data in response to a [`chain::Watch::watch_channel`] call. This is
 	/// called by [`ChannelManager`] for new channels, or may be called directly, e.g. on startup.
 	///
-	/// The data can be stored any way you want, 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 [`ChannelMonitor::persistence_key`] is
+	/// used to maintain a correct mapping with the stored channel data. Note that you **must**
+	/// persist every new monitor to disk.
 	///
 	/// The [`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 +117,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;
 
 	/// Update one channel's data. The provided [`ChannelMonitor`] has already applied the given
 	/// update.
@@ -156,7 +156,7 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> {
 	/// [`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<ChannelSigner>) -> ChannelMonitorUpdateStatus;
+	fn update_persisted_channel(&self, monitor_update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
 	/// Prevents the channel monitor from being loaded on startup.
 	///
 	/// Archiving the data in a backup location (rather than deleting it fully) is useful for
@@ -168,7 +168,7 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> {
 	/// 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: &ChannelMonitor<ChannelSigner>);
 }
 
 struct MonitorHolder<ChannelSigner: EcdsaChannelSigner> {
@@ -342,8 +342,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().0;
-			match self.persister.update_persisted_channel(funding_txo, None, monitor) {
+			match self.persister.update_persisted_channel(None, monitor) {
 				ChannelMonitorUpdateStatus::Completed =>
 					log_trace!(logger, "Finished syncing Channel Monitor for channel {} for block-data",
 						log_funding_info!(monitor)
@@ -642,7 +641,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().0, None, &monitor_holder.monitor);
+				self.persister.update_persisted_channel(None, &monitor_holder.monitor);
 			}
 		}
 		if have_monitors_to_prune {
@@ -655,7 +654,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().0);
+					self.persister.archive_persisted_channel(&monitor_holder.monitor);
 					false
 				} else {
 					true
@@ -769,7 +768,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().0, &monitor);
+		let persist_res = self.persister.persist_new_channel(&monitor);
 		match persist_res {
 			ChannelMonitorUpdateStatus::InProgress => {
 				log_info!(logger, "Persistence of new ChannelMonitor for channel {} in progress", log_funding_info!(monitor));
@@ -825,17 +824,16 @@ 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().0;
 				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.
 					// We don't want to persist a `monitor_update` which results in a failure to apply later
 					// 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(None, monitor)
 				} else {
-					self.persister.update_persisted_channel(funding_txo, Some(update), monitor)
+					self.persister.update_persisted_channel(Some(update), monitor)
 				};
 				match persist_res {
 					ChannelMonitorUpdateStatus::InProgress => {

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

lightning/src/util/persist.rs

@@ -261,9 +261,9 @@ impl<ChannelSigner: EcdsaChannelSigner, K: KVStore + ?Sized> Persist<ChannelSign
 	// just shut down the node since we're not retrying persistence!
 
 	fn persist_new_channel(
-		&self, funding_txo: OutPoint, monitor: &ChannelMonitor<ChannelSigner>,
+		&self, monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
-		let monitor_name = MonitorName::from(funding_txo);
+		let monitor_name = monitor.persistence_key();
 		match self.write(
 			CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 			CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
@@ -276,10 +276,9 @@ impl<ChannelSigner: EcdsaChannelSigner, K: KVStore + ?Sized> Persist<ChannelSign
 	}
 
 	fn update_persisted_channel(
-		&self, funding_txo: OutPoint, _update: Option<&ChannelMonitorUpdate>,
-		monitor: &ChannelMonitor<ChannelSigner>,
+		&self, _update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
-		let monitor_name = MonitorName::from(funding_txo);
+		let monitor_name = monitor.persistence_key();
 		match self.write(
 			CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 			CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
@@ -291,8 +290,8 @@ impl<ChannelSigner: EcdsaChannelSigner, K: KVStore + ?Sized> Persist<ChannelSign
 		}
 	}
 
-	fn archive_persisted_channel(&self, funding_txo: OutPoint) {
-		let monitor_name = MonitorName::from(funding_txo);
+	fn archive_persisted_channel(&self, monitor: &ChannelMonitor<ChannelSigner>) {
+		let monitor_name = monitor.persistence_key();
 		let monitor = match self.read(
 			CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 			CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
@@ -334,21 +333,6 @@ where
 		CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 		CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
 	)? {
-		if stored_key.len() < 66 {
-			return Err(io::Error::new(
-				io::ErrorKind::InvalidData,
-				"Stored key has invalid length",
-			));
-		}
-
-		let txid = Txid::from_str(stored_key.split_at(64).0).map_err(|_| {
-			io::Error::new(io::ErrorKind::InvalidData, "Invalid tx ID in stored key")
-		})?;
-
-		let index: u16 = stored_key.split_at(65).1.parse().map_err(|_| {
-			io::Error::new(io::ErrorKind::InvalidData, "Invalid tx index in stored key")
-		})?;
-
 		match <(BlockHash, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>)>::read(
 			&mut io::Cursor::new(kv_store.read(
 				CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
@@ -358,14 +342,14 @@ where
 			(&*entropy_source, &*signer_provider),
 		) {
 			Ok((block_hash, channel_monitor)) => {
-				if channel_monitor.get_funding_txo().0.txid != txid
-					|| channel_monitor.get_funding_txo().0.index != index
-				{
+				let monitor_name = MonitorName::new(stored_key)?;
+				if channel_monitor.persistence_key().as_str() != monitor_name.as_str() {
 					return Err(io::Error::new(
 						io::ErrorKind::InvalidData,
 						"ChannelMonitor was stored under the wrong key",
 					));
 				}
+
 				res.push((block_hash, channel_monitor));
 			},
 			Err(_) => {
@@ -406,12 +390,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.
@@ -550,15 +535,17 @@ 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`
 	///
 	/// 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(
@@ -604,7 +591,6 @@ where
 		&self, monitor_name: &MonitorName,
 	) -> Result<(BlockHash, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), io::Error>
 	{
-		let outpoint: OutPoint = monitor_name.try_into()?;
 		let mut monitor_cursor = io::Cursor::new(self.kv_store.read(
 			CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
 			CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
@@ -619,9 +605,7 @@ where
 			(&*self.entropy_source, &*self.signer_provider),
 		) {
 			Ok((blockhash, channel_monitor)) => {
-				if channel_monitor.get_funding_txo().0.txid != outpoint.txid
-					|| channel_monitor.get_funding_txo().0.index != outpoint.index
-				{
+				if channel_monitor.persistence_key().as_str() != monitor_name.as_str() {
 					log_error!(
 						self.logger,
 						"ChannelMonitor {} was stored under the wrong key!",
@@ -724,10 +708,10 @@ 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<ChannelSigner>,
+		&self, monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		// Determine the proper key for this monitor
-		let monitor_name = MonitorName::from(funding_txo);
+		let monitor_name = monitor.persistence_key();
 		// Serialize and write the new monitor
 		let mut monitor_bytes = Vec::with_capacity(
 			MONITOR_UPDATING_PERSISTER_PREPEND_SENTINEL.len() + monitor.serialized_length(),
@@ -765,15 +749,14 @@ 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>,
-		monitor: &ChannelMonitor<ChannelSigner>,
+		&self, update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX;
 		if let Some(update) = update {
+			let monitor_name = monitor.persistence_key();
 			let 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 update_name = UpdateName::from(update.update_id);
 				match self.kv_store.write(
 					CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
@@ -795,7 +778,6 @@ where
 					},
 				}
 			} else {
-				let monitor_name = MonitorName::from(funding_txo);
 				// 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() {
@@ -804,7 +786,7 @@ where
 				};
 
 				// We could write this update, but it meets criteria of our design that calls for a full monitor write.
-				let monitor_update_status = self.persist_new_channel(funding_txo, monitor);
+				let monitor_update_status = self.persist_new_channel(monitor);
 
 				if let chain::ChannelMonitorUpdateStatus::Completed = monitor_update_status {
 					let channel_closed_legacy =
@@ -835,12 +817,12 @@ 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)
 		}
 	}
 
-	fn archive_persisted_channel(&self, funding_txo: OutPoint) {
-		let monitor_name = MonitorName::from(funding_txo);
+	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) {
 			Ok((_block_hash, monitor)) => monitor,
@@ -901,14 +883,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.
+/// [`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
 ///
 /// ```
@@ -1402,10 +1385,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),
@@ -1416,7 +1395,7 @@ 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) {
+			match ro_persister.persist_new_channel(&added_monitors[0].1) {
 				ChannelMonitorUpdateStatus::UnrecoverableError => {
 					// correct result
 				},
@@ -1427,7 +1406,7 @@ 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(Some(cmu), &added_monitors[0].1) {
 				ChannelMonitorUpdateStatus::UnrecoverableError => {
 					// correct result
 				},