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

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<TestChannelSigner> for TestPersister {
 	fn persist_new_channel(
-		&self, _funding_txo: OutPoint, _data: &channelmonitor::ChannelMonitor<TestChannelSigner>,
+		&self, _monitor_name: MonitorName,
+		_data: &channelmonitor::ChannelMonitor<TestChannelSigner>,
 	) -> 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<TestChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
 		self.update_ret.lock().unwrap().clone()
 	}
 
-	fn archive_persisted_channel(&self, _: OutPoint) {}
+	fn archive_persisted_channel(&self, _monitor_name: MonitorName) {}
 }

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"),
 		}

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<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, 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<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_name: MonitorName, monitor: &ChannelMonitor<ChannelSigner>) -> ChannelMonitorUpdateStatus;
 
 	/// Update one channel's data. The provided [`ChannelMonitor`] has already applied the given
 	/// update.
@@ -156,7 +159,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_name: MonitorName, 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 +171,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_name: MonitorName);
 }
 
 struct MonitorHolder<ChannelSigner: EcdsaChannelSigner> {
@@ -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,17 +827,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();
 				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(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 => {

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<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		})
 	}
 
+	/// 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)