Correct `maximum_pending_updates` of 0 in MonitorUpdatingPersister
What changed, and why it matters
This commit fixes a bug where setting a configuration value called `maximum_pending_updates` to 0 in a Lightning node persistence helper would cause the program to crash (panic) due to division by zero. The fix makes the code skip storing incremental updates when the limit is 0, which is a valid 'do not store updates' setting. It is a robustness fix rather than an exploitable security vulnerability, and the crash would only affect the node operator who configured the value to 0.
Treat as a low-severity bug fix. No immediate security response is required, but downstream users relying on `MonitorUpdatingPersister` should ensure they upgrade if they intend to set `maximum_pending_updates` to 0. Review other modulo/division uses in configuration paths for similar zero-guard issues.
Security signals we found
Integer divide-by-zero panic in configuration-dependent code path
Denial-of-service-like crash triggered by user-supplied configuration value
No evidence of memory corruption, privilege escalation, or remote trigger
Fix is defensive hardening of a public API/config option
Evidence from the diff
In MonitorUpdatingPersister::persist_monitor_update, the code used update.update_id % self.maximum_pending_updates to decide whether to persist a ChannelMonitorUpdate. If maximum_pending_updates was 0, the modulo operation panicked with an integer divide-by-zero. The patch adds a guard self.maximum_pending_updates != 0 so that when the limit is 0, updates are not persisted as incremental updates. Tests are refactored to parameterize max_pending_updates and now explicitly exercise the 0 case.
Changed components
lightning/src/util/persist.rsMonitorUpdatingPersisterChannelMonitorUpdate persistence logicInspect captured patch +34 / −30
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index e3fb86f..974c7a4 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -796,6 +796,7 @@ where
const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX;
if let Some(update) = update {
let persist_update = update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID
+ && self.maximum_pending_updates != 0
&& update.update_id % self.maximum_pending_updates != 0;
if persist_update {
let monitor_key = monitor_name.to_string();
@@ -1188,17 +1189,12 @@ mod tests {
}
// Exercise the `MonitorUpdatingPersister` with real channels and payments.
- #[test]
- fn persister_with_real_monitors() {
- // This value is used later to limit how many iterations we perform.
- let persister_0_max_pending_updates = 7;
- // Intentionally set this to a smaller value to test a different alignment.
- let persister_1_max_pending_updates = 3;
+ fn do_persister_with_real_monitors(max_pending_updates_0: u64, max_pending_updates_1: u64) {
let chanmon_cfgs = create_chanmon_cfgs(4);
let persister_0 = MonitorUpdatingPersister {
kv_store: &TestStore::new(false),
logger: &TestLogger::new(),
- maximum_pending_updates: persister_0_max_pending_updates,
+ maximum_pending_updates: max_pending_updates_0,
entropy_source: &chanmon_cfgs[0].keys_manager,
signer_provider: &chanmon_cfgs[0].keys_manager,
broadcaster: &chanmon_cfgs[0].tx_broadcaster,
@@ -1207,7 +1203,7 @@ mod tests {
let persister_1 = MonitorUpdatingPersister {
kv_store: &TestStore::new(false),
logger: &TestLogger::new(),
- maximum_pending_updates: persister_1_max_pending_updates,
+ maximum_pending_updates: max_pending_updates_1,
entropy_source: &chanmon_cfgs[1].keys_manager,
signer_provider: &chanmon_cfgs[1].keys_manager,
broadcaster: &chanmon_cfgs[1].tx_broadcaster,
@@ -1256,17 +1252,17 @@ mod tests {
assert_eq!(mon.get_latest_update_id(), $expected_update_id);
let monitor_name = mon.persistence_key();
- assert_eq!(
- KVStoreSync::list(
- &*persister_0.kv_store,
- CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
- &monitor_name.to_string()
- )
- .unwrap()
- .len() as u64,
- mon.get_latest_update_id() % persister_0_max_pending_updates,
- "Wrong number of updates stored in persister 0",
+ let expected_updates = if max_pending_updates_0 == 0 {
+ 0
+ } else {
+ mon.get_latest_update_id() % max_pending_updates_0
+ };
+ let update_list = KVStoreSync::list(
+ &*persister_0.kv_store,
+ CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
+ &monitor_name.to_string(),
);
+ assert_eq!(update_list.unwrap().len() as u64, expected_updates, "persister 0");
}
persisted_chan_data_1 =
persister_1.read_all_channel_monitors_with_updates().unwrap();
@@ -1274,17 +1270,17 @@ mod tests {
for (_, mon) in persisted_chan_data_1.iter() {
assert_eq!(mon.get_latest_update_id(), $expected_update_id);
let monitor_name = mon.persistence_key();
- assert_eq!(
- KVStoreSync::list(
- &*persister_1.kv_store,
- CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
- &monitor_name.to_string()
- )
- .unwrap()
- .len() as u64,
- mon.get_latest_update_id() % persister_1_max_pending_updates,
- "Wrong number of updates stored in persister 1",
+ let expected_updates = if max_pending_updates_1 == 0 {
+ 0
+ } else {
+ mon.get_latest_update_id() % max_pending_updates_1
+ };
+ let update_list = KVStoreSync::list(
+ &*persister_1.kv_store,
+ CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
+ &monitor_name.to_string(),
);
+ assert_eq!(update_list.unwrap().len() as u64, expected_updates, "persister 1");
}
};
}
@@ -1302,7 +1298,7 @@ mod tests {
// Send a few more payments to try all the alignments of max pending updates with
// updates for a payment sent and received.
let mut sender = 0;
- for i in 3..=persister_0_max_pending_updates * 2 {
+ for i in 3..=max_pending_updates_0 * 2 {
let receiver;
if sender == 0 {
sender = 1;
@@ -1345,11 +1341,19 @@ mod tests {
check_added_monitors!(nodes[1], 1);
// Make sure everything is persisted as expected after close.
+ // We always send at least two payments, and loop up to max_pending_updates_0 * 2.
check_persisted_data!(
- persister_0_max_pending_updates * 2 * EXPECTED_UPDATES_PER_PAYMENT + 1
+ cmp::max(2, max_pending_updates_0 * 2) * EXPECTED_UPDATES_PER_PAYMENT + 1
);
}
+ #[test]
+ fn persister_with_real_monitors() {
+ do_persister_with_real_monitors(7, 3);
+ do_persister_with_real_monitors(0, 1);
+ do_persister_with_real_monitors(4, 2);
+ }
+
// Test that if the `MonitorUpdatingPersister`'s can't actually write, trying to persist a
// monitor or update with it results in the persister returning an UnrecoverableError status.
#[test]
Why this scored 29/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.