Skip ChannelManager persistence after monitor update completion#4424
Skip ChannelManager persistence after monitor update completion#4424joostjager wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
Since we are pulling ChannelManager persistence into the critical path, we should avoid persisting unnecessarily. When process_pending_monitor_events processes only Completed events, the resulting state changes are all recoverable on restart and do not require persistence. Return NotifyOption from process_pending_monitor_events instead of bool, returning SkipPersistHandleEvents for Completed events and DoPersist for HTLCEvent/HolderForceClosed/CommitmentTxConfirmed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
👋 Hi! I see this is a draft PR. |
| 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.
When a ChannelMonitorUpdate is sent to the chain monitor, the ChannelManager must be persisted so that in_flight_monitor_updates stays in sync. Most callers already ensure this via PersistenceNotifierGuard, but handle_monitor_update_release (reached from the Completed event path in channel_monitor_updated) does not. Set needs_persist_flag directly in handle_new_monitor_update_locked_actions_handled_by_caller, right after calling chain_monitor.update_channel, making the invariant self-enforcing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4424 +/- ##
==========================================
- Coverage 85.91% 85.91% -0.01%
==========================================
Files 156 156
Lines 103958 103965 +7
Branches 103958 103965 +7
==========================================
+ Hits 89317 89323 +6
+ Misses 12122 12118 -4
- Partials 2519 2524 +5
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:
|
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, the resulting state changes are all recoverable on restart and do not require persistence.
Return NotifyOption from process_pending_monitor_events instead of bool, returning SkipPersistHandleEvents for Completed events and DoPersist for HTLCEvent/HolderForceClosed/CommitmentTxConfirmed.