Skip `LSPS5ServiceHandler` persistence if unnecessary
What changed, and why it matters
This commit is a performance and robustness improvement for the LSPS5 service handler in rust-lightning. It adds a 'needs_persist' flag so that peer state is only written to disk when something has actually changed, rather than on every persistence call. It also handles the case where a peer's state has already been dropped, returning success instead of an error. There is no direct evidence in the commit or supplied references that this fixes an active security vulnerability.
No immediate security action required. Treat as a normal code-quality/performance patch. Monitor the added TODO regarding removal of pruned peer state entries from the KVStore, as incomplete cleanup could become a minor operational or privacy concern over time.
Security signals we found
Change reduces unnecessary disk writes and error returns for missing peer state
Adds failure handling to re-mark state as needing persistence if async write fails
No explicit security claim in commit message or diff
TODO comment indicates incomplete cleanup of pruned peer state from KVStore
Evidence from the diff
The change introduces a needs_persist: bool field to PeerState in lightning-liquidity/src/lsps5/service.rs. Mutating accessors (webhook_mut, webhooks_mut, register_webhook, remove_webhook, reset_notification_state) set this flag. persist() now only iterates over peers where needs_persist is true. persist_peer takes a write lock, returns Ok(()) if the peer entry is absent or already persisted, clears the flag before writing, and restores the flag if the async write fails. PeerState::default() sets needs_persist = true so new empty states are written once. The field is marked as a static false value for TLV serialization, preserving backward compatibility. A TODO is added about removing pruned peer state entries from the KVStore.
Changed components
lightning-liquidity/src/lsps5/service.rsLSPS5ServiceHandler persistence logicPeerState struct and serializationInspect captured patch +48 / −13
diff --git a/lightning-liquidity/src/lsps5/service.rs b/lightning-liquidity/src/lsps5/service.rs
index a2dee15..5753dc8 100644
--- a/lightning-liquidity/src/lsps5/service.rs
+++ b/lightning-liquidity/src/lsps5/service.rs
@@ -201,16 +201,21 @@ where
&self, counterparty_node_id: PublicKey,
) -> Result<(), lightning::io::Error> {
let fut = {
- let outer_state_lock = self.per_peer_state.read().unwrap();
- let encoded = match outer_state_lock.get(&counterparty_node_id) {
+ let mut outer_state_lock = self.per_peer_state.write().unwrap();
+ let encoded = match outer_state_lock.get_mut(&counterparty_node_id) {
None => {
- let err = lightning::io::Error::new(
- lightning::io::ErrorKind::Other,
- "Failed to get peer entry",
- );
- return Err(err);
+ // We dropped the peer state by now.
+ return Ok(());
+ },
+ Some(entry) => {
+ if !entry.needs_persist {
+ // We already have persisted otherwise by now.
+ return Ok(());
+ } else {
+ entry.needs_persist = false;
+ entry.encode()
+ }
},
- Some(entry) => entry.encode(),
};
let key = counterparty_node_id.to_string();
@@ -223,7 +228,14 @@ where
)
};
- fut.await
+ fut.await.map_err(|e| {
+ self.per_peer_state
+ .write()
+ .unwrap()
+ .get_mut(&counterparty_node_id)
+ .map(|p| p.needs_persist = true);
+ e
+ })
}
pub(crate) async fn persist(&self) -> Result<(), lightning::io::Error> {
@@ -232,7 +244,10 @@ where
// time.
let need_persist: Vec<PublicKey> = {
let outer_state_lock = self.per_peer_state.read().unwrap();
- outer_state_lock.iter().filter_map(|(k, v)| Some(*k)).collect()
+ outer_state_lock
+ .iter()
+ .filter_map(|(k, v)| if v.needs_persist { Some(*k) } else { None })
+ .collect()
};
for counterparty_node_id in need_persist.into_iter() {
@@ -259,6 +274,7 @@ where
// Don't prune clients with open channels
return true;
}
+ // TODO: Remove peer state entry from the KVStore
!peer_state.prune_stale_webhooks(now)
});
*last_pruning = Some(now);
@@ -289,6 +305,7 @@ where
webhook.url = params.webhook.clone();
webhook.last_used = now;
webhook.last_notification_sent = None;
+ peer_state.needs_persist |= true;
}
} else {
if num_webhooks >= self.config.max_webhooks_per_client as usize {
@@ -649,14 +666,18 @@ where
}
}
-#[derive(Debug, Default)]
+#[derive(Debug)]
pub(crate) struct PeerState {
webhooks: Vec<(LSPS5AppName, Webhook)>,
+ needs_persist: bool,
}
impl PeerState {
fn webhook_mut(&mut self, name: &LSPS5AppName) -> Option<&mut Webhook> {
- self.webhooks.iter_mut().find_map(|(n, h)| if n == name { Some(h) } else { None })
+ let res =
+ self.webhooks.iter_mut().find_map(|(n, h)| if n == name { Some(h) } else { None });
+ self.needs_persist |= true;
+ res
}
fn webhooks(&self) -> &Vec<(LSPS5AppName, Webhook)> {
@@ -664,7 +685,9 @@ impl PeerState {
}
fn webhooks_mut(&mut self) -> &mut Vec<(LSPS5AppName, Webhook)> {
- &mut self.webhooks
+ let res = &mut self.webhooks;
+ self.needs_persist |= true;
+ res
}
fn webhooks_len(&self) -> usize {
@@ -684,6 +707,7 @@ impl PeerState {
}
self.webhooks.push((name, hook));
+ self.needs_persist |= true;
}
fn remove_webhook(&mut self, name: &LSPS5AppName) -> bool {
@@ -696,6 +720,7 @@ impl PeerState {
false
}
});
+ self.needs_persist |= true;
removed
}
@@ -703,6 +728,7 @@ impl PeerState {
for (_, h) in self.webhooks.iter_mut() {
h.last_notification_sent = None;
}
+ self.needs_persist |= true;
}
// Returns whether the entire state is empty and can be pruned.
@@ -715,6 +741,15 @@ impl PeerState {
}
}
+impl Default for PeerState {
+ fn default() -> Self {
+ let webhooks = Vec::new();
+ let needs_persist = true;
+ Self { webhooks, needs_persist }
+ }
+}
+
impl_writeable_tlv_based!(PeerState, {
(0, webhooks, required_vec),
+ (_unused, needs_persist, (static_value, false)),
});
Why this scored 19/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.