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.

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index f897ba6e0922c785593cbf2fa6cd6211f99fadf5..821acfc530111343d49db9f3edfad847b8b58da8 100644 (file)
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -768,7 +768,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);
                        }
 

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 6f88985d9da2b0adaa96ef7caf7dbc359e2e738a..dc07165113e9692ad7a1ccbdd0d6451377df0cb9 100644 (file)
--- 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<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.
@@ -12316,7 +12319,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>
@@ -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<<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,

diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 70c64fd2192df8805f9117e2c55809beb58c41a6..de0b4c7d4bb9c263289c987e16217192645aaa84 100644 (file)
--- 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 a043d5c0c8615d991d3d5194d86ddf829da1453d..40f7c785bc5221c1f016061c2466491b6712b88b 100644 (file)
--- 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 (file)
index 0000000..c8bb0c6
--- /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.