Skip ChannelManager persistence after monitor update completion#4424
Skip ChannelManager persistence after monitor update completion#4424joostjager wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Hi! I see this is a draft PR. |
lightning/src/ln/channelmanager.rs
Outdated
| if result == NotifyOption::SkipPersistNoEvents { | ||
| result = NotifyOption::SkipPersistHandleEvents; | ||
| } | ||
| self.channel_monitor_updated( |
There was a problem hiding this comment.
We'll need to carefully review everything this calls to make sure they set the store-required flag. Might be easier to just manually set the flag if there's anything to do in handle_post_monitor_update_chan_resume?
There was a problem hiding this comment.
By "anything to do in handle_post_monitor_update_chan_resume", do you mean specifically a new monitor update being created? Or is there more that isn't recoverable on restart?
Added a commit that sets needs_persist_flag in handle_new_monitor_update_locked_actions_handled_by_caller right after chain_monitor.update_channel to cover that case.
Have to say that certainty level instantly drops when working on this state machine 😬
There was a problem hiding this comment.
I have an alternative approach on that is more conservative in stripping out persists: https://github.com/joostjager/rust-lightning/tree/skip-cm-persist-monitor-completed-alt
Rather than skipping persistence for all MonitorEvent::Completed events unconditionally, it inspects the result of monitor_updating_restored and only skips persistence when the effect is just queuing outbound messages (commitment_signed, revoke_and_ack). But also needs to be carefully reviewed to see if we don't miss anything.
There was a problem hiding this comment.
Rather than trying to carefully thread the needs-persistence flags through, maybe we just explicitly set needs_persist_flag in channel_monitor_updated? But yea I think your second approach there makes way more sense.
There was a problem hiding this comment.
Ok, pushed that to this PR after splitting in two commits for reviewability. Now it needs a careful eye to see if the persist condition is not missing anything.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4424 +/- ##
==========================================
+ Coverage 85.94% 85.97% +0.03%
==========================================
Files 159 159
Lines 104644 104659 +15
Branches 104644 104659 +15
==========================================
+ Hits 89934 89979 +45
+ Misses 12204 12176 -28
+ Partials 2506 2504 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joostjager
force-pushed
the
skip-cm-persist-monitor-completed
branch
2 times, most recently
from
7b1c046 to
48dce43
Compare
Refactor process_pending_monitor_events to return a NotifyOption instead of a bool, allowing callers to distinguish between DoPersist, SkipPersistHandleEvents, and SkipPersistNoEvents. Both call sites in process_events_body and get_and_clear_pending_msg_events are updated accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a MonitorEvent::Completed fires and the only result of channel resumption is queuing outbound messages (commitment_signed, revoke_and_ack), skip the ChannelManager persist. This avoids an unnecessary write on every HTLC send where the monitor update for the counterparty's commitment completes. The persist decision is computed in try_resume_channel_post_monitor_update where MonitorRestoreUpdates fields are still available, and returned alongside the PostMonitorUpdateChanResume as a tuple. Persistence is requested only when state-changing operations occur (funding broadcast, channel_ready, announcement_sigs, HTLC forwards/failures/claims, update actions, or batch funding). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joostjager
force-pushed
the
skip-cm-persist-monitor-completed
branch
from
48dce43 to
b0ec5eb
Compare
3 participants
Since we are pulling ChannelManager persistence into the critical path, we should avoid persisting unnecessarily. When process_pending_monitor_events processes only Completed events, resulting state changes may be recoverable on restart and do not require persistence.