From 6c203d803e51ce5c5d1014646dc2d9372337c4fb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 20 Jun 2024 15:17:10 +0000 Subject: [PATCH] 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. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning/src/ln/channelmanager.rs | 15 +++++++++------ lightning/src/ln/functional_test_utils.rs | 4 ++-- lightning/src/ln/reload_tests.rs | 4 ++-- pending_changelog/3322-b.txt | 7 +++++++ 5 files changed, 21 insertions(+), 11 deletions(-) create mode 100644 pending_changelog/3322-b.txt diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f897ba6e0..821acfc53 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -768,7 +768,7 @@ pub fn do_test(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); } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6f88985d9..dc0716511 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1629,7 +1629,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)?; @@ -12231,9 +12231,12 @@ impl Readable for VecDeque<(Event, Option)> { /// 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. @@ -12316,7 +12319,7 @@ where /// this struct. /// /// This is not exported to bindings users because we have no HashMap bindings - pub channel_monitors: HashMap::EcdsaSigner>>, + pub channel_monitors: HashMap::EcdsaSigner>>, } impl<'a, M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, MR: Deref, L: Deref> @@ -12339,7 +12342,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<::EcdsaSigner>>, + mut channel_monitors: Vec<&'a ChannelMonitor<::EcdsaSigner>>, ) -> Self { Self { entropy_source, node_signer, signer_provider, fee_estimator, chain_monitor, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 70c64fd21..de0b4c7d4 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -686,7 +686,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); } @@ -1128,7 +1128,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 { diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index a043d5c0c..40f7c785b 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/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()); diff --git a/pending_changelog/3322-b.txt b/pending_changelog/3322-b.txt new file mode 100644 index 000000000..c8bb0c64b --- /dev/null +++ b/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. -- 2.39.5