Prefer to use `MonitorUpdateRegeneratedOnStartup` where possible

In the next commit we'll drop the magic `u64::MAX`
`ChannelMonitorUpdate::update_id` value used when we don't know the
`ChannelMonitor`'s `latest_update_id` (i.e. when the channel is
closed). In order to do so, we will store further information about
`ChannelMonitor`s in the per-peer structure, keyed by the
counterparty's node ID, which will be used when applying
`ChannelMonitorUpdate`s to closed channels.

By taking advantage of the change in the previous commit, that
information is now reliably available when we generate the
`ChannelMonitorUpdate` (when claiming HTLCs), but in order to
ensure it is available when applying the `ChannelMonitorUpdate` we
need to use `BackgroundEvent::MonitorUpdateRegeneratedOnStartup`
instead of
`BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup` where
possible.

Here we do this, leaving `ClosedMonitorUpdateRegeneratedOnStartup`
only used to ensure very old channels (created in 0.0.118 or
earlier) which are not in the `ChannelManager` are force-closed on
startup.

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 15a8f0cddbeacda35efddd33db71e5db8b43845c..4e28c3129cc626f7051ff2d0b59c1567e8c6c132 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -897,9 +897,9 @@ struct ClaimablePayments {
 #[derive(Debug)]
 enum BackgroundEvent {
        /// Handle a ChannelMonitorUpdate which closes the channel or for an already-closed channel.
-       /// This is only separated from [`Self::MonitorUpdateRegeneratedOnStartup`] as the
-       /// maybe-non-closing variant needs a public key to handle channel resumption, whereas if the
-       /// channel has been force-closed we do not need the counterparty node_id.
+       /// This is only separated from [`Self::MonitorUpdateRegeneratedOnStartup`] as for truly
+       /// ancient [`ChannelMonitor`]s that haven't seen an update since LDK 0.0.118 we may not have
+       /// the counterparty node ID available.
        ///
        /// Note that any such events are lost on shutdown, so in general they must be updates which
        /// are regenerated on startup.
@@ -6978,17 +6978,15 @@ where
                        // If we're running during init we cannot update a monitor directly - they probably
                        // haven't actually been loaded yet. Instead, push the monitor update as a background
                        // event.
-                       // Note that while it's safe to use `ClosedMonitorUpdateRegeneratedOnStartup` here (the
-                       // channel is already closed) we need to ultimately handle the monitor update
-                       // completion action only after we've completed the monitor update. This is the only
-                       // way to guarantee this update *will* be regenerated on startup (otherwise if this was
-                       // from a forwarded HTLC the downstream preimage may be deleted before we claim
-                       // upstream). Thus, we need to transition to some new `BackgroundEvent` type which will
-                       // complete the monitor update completion action from `completion_action`.
-                       self.pending_background_events.lock().unwrap().push(
-                               BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((
-                                       prev_hop.outpoint, prev_hop.channel_id, preimage_update,
-                               )));
+                       // TODO: Track this update as pending and only complete the completion action when it
+                       // finishes.
+                       let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+                               counterparty_node_id,
+                               funding_txo: prev_hop.outpoint,
+                               channel_id: prev_hop.channel_id,
+                               update: preimage_update,
+                       };
+                       self.pending_background_events.lock().unwrap().push(event);
                }
                // Note that we do process the completion action here. This totally could be a
                // duplicate claim, but we have no way of knowing without interrogating the
@@ -7081,22 +7079,35 @@ where
                                                                // There should be a `BackgroundEvent` pending...
                                                                assert!(background_events.iter().any(|ev| {
                                                                        match ev {
-                                                                               // to apply a monitor update that blocked the claiming channel,
                                                                                BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
                                                                                        funding_txo, update, ..
                                                                                } => {
                                                                                        if *funding_txo == claiming_chan_funding_outpoint {
+                                                                                               // to apply a monitor update that blocked the claiming channel,
                                                                                                assert!(update.updates.iter().any(|upd|
                                                                                                        if let ChannelMonitorUpdateStep::PaymentPreimage {
                                                                                                                payment_preimage: update_preimage
                                                                                                        } = upd {
                                                                                                                payment_preimage == *update_preimage
-                                                                                                       } else { false }
+                                                                                                       } else {
+                                                                                                               false
+                                                                                                       }
+                                                                                               ), "{:?}", update);
+                                                                                               true
+                                                                                       } else if *funding_txo == next_channel_outpoint {
+                                                                                               // or the channel we'd unblock is already closed,
+                                                                                               assert!(update.updates.iter().any(|upd|
+                                                                                                       if let ChannelMonitorUpdateStep::ChannelForceClosed { .. } = upd {
+                                                                                                               true
+                                                                                                       } else {
+                                                                                                               false
+                                                                                                       }
                                                                                                ), "{:?}", update);
                                                                                                true
                                                                                        } else { false }
                                                                                },
-                                                                               // or the channel we'd unblock is already closed,
+                                                                               // or the channel we'd unblock is already closed (for an
+                                                                               // old channel),
                                                                                BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(
                                                                                        (funding_txo, _channel_id, monitor_update)
                                                                                ) => {
@@ -12389,7 +12400,23 @@ where
                                        updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }],
                                        channel_id: Some(monitor.channel_id()),
                                };
-                               close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update)));
+                               if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() {
+                                       let update = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+                                               counterparty_node_id,
+                                               funding_txo: *funding_txo,
+                                               channel_id,
+                                               update: monitor_update,
+                                       };
+                                       close_background_events.push(update);
+                               } else {
+                                       // This is a fairly old `ChannelMonitor` that hasn't seen an update to its
+                                       // off-chain state since LDK 0.0.118 (as in LDK 0.0.119 any off-chain
+                                       // `ChannelMonitorUpdate` will set the counterparty ID).
+                                       // Thus, we assume that it has no pending HTLCs and we will not need to
+                                       // generate a `ChannelMonitorUpdate` for it aside from this
+                                       // `ChannelForceClosed` one.
+                                       close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update)));
+                               }
                        }
                }