Use channel ID over funding outpoint to track monitors in ChainMonitor

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.

Author: wpaulino

lightning/src/chain/chainmonitor.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, ()>;
 
 	/// Updates a channel identified by `funding_txo` by applying `update` to its monitor.
 	///
@@ -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;
 
 	/// Returns any monitor events since the last call. Subsequent calls must only return new
 	/// events.

lightning/src/chain/mod.rs

@@ -489,8 +489,8 @@ fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCas
 	if test_case == UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored {
 		dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, block_raa_signer_op);
 		chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
-		let (outpoint, latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
-		dst.chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
+		let (latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+		dst.chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id, latest_update);
 		check_added_monitors!(dst, 0);
 	}
 
@@ -614,8 +614,8 @@ fn do_test_async_commitment_signature_peer_disconnect(test_case: UnblockSignerAc
 	if test_case == UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored {
 		dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
 		chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
-		let (outpoint, latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
-		dst.chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
+		let (latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+		dst.chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id, latest_update);
 		check_added_monitors!(dst, 0);
 	}
 
@@ -737,8 +737,8 @@ fn do_test_async_commitment_signature_ordering(monitor_update_failure: bool) {
 
 	if monitor_update_failure {
 		chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
-		let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
-		nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
+		let (latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+		nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id, latest_update);
 		check_added_monitors!(nodes[0], 0);
 	}
 

lightning/src/ln/async_signer_tests.rs

@@ -1700,7 +1700,7 @@ where
 ///
 /// // Move the monitors to the ChannelManager's chain::Watch parameter
 /// for monitor in channel_monitors {
-///     chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
+///     chain_monitor.watch_channel(monitor.channel_id(), monitor);
 /// }
 /// # Ok(())
 /// # }
@@ -3295,7 +3295,7 @@ macro_rules! handle_new_monitor_update {
 				$in_flight_updates.len() - 1
 			});
 		if $self.background_events_processed_since_startup.load(Ordering::Acquire) {
-			let update_res = $self.chain_monitor.update_channel($funding_txo, &$in_flight_updates[$update_idx]);
+			let update_res = $self.chain_monitor.update_channel($chan_id, &$in_flight_updates[$update_idx]);
 			handle_new_monitor_update!($self, update_res, $logger, $chan_id, _internal, $completed)
 		} else {
 			// We blindly assume that the ChannelMonitorUpdate will be regenerated on startup if we
@@ -6307,10 +6307,10 @@ where
 
 		for event in background_events.drain(..) {
 			match event {
-				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);
 				},
 				BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => {
 					self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update);
@@ -8116,7 +8116,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						fail_chan!("The funding_created message had the same funding_txid as an existing channel - funding is not possible");
 					},
 					hash_map::Entry::Vacant(i_e) => {
-						let monitor_res = self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
+						let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor);
 						if let Ok(persist_state) = monitor_res {
 							i_e.insert(chan.context.get_counterparty_node_id());
 							mem::drop(outpoint_to_peer_lock);
@@ -8175,7 +8175,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						chan.funding_signed(&msg, best_block, &self.signer_provider, &&logger);
 					match res {
 						Ok((mut chan, monitor)) => {
-							if let Ok(persist_status) = self.chain_monitor.watch_channel(chan.context.get_funding_txo().unwrap(), monitor) {
+							if let Ok(persist_status) = self.chain_monitor.watch_channel(chan.context.channel_id(), monitor) {
 								// We really should be able to insert here without doing a second
 								// lookup, but sadly rust stdlib doesn't currently allow keeping
 								// the original Entry around with the value removed.
@@ -8860,7 +8860,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						let monitor = try_chan_phase_entry!(
 							self, peer_state, chan.commitment_signed_initial_v2(msg, best_block, &self.signer_provider, &&logger),
 							chan_phase_entry);
-						let monitor_res = self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
+						let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor);
 						if let Ok(persist_state) = monitor_res {
 							handle_new_monitor_update!(self, persist_state, peer_state_lock, peer_state,
 								per_peer_state, chan, INITIAL_MONITOR);

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -255,8 +255,8 @@ pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block)
 
 fn call_claimable_balances<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>) {
 	// Ensure `get_claimable_balances`' self-tests never panic
-	for (funding_outpoint, _channel_id) in node.chain_monitor.chain_monitor.list_monitors() {
-		node.chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances();
+	for channel_id in node.chain_monitor.chain_monitor.list_monitors() {
+		node.chain_monitor.chain_monitor.get_monitor(channel_id).unwrap().get_claimable_balances();
 	}
 }
 
@@ -554,9 +554,7 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> {
 			channel_keys_id = Some(chan.channel_keys_id);
 		}
 
-		let monitor = self.chain_monitor.chain_monitor.list_monitors().into_iter()
-			.find(|(_, channel_id)| *channel_id == *chan_id)
-			.and_then(|(funding_txo, _)| self.chain_monitor.chain_monitor.get_monitor(funding_txo).ok());
+		let monitor = self.chain_monitor.chain_monitor.get_monitor(*chan_id).ok();
 		if let Some(monitor) = monitor {
 			monitor.do_mut_signer_call(|signer| {
 				channel_keys_id = channel_keys_id.or(Some(signer.inner.channel_keys_id()));
@@ -690,9 +688,9 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 			let feeest = test_utils::TestFeeEstimator::new(253);
 			let mut deserialized_monitors = Vec::new();
 			{
-				for (outpoint, _channel_id) in self.chain_monitor.chain_monitor.list_monitors() {
+				for channel_id in self.chain_monitor.chain_monitor.list_monitors() {
 					let mut w = test_utils::TestVecWriter(Vec::new());
-					self.chain_monitor.chain_monitor.get_monitor(outpoint).unwrap().write(&mut w).unwrap();
+					self.chain_monitor.chain_monitor.get_monitor(channel_id).unwrap().write(&mut w).unwrap();
 					let (_, deserialized_monitor) = <(BlockHash, ChannelMonitor<TestChannelSigner>)>::read(
 						&mut io::Cursor::new(&w.0), (self.keys_manager, self.keys_manager)).unwrap();
 					deserialized_monitors.push(deserialized_monitor);
@@ -734,8 +732,8 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 			let chain_source = test_utils::TestChainSource::new(Network::Testnet);
 			let chain_monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &broadcaster, &self.logger, &feeest, &persister, &self.keys_manager);
 			for deserialized_monitor in deserialized_monitors.drain(..) {
-				let funding_outpoint = deserialized_monitor.get_funding_txo().0;
-				if chain_monitor.watch_channel(funding_outpoint, deserialized_monitor) != Ok(ChannelMonitorUpdateStatus::Completed) {
+				let channel_id = deserialized_monitor.channel_id();
+				if chain_monitor.watch_channel(channel_id, deserialized_monitor) != Ok(ChannelMonitorUpdateStatus::Completed) {
 					panic!();
 				}
 			}
@@ -1033,20 +1031,7 @@ macro_rules! get_channel_type_features {
 macro_rules! get_monitor {
 	($node: expr, $channel_id: expr) => {
 		{
-			use bitcoin::hashes::Hash;
-			let mut monitor = None;
-			// Assume funding vout is either 0 or 1 blindly
-			for index in 0..2 {
-				if let Ok(mon) = $node.chain_monitor.chain_monitor.get_monitor(
-					$crate::chain::transaction::OutPoint {
-						txid: bitcoin::Txid::from_slice(&$channel_id.0[..]).unwrap(), index
-					})
-				{
-					monitor = Some(mon);
-					break;
-				}
-			}
-			monitor.unwrap()
+			$node.chain_monitor.chain_monitor.get_monitor($channel_id).unwrap()
 		}
 	}
 }
@@ -1170,8 +1155,8 @@ pub fn _reload_node<'a, 'b, 'c>(node: &'a Node<'a, 'b, 'c>, default_config: User
 	assert!(node_read.is_empty());
 
 	for monitor in monitors_read.drain(..) {
-		let funding_outpoint = monitor.get_funding_txo().0;
-		assert_eq!(node.chain_monitor.watch_channel(funding_outpoint, monitor),
+		let channel_id = monitor.channel_id();
+		assert_eq!(node.chain_monitor.watch_channel(channel_id, monitor),
 			Ok(ChannelMonitorUpdateStatus::Completed));
 		check_added_monitors!(node, 1);
 	}
@@ -1275,19 +1260,24 @@ pub fn create_dual_funding_utxos_with_prev_txs(
 }
 
 pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>, channel_value: u64, expected_temporary_channel_id: ChannelId) -> Transaction {
-	let (temporary_channel_id, tx, funding_output) = create_funding_transaction(node_a, &node_b.node.get_our_node_id(), channel_value, 42);
+	let (temporary_channel_id, tx, _) = create_funding_transaction(node_a, &node_b.node.get_our_node_id(), channel_value, 42);
 	assert_eq!(temporary_channel_id, expected_temporary_channel_id);
 
 	assert!(node_a.node.funding_transaction_generated(temporary_channel_id, node_b.node.get_our_node_id(), tx.clone()).is_ok());
 	check_added_monitors!(node_a, 0);
 
 	let funding_created_msg = get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id());
 	assert_eq!(funding_created_msg.temporary_channel_id, expected_temporary_channel_id);
+
+	let channel_id = ChannelId::v1_from_funding_txid(
+		funding_created_msg.funding_txid.as_byte_array(), funding_created_msg.funding_output_index
+	);
+
 	node_b.node.handle_funding_created(node_a.node.get_our_node_id(), &funding_created_msg);
 	{
 		let mut added_monitors = node_b.chain_monitor.added_monitors.lock().unwrap();
 		assert_eq!(added_monitors.len(), 1);
-		assert_eq!(added_monitors[0].0, funding_output);
+		assert_eq!(added_monitors[0].0, channel_id);
 		added_monitors.clear();
 	}
 	expect_channel_pending_event(&node_b, &node_a.node.get_our_node_id());
@@ -1296,7 +1286,7 @@ pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &
 	{
 		let mut added_monitors = node_a.chain_monitor.added_monitors.lock().unwrap();
 		assert_eq!(added_monitors.len(), 1);
-		assert_eq!(added_monitors[0].0, funding_output);
+		assert_eq!(added_monitors[0].0, channel_id);
 		added_monitors.clear();
 	}
 	expect_channel_pending_event(&node_a, &node_b.node.get_our_node_id());