From: Matt Corallo Date: Sun, 6 Oct 2024 19:54:32 +0000 (+0000) Subject: Prefer to use `MonitorUpdateRegeneratedOnStartup` where possible X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=3f368909121897d35cfd3dc77e743531c7301ee8;p=rust-lightning 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 30fea3780..c9ffc73a3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -973,9 +973,9 @@ impl 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. @@ -7109,17 +7109,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.funding_txo, 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.funding_txo, + 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 @@ -7212,22 +7210,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) ) => { @@ -12543,7 +12554,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))); + } } }