Remove Completed from Watch trait, making updates always-async#4451
Draft
joostjager wants to merge 1 commit intolightningdevkit:mainfrom
Draft
Remove Completed from Watch trait, making updates always-async#4451joostjager wants to merge 1 commit intolightningdevkit:mainfrom
joostjager wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
The Watch trait previously shared ChannelMonitorUpdateStatus (with Completed, InProgress, UnrecoverableError variants) with the Persist trait. This meant ChannelManager had to handle both synchronous completion and asynchronous completion, including a runtime check (monitor_update_type atomic) to ensure they weren't mixed. This commit makes the Watch trait always-async: watch_channel returns Result<(), ()> and update_channel returns (). ChainMonitor maps Persist::Completed onto an immediately-queued MonitorEvent::Completed so the channel unblocks on the next event processing round. When Persist returns Completed but prior async updates are still pending, no event is emitted since the prior updates' eventual completion via channel_monitor_updated will cover this update too. This allows removing from ChannelManager: - The WatchUpdateStatus enum (and former ChannelMonitorUpdateStatus usage in Watch) - The monitor_update_type atomic and its mode-mixing checks - handle_monitor_update_res (was just a log, inlined) - handle_post_close_monitor_update (trivial wrapper, inlined) - handle_new_monitor_update_with_status (sync completion path) AI tools were used in preparing this commit.
|
👋 Hi! I see this is a draft PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The complexity of maintaining both synchronous and asynchronous monitor update completion paths in
ChannelManagerhas become increasingly difficult to reason about. Each new feature touching monitor updates must account for both modes, and subtle bugs have emerged from the interplay.This PR removes the sync path by making the
Watch-ChannelManagerinterface always-async.ChainMonitormapsPersist::Completedonto an immediately-queuedMonitorEvent::Completed, so channels unblock on the next event processing round rather than inline. ThePersisttrait andChannelMonitorUpdateStatusenum are unchanged.