Stop relying on ChannelMonitor persistence after manager read

When we discover we've only partially claimed an MPP HTLC during
`ChannelManager` reading, we need to add the payment preimage to
all other `ChannelMonitor`s that were a part of the payment.

We previously did this with a direct call on the `ChannelMonitor`,
requiring users write the full `ChannelMonitor` to disk to ensure
that updated information made it.

This adds quite a bit of delay during initial startup - fully
resilvering each `ChannelMonitor` just to handle this one case is
incredibly excessive.

Over the past few commits we dropped the need to pass HTLCs
directly to the `ChannelMonitor`s using the background events to
provide `ChannelMonitorUpdate`s insetad.

Thus, here we finally drop the requirement to resilver
`ChannelMonitor`s on startup.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -767,7 +767,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 				chain_monitor.latest_monitors.lock().unwrap().insert(outpoint, prev_state);
 			}
 			let mut monitor_refs = new_hash_map();
-			for (outpoint, monitor) in monitors.iter_mut() {
+			for (outpoint, monitor) in monitors.iter() {
 				monitor_refs.insert(*outpoint, monitor);
 			}
 

lightning/src/ln/channelmanager.rs

@@ -1607,7 +1607,7 @@ where
 /// let mut channel_monitors = read_channel_monitors();
 /// let args = ChannelManagerReadArgs::new(
 ///     entropy_source, node_signer, signer_provider, fee_estimator, chain_monitor, tx_broadcaster,
-///     router, message_router, logger, default_config, channel_monitors.iter_mut().collect(),
+///     router, message_router, logger, default_config, channel_monitors.iter().collect(),
 /// );
 /// let (block_hash, channel_manager) =
 ///     <(BlockHash, ChannelManager<_, _, _, _, _, _, _, _, _>)>::read(&mut reader, args)?;
@@ -12217,9 +12217,12 @@ impl Readable for VecDeque<(Event, Option<EventCompletionAction>)> {
 /// 3) If you are not fetching full blocks, register all relevant [`ChannelMonitor`] outpoints the
 ///    same way you would handle a [`chain::Filter`] call using
 ///    [`ChannelMonitor::get_outputs_to_watch`] and [`ChannelMonitor::get_funding_txo`].
-/// 4) Reconnect blocks on your [`ChannelMonitor`]s.
-/// 5) Disconnect/connect blocks on the [`ChannelManager`].
-/// 6) Re-persist the [`ChannelMonitor`]s to ensure the latest state is on disk.
+/// 4) Disconnect/connect blocks on your [`ChannelMonitor`]s to get them in sync with the chain.
+/// 5) Disconnect/connect blocks on the [`ChannelManager`] to get it in sync with the chain.
+/// 6) Optionally re-persist the [`ChannelMonitor`]s to ensure the latest state is on disk.
+///    This is important if you have replayed a nontrivial number of blocks in step (4), allowing
+///    you to avoid having to replay the same blocks if you shut down quickly after startup. It is
+///    otherwise not required.
 ///    Note that if you're using a [`ChainMonitor`] for your [`chain::Watch`] implementation, you
 ///    will likely accomplish this as a side-effect of calling [`chain::Watch::watch_channel`] in
 ///    the next step.
@@ -12302,7 +12305,7 @@ where
 	/// this struct.
 	///
 	/// This is not exported to bindings users because we have no HashMap bindings
-	pub channel_monitors: HashMap<OutPoint, &'a mut ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>,
+	pub channel_monitors: HashMap<OutPoint, &'a ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>,
 }
 
 impl<'a, M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, MR: Deref, L: Deref>
@@ -12325,7 +12328,7 @@ where
 		entropy_source: ES, node_signer: NS, signer_provider: SP, fee_estimator: F,
 		chain_monitor: M, tx_broadcaster: T, router: R, message_router: MR, logger: L,
 		default_config: UserConfig,
-		mut channel_monitors: Vec<&'a mut ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>,
+		mut channel_monitors: Vec<&'a ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>,
 	) -> Self {
 		Self {
 			entropy_source, node_signer, signer_provider, fee_estimator, chain_monitor,

lightning/src/ln/functional_test_utils.rs

@@ -684,7 +684,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 			// them to ensure we can write and reload our ChannelManager.
 			{
 				let mut channel_monitors = new_hash_map();
-				for monitor in deserialized_monitors.iter_mut() {
+				for monitor in deserialized_monitors.iter() {
 					channel_monitors.insert(monitor.get_funding_txo().0, monitor);
 				}
 
@@ -1126,7 +1126,7 @@ pub fn _reload_node<'a, 'b, 'c>(node: &'a Node<'a, 'b, 'c>, default_config: User
 	let mut node_read = &chanman_encoded[..];
 	let (_, node_deserialized) = {
 		let mut channel_monitors = new_hash_map();
-		for monitor in monitors_read.iter_mut() {
+		for monitor in monitors_read.iter() {
 			assert!(channel_monitors.insert(monitor.get_funding_txo().0, monitor).is_none());
 		}
 		<(BlockHash, TestChannelManager<'b, 'c>)>::read(&mut node_read, ChannelManagerReadArgs {

lightning/src/ln/reload_tests.rs

@@ -426,7 +426,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
 		chain_monitor: nodes[0].chain_monitor,
 		tx_broadcaster: nodes[0].tx_broadcaster,
 		logger: &logger,
-		channel_monitors: node_0_stale_monitors.iter_mut().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(),
+		channel_monitors: node_0_stale_monitors.iter().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(),
 	}) { } else {
 		panic!("If the monitor(s) are stale, this indicates a bug and we should get an Err return");
 	};
@@ -444,7 +444,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
 		chain_monitor: nodes[0].chain_monitor,
 		tx_broadcaster: nodes[0].tx_broadcaster,
 		logger: &logger,
-		channel_monitors: node_0_monitors.iter_mut().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(),
+		channel_monitors: node_0_monitors.iter().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(),
 	}).unwrap();
 	nodes_0_deserialized = nodes_0_deserialized_tmp;
 	assert!(nodes_0_read.is_empty());

pending_changelog/3322-b.txt

@@ -0,0 +1,7 @@
+API Updates
+===========
+
+As a part of adding robustness against several unlikely scenarios, redundant `PaymentClaimed`
+`Event`s will be generated more frequently on startup for payments received on LDK 0.1 and
+newer. A new `Event::PaymentClaimed::payment_id` field may be used to better differentiate
+between redundant payments.