← Watch feed
Avoid persisting ChannelManager on handle_tx_* errors
Public commit record
85/100 · Strong
What the developer wrote
Authored by Wilmer Paulino
Avoid persisting ChannelManager on handle_tx_* errors
These errors will only ever affect our in-memory state, so there's no
need to persist the ChannelManager when we come across one. Note that
`tx_abort` is not included here because there is a possibility we force
close the channel, which we should persist.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This change adjusts when LDK saves its channel state to disk while processing certain transaction-sharing messages during channel setup. Previously, an error in handling these messages would trigger a disk save even though the error only affected temporary in-memory state. Now the code skips that unnecessary save, while still saving when something important actually happens (like completing a signing session). The commit message frames this as a cleanup/optimization, not a security fix.
Recommended action
Treat as a defensive hardening/optimization commit. Review whether the assumption that these specific errors never affect persisted state holds across all error variants, and ensure the debug_assert! invariants are also enforced in release builds or via tests. No urgent security response is indicated by the commit itself.
Security signals we found
01
Change in persistence behavior on error paths
02
Addition of debug_assert! that error does not close channel
03
Explicit note that tx_abort is excluded because it may force-close
04
No mention of vulnerability, CVE, or security bug in commit message
Evidence from the diff
The patch modifies lightning/src/ln/channelmanager.rs so that handle_tx_add_input, handle_tx_add_output, handle_tx_remove_input, handle_tx_remove_output, and handle_tx_complete use a new PersistenceNotifierGuard::manually_notify helper. The internal message handlers now return Result<(), MsgHandleErrInternal> instead of Result<NotifyOption, MsgHandleErrInternal>. On error paths, the code no longer sets needs_persist_flag or notifies the persistence notifier; successful signing-session outcomes still explicitly persist. A debug_assert! checks that errors from these handlers do not close the channel. The commit explicitly excludes tx_abort because it may force-close and therefore must persist.
Changed components
lightning/src/ln/channelmanager.rsInteractive transaction message handlers (handle_tx_add_input, handle_tx_add_output, handle_tx_remove_input, handle_tx_remove_output, handle_tx_complete)PersistenceNotifierGuardInspect captured patch +44 / −43
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 930f5fe..c9fbdc7 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2935,6 +2935,23 @@ impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> {
Self::optionally_notify(cm, || -> NotifyOption { NotifyOption::DoPersist })
}
+ fn manually_notify<F: FnOnce(), C: AChannelManager>(
+ cm: &'a C, f: F,
+ ) -> PersistenceNotifierGuard<'a, impl FnOnce() -> NotifyOption> {
+ let read_guard = cm.get_cm().total_consistency_lock.read().unwrap();
+ let force_notify = cm.get_cm().process_background_events();
+
+ PersistenceNotifierGuard {
+ event_persist_notifier: &cm.get_cm().event_persist_notifier,
+ needs_persist_flag: &cm.get_cm().needs_persist_flag,
+ should_persist: Some(move || {
+ f();
+ force_notify
+ }),
+ _read_guard: read_guard,
+ }
+ }
+
fn optionally_notify<F: FnOnce() -> NotifyOption, C: AChannelManager>(
cm: &'a C, persist_check: F,
) -> PersistenceNotifierGuard<'a, impl FnOnce() -> NotifyOption> {
@@ -11336,7 +11353,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
>(
&self, counterparty_node_id: &PublicKey, channel_id: ChannelId,
tx_msg_handler: HandleTxMsgFn,
- ) -> Result<NotifyOption, MsgHandleErrInternal> {
+ ) -> Result<(), MsgHandleErrInternal> {
let per_peer_state = self.per_peer_state.read().unwrap();
let peer_state_mutex = per_peer_state.get(counterparty_node_id).ok_or_else(|| {
debug_assert!(false);
@@ -11351,7 +11368,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
Ok(msg_send) => {
let msg_send_event = msg_send.into_msg_send_event(*counterparty_node_id);
peer_state.pending_msg_events.push(msg_send_event);
- Ok(NotifyOption::SkipPersistHandleEvents)
+ Ok(())
},
Err(InteractiveTxMsgError {
err,
@@ -11389,7 +11406,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn internal_tx_add_input(
&self, counterparty_node_id: PublicKey, msg: &msgs::TxAddInput,
- ) -> Result<NotifyOption, MsgHandleErrInternal> {
+ ) -> Result<(), MsgHandleErrInternal> {
self.internal_tx_msg(&counterparty_node_id, msg.channel_id, |channel: &mut Channel<SP>| {
channel.tx_add_input(msg, &self.logger)
})
@@ -11397,7 +11414,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn internal_tx_add_output(
&self, counterparty_node_id: PublicKey, msg: &msgs::TxAddOutput,
- ) -> Result<NotifyOption, MsgHandleErrInternal> {
+ ) -> Result<(), MsgHandleErrInternal> {
self.internal_tx_msg(&counterparty_node_id, msg.channel_id, |channel: &mut Channel<SP>| {
channel.tx_add_output(msg, &self.logger)
})
@@ -11405,7 +11422,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn internal_tx_remove_input(
&self, counterparty_node_id: PublicKey, msg: &msgs::TxRemoveInput,
- ) -> Result<NotifyOption, MsgHandleErrInternal> {
+ ) -> Result<(), MsgHandleErrInternal> {
self.internal_tx_msg(&counterparty_node_id, msg.channel_id, |channel: &mut Channel<SP>| {
channel.tx_remove_input(msg, &self.logger)
})
@@ -11413,7 +11430,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn internal_tx_remove_output(
&self, counterparty_node_id: PublicKey, msg: &msgs::TxRemoveOutput,
- ) -> Result<NotifyOption, MsgHandleErrInternal> {
+ ) -> Result<(), MsgHandleErrInternal> {
self.internal_tx_msg(&counterparty_node_id, msg.channel_id, |channel: &mut Channel<SP>| {
channel.tx_remove_output(msg, &self.logger)
})
@@ -11421,7 +11438,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn internal_tx_complete(
&self, counterparty_node_id: PublicKey, msg: &msgs::TxComplete,
- ) -> Result<NotifyOption, MsgHandleErrInternal> {
+ ) -> Result<(), MsgHandleErrInternal> {
let per_peer_state = self.per_peer_state.read().unwrap();
let peer_state_mutex = per_peer_state.get(&counterparty_node_id).ok_or_else(|| {
debug_assert!(false);
@@ -11434,15 +11451,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
let chan = chan_entry.get_mut();
match chan.tx_complete(msg, &self.fee_estimator, &self.logger) {
Ok(tx_complete_result) => {
- let mut persist = NotifyOption::SkipPersistNoEvents;
-
if let Some(interactive_tx_msg_send) =
tx_complete_result.interactive_tx_msg_send
{
let msg_send_event =
interactive_tx_msg_send.into_msg_send_event(counterparty_node_id);
peer_state.pending_msg_events.push(msg_send_event);
- persist = NotifyOption::SkipPersistHandleEvents;
};
if let Some(unsigned_transaction) = tx_complete_result.event_unsigned_tx {
@@ -11456,7 +11470,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
None,
));
// We have a successful signing session that we need to persist.
- persist = NotifyOption::DoPersist;
+ self.needs_persist_flag.store(true, Ordering::Release);
+ self.event_persist_notifier.notify()
}
if let Some(FundingTxSigned {
@@ -11501,10 +11516,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
// We have a successful signing session that we need to persist.
- persist = NotifyOption::DoPersist;
+ self.needs_persist_flag.store(true, Ordering::Release);
+ self.event_persist_notifier.notify()
}
- Ok(persist)
+ Ok(())
},
Err(InteractiveTxMsgError {
err,
@@ -16182,62 +16198,47 @@ impl<
}
fn handle_tx_add_input(&self, counterparty_node_id: PublicKey, msg: &msgs::TxAddInput) {
- let _persistence_guard = PersistenceNotifierGuard::optionally_notify(self, || {
+ let _persistence_guard = PersistenceNotifierGuard::manually_notify(self, || {
let res = self.internal_tx_add_input(counterparty_node_id, msg);
- let persist = match &res {
- Err(_) => NotifyOption::DoPersist,
- Ok(persist) => *persist,
- };
+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
let _ = self.handle_error(res, counterparty_node_id);
- persist
+ self.event_persist_notifier.notify();
});
}
fn handle_tx_add_output(&self, counterparty_node_id: PublicKey, msg: &msgs::TxAddOutput) {
- let _persistence_guard = PersistenceNotifierGuard::optionally_notify(self, || {
+ let _persistence_guard = PersistenceNotifierGuard::manually_notify(self, || {
let res = self.internal_tx_add_output(counterparty_node_id, msg);
- let persist = match &res {
- Err(_) => NotifyOption::DoPersist,
- Ok(persist) => *persist,
- };
+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
let _ = self.handle_error(res, counterparty_node_id);
- persist
+ self.event_persist_notifier.notify();
});
}
fn handle_tx_remove_input(&self, counterparty_node_id: PublicKey, msg: &msgs::TxRemoveInput) {
- let _persistence_guard = PersistenceNotifierGuard::optionally_notify(self, || {
+ let _persistence_guard = PersistenceNotifierGuard::manually_notify(self, || {
let res = self.internal_tx_remove_input(counterparty_node_id, msg);
- let persist = match &res {
- Err(_) => NotifyOption::DoPersist,
- Ok(persist) => *persist,
- };
+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
let _ = self.handle_error(res, counterparty_node_id);
- persist
+ self.event_persist_notifier.notify();
});
}
fn handle_tx_remove_output(&self, counterparty_node_id: PublicKey, msg: &msgs::TxRemoveOutput) {
- let _persistence_guard = PersistenceNotifierGuard::optionally_notify(self, || {
+ let _persistence_guard = PersistenceNotifierGuard::manually_notify(self, || {
let res = self.internal_tx_remove_output(counterparty_node_id, msg);
- let persist = match &res {
- Err(_) => NotifyOption::DoPersist,
- Ok(persist) => *persist,
- };
+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
let _ = self.handle_error(res, counterparty_node_id);
- persist
+ self.event_persist_notifier.notify();
});
}
fn handle_tx_complete(&self, counterparty_node_id: PublicKey, msg: &msgs::TxComplete) {
- let _persistence_guard = PersistenceNotifierGuard::optionally_notify(self, || {
+ let _persistence_guard = PersistenceNotifierGuard::manually_notify(self, || {
let res = self.internal_tx_complete(counterparty_node_id, msg);
- let persist = match &res {
- Err(_) => NotifyOption::DoPersist,
- Ok(persist) => *persist,
- };
+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
let _ = self.handle_error(res, counterparty_node_id);
- persist
+ self.event_persist_notifier.notify();
});
}
Risk score
Our methodology →Why this scored 34/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.