From dd21fce8c2247b1134ed21bb0168c019f367d7d3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 30 Sep 2025 10:41:38 +0000 Subject: [PATCH] Correctly handle new `ChannelMonitorUpdate`s to old post-FC chans In 0.1 (1481216793169ae83c87ae26fddda8a1971e35f9) we started setting `ChannelMonitorUpdate::update_id` to a non-`u64::MAX` value for updates generated after a channel has been closed. This is great, but in 71a364c13345396fd75b22877e03b6a7b1d2bca1 we then started calculating the next `update_id` by incrementing the last `update_id` we saw when we started and were looking at the `ChannelMonitor`. However, the last-applied `update_id` may well be `u64::MAX` for old `ChannelMonitor`s which were closed prior to 0.1. In that case the increment would overflow. Here we fix this naively by simply replacing the increment with a `saturating_add`. While its possible this will result in a `ChannelMonitorUpdate` being tracked as in-flight (only for the `ReleasePaymentComplete` updates added in 71a364c13345396fd75b2287) at the same `update_id` as other updates already in-flight and handling post-`ChannelMonitorUpdate` actions too early, this should only apply to releasing payment complete updates, which have no post-`ChannelMonitorUpdate` action. Its also possible that this leads to a regression in the future, where we have some new post-closure update that does have a post-`ChannelMonitorUpdate` action and we run it too early, but by then presumably its fairly rare to have a `ChannelMonitor` for a channel closed pre-0.1 that still needs multiple updates. --- lightning/src/ln/channelmanager.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1d87eccfe66..222da4b7626 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1601,6 +1601,8 @@ where /// the highest `update_id` of all the pending in-flight updates (note that any pending updates /// not yet applied sitting in [`ChannelManager::pending_background_events`] will also be /// considered as they are also in [`Self::in_flight_monitor_updates`]). + /// + /// Note that channels which were closed prior to LDK 0.1 may have a value here of `u64::MAX`. closed_channel_monitor_update_ids: BTreeMap, /// The peer is currently connected (i.e. we've seen a /// [`BaseMessageHandler::peer_connected`] and no corresponding @@ -13262,7 +13264,8 @@ where .closed_channel_monitor_update_ids .get_mut(&channel_id) .expect("Channels originating a payment resolution must have a monitor"); - *update_id += 1; + // Note that for channels closed pre-0.1, the latest update_id is `u64::MAX`. + *update_id = update_id.saturating_add(1); let update = ChannelMonitorUpdate { update_id: *update_id, @@ -16523,7 +16526,9 @@ where should_queue_fc_update = !monitor.no_further_updates_allowed(); let mut latest_update_id = monitor.get_latest_update_id(); if should_queue_fc_update { - latest_update_id += 1; + // Note that for channels closed pre-0.1, the latest update_id is + // `u64::MAX`. + latest_update_id = latest_update_id.saturating_add(1); } per_peer_state .entry(counterparty_node_id) @@ -17170,7 +17175,9 @@ where .closed_channel_monitor_update_ids .get_mut(channel_id) .expect("Channels originating a preimage must have a monitor"); - *update_id += 1; + // Note that for channels closed pre-0.1, the latest + // update_id is `u64::MAX`. + *update_id = update_id.saturating_add(1); pending_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id: monitor.get_counterparty_node_id(),