Correctly handle new `ChannelMonitorUpdate`s to old post-FC chans
What the developer wrote
Authored by Matt Corallo
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.
What changed, and why it matters
This patch fixes an integer overflow bug in the Lightning Dev Kit (LDK) rust-lightning implementation. For very old channels that were closed before version 0.1, an internal counter used to track channel monitor updates could be set to the maximum possible value (u64::MAX). When LDK later needed to generate a new update for one of these old closed channels, it tried to add 1 to that maximum value, which would overflow and cause a panic (crash). The fix replaces the simple addition with a 'saturating add,' which keeps the value at the maximum instead of wrapping around. The commit notes this is a naive fix that could theoretically cause a future regression if new types of post-closure updates are added, but for now only applies to updates that have no follow-up action.
Apply the patch. Users running nodes with ChannelMonitors from channels closed before LDK 0.1 should upgrade to avoid a potential panic when payment resolution or force-closure-related monitor updates are regenerated. Monitor for any future post-closure update types that may rely on unique update_id sequencing.
Security signals we found
Integer overflow in update_id generation for pre-0.1 closed channels
Potential panic/crash when processing monitor updates for legacy closed channels
Post-closure ChannelMonitorUpdate ID collision risk acknowledged by author
Saturating arithmetic used as defensive fix
Evidence from the diff
In LDK 0.1, ChannelMonitorUpdate::update_id began using non-u64::MAX values for updates generated after a channel was force-closed (FC). Later, commit 71a364c started computing the next update_id by incrementing the last seen/applied update_id. For ChannelMonitors closed prior to 0.1, the last-applied update_id may be u64::MAX, so incrementing it causes an arithmetic overflow/panic. The patch changes three += 1 operations on closed_channel_monitor_update_ids and latest_update_id to saturating_add(1), capping at u64::MAX. The commit author acknowledges this is a minimal fix: it may cause a ReleasePaymentComplete update to share an update_id with an already in-flight update, but such updates currently have no post-ChannelMonitorUpdate action, so the risk is limited.
Changed components
lightning/src/ln/channelmanager.rsclosed_channel_monitor_update_ids mapChannelMonitorUpdate update_id generationBackgroundEvent::MonitorUpdateRegeneratedOnStartup handlingForce-closure (FC) monitor update queuingInspect captured patch +10 / −3
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 1d87ecc..222da4b 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<ChannelId, u64>,
/// 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(),
Why this scored 42/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.