Merge pull request #3569 from jkczyz/2025-01-v2-channel-monitor-persist

Support persisting `ChannelMonitor`s after splicing

Author: TheBlueMatt

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};
@@ -879,6 +880,7 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	holder_revocation_basepoint: RevocationBasepoint,
 	channel_id: ChannelId,
 	funding_info: (OutPoint, ScriptBuf),
+	first_confirmed_funding_txo: OutPoint,
 	current_counterparty_commitment_txid: Option<Txid>,
 	prev_counterparty_commitment_txid: Option<Txid>,
 
@@ -1246,6 +1248,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
 			(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 +1401,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 first_confirmed_funding_txo = funding_info.0;
+
 		Self::from_impl(ChannelMonitorImpl {
 			latest_update_id: 0,
 			commitment_transaction_number_obscure_factor,
@@ -1411,6 +1416,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			holder_revocation_basepoint,
 			channel_id,
 			funding_info,
+			first_confirmed_funding_txo,
 			current_counterparty_commitment_txid: None,
 			prev_counterparty_commitment_txid: None,
 
@@ -1460,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::V1Channel(funding_outpoint)
+		} else {
+			MonitorName::V2Channel(channel_id)
+		}
+	}
+
 	#[cfg(test)]
 	fn provide_secret(&self, idx: u64, secret: [u8; 32]) -> Result<(), &'static str> {
 		self.inner.lock().unwrap().provide_secret(idx, secret)
@@ -5047,6 +5073,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<HashMap<_, _>> = 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),
@@ -5061,6 +5088,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() {
@@ -5113,6 +5141,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,
 

lightning/src/ln/channel.rs

@@ -1965,6 +1965,8 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> 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),

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();

lightning/src/ln/functional_tests.rs

@@ -2771,8 +2771,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
@@ -2788,7 +2787,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]);