Simplify legacy closed-channel monitor update persistence handling
What changed, and why it matters
This commit is a code cleanup in the Lightning Dev Kit's storage layer. It removes a special-case code path that read an old channel monitor just to decide which stale monitor updates to delete. Instead, it reuses an existing list-and-delete helper. The change only affects how old data is cleaned up after a channel is closed, and the commit message explicitly says the removed path is no longer expected to be hit. There is no direct evidence this fixes a security vulnerability.
No immediate security action required. Treat as a normal maintenance refactor. If deploying, verify through existing tests that stale monitor updates are still correctly cleaned up after channel closure and on startup.
Security signals we found
Refactor of persistence cleanup logic for channel monitors
Removal of pre-read of old monitor before persisting closed-channel update
No explicit security claim in commit message or diff
Commit describes the changed code as legacy and unlikely to be hit
Evidence from the diff
The patch refactors MonitorUpdatingPersister in lightning/src/util/persist.rs. Previously, when a legacy closed-channel monitor update was persisted (update_id == u64::MAX), the code first read the old persisted monitor to compute a cleanup range of stale updates. The new code removes that read and instead calls cleanup_stale_updates_for_monitor_to, which lists all updates for the monitor and deletes those with update_id <= latest_update_id. This is functionally equivalent for removing stale updates but avoids an extra monitor read and simplifies the logic. A new helper is extracted and reused in both the startup cleanup and the update-persistence paths.
Changed components
lightning/src/util/persist.rsMonitorUpdatingPersisterChannelMonitorUpdate persistence cleanupInspect captured patch +27 / −35
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index 912dc04..356bbdb 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -16,7 +16,6 @@ use alloc::sync::Arc;
use bitcoin::hashes::hex::FromHex;
use bitcoin::{BlockHash, Txid};
-use core::cmp;
use core::future::Future;
use core::ops::Deref;
use core::pin::Pin;
@@ -938,14 +937,22 @@ where
for monitor_key in monitor_keys {
let monitor_name = MonitorName::from_str(&monitor_key)?;
let (_, current_monitor) = self.read_monitor(&monitor_name, &monitor_key).await?;
- let primary = CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE;
- let updates = self.kv_store.list(primary, monitor_key.as_str()).await?;
- for update in updates {
- let update_name = UpdateName::new(update)?;
- // if the update_id is lower than the stored monitor, delete
- if update_name.0 <= current_monitor.get_latest_update_id() {
- self.kv_store.remove(primary, &monitor_key, update_name.as_str(), lazy).await?;
- }
+ let latest_update_id = current_monitor.get_latest_update_id();
+ self.cleanup_stale_updates_for_monitor_to(&monitor_key, latest_update_id, lazy).await;
+ }
+ Ok(())
+ }
+
+ async fn cleanup_stale_updates_for_monitor_to(
+ &self, monitor_key: &str, latest_update_id: u64, lazy: bool,
+ ) -> Result<(), io::Error> {
+ let primary = CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE;
+ let updates = self.kv_store.list(primary, monitor_key).await?;
+ for update in updates {
+ let update_name = UpdateName::new(update)?;
+ // if the update_id is lower than the stored monitor, delete
+ if update_name.0 <= latest_update_id {
+ self.kv_store.remove(primary, monitor_key, update_name.as_str(), lazy).await?;
}
}
Ok(())
@@ -989,40 +996,24 @@ where
.write(primary, &monitor_key, update_name.as_str(), update.encode())
.await
} else {
- // In case of channel-close monitor update, we need to read old monitor before persisting
- // the new one in order to determine the cleanup range.
- let maybe_old_monitor = match monitor.get_latest_update_id() {
- LEGACY_CLOSED_CHANNEL_UPDATE_ID => {
- let monitor_key = monitor_name.to_string();
- self.read_monitor(&monitor_name, &monitor_key).await.ok()
- },
- _ => None,
- };
-
// We could write this update, but it meets criteria of our design that calls for a full monitor write.
let write_status = self.persist_new_channel(monitor_name, monitor).await;
if let Ok(()) = write_status {
let channel_closed_legacy =
monitor.get_latest_update_id() == LEGACY_CLOSED_CHANNEL_UPDATE_ID;
- let cleanup_range = if channel_closed_legacy {
- // If there is an error while reading old monitor, we skip clean up.
- maybe_old_monitor.map(|(_, ref old_monitor)| {
- let start = old_monitor.get_latest_update_id();
- // We never persist an update with the legacy closed update_id
- let end = cmp::min(
- start.saturating_add(self.maximum_pending_updates),
- LEGACY_CLOSED_CHANNEL_UPDATE_ID - 1,
- );
- (start, end)
- })
+ let latest_update_id = monitor.get_latest_update_id();
+ if channel_closed_legacy {
+ let monitor_key = monitor_name.to_string();
+ self.cleanup_stale_updates_for_monitor_to(
+ &monitor_key,
+ latest_update_id,
+ true,
+ )
+ .await;
} else {
- let end = monitor.get_latest_update_id();
+ let end = latest_update_id;
let start = end.saturating_sub(self.maximum_pending_updates);
- Some((start, end))
- };
-
- if let Some((start, end)) = cleanup_range {
self.cleanup_in_range(monitor_name, start, end).await;
}
}
@@ -1263,6 +1254,7 @@ mod tests {
use crate::util::test_utils::{self, TestStore};
use crate::{check_added_monitors, check_closed_broadcast};
use bitcoin::hashes::hex::FromHex;
+ use core::cmp;
const EXPECTED_UPDATES_PER_PAYMENT: u64 = 5;
Why this scored 17/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.