Only mark all mon updates complete if there are no blocked updates #3907
Conversation
|
I've assigned @wpaulino as a reviewer! |
TheBlueMatt
force-pushed
the
2025-06-no-mon-compl-with-blocked-updates
branch
2 times, most recently
from
6bfe0bd to
49ad15c
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
TheBlueMatt
force-pushed
the
2025-06-no-mon-compl-with-blocked-updates
branch
2 times, most recently
from
7759b4d to
95c0696
Compare
In `handle_new_monitor_update!`, we correctly check that the channel doesn't have any blocked monitor updates pending before calling `handle_monitor_update_completion!` (which calls `Channel::monitor_updating_restored`, which in turn assumes that all generated `ChannelMonitorUpdate`s, including blocked ones, have completed). We, however, did not do the same check at several other places where we called `handle_monitor_update_completion!`. Specifically, after a monitor update completes during reload (processed via a `BackgroundEvent` or when monitor update completes async, we didn't check if there were any blocked monitor updates before completing). Here we add the missing check, as well as an assertion in `Channel::monitor_updating_restored`.
TheBlueMatt
force-pushed
the
2025-06-no-mon-compl-with-blocked-updates
branch
from
95c0696 to
ab0dda7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3907 +/- ##
==========================================
- Coverage 88.82% 88.80% -0.02%
==========================================
Files 165 165
Lines 119075 119123 +48
Branches 119075 119123 +48
==========================================
+ Hits 105769 105790 +21
- Misses 10986 11003 +17
- Partials 2320 2330 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
| @@ -8227,7 +8229,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ | |||
| { | |||
| if chan.is_awaiting_monitor_update() { | |||
| log_trace!(logger, "Channel is open and awaiting update, resuming it"); | |||
| handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); | |||
| if chan.blocked_monitor_updates_pending() == 0 { | |||
| handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); | |||
There was a problem hiding this comment.
Wouldn't we still want to call self.handle_monitor_update_completion_actions so that we can actually unblock any monitor updates that are pending blocked?
There was a problem hiding this comment.
Hmm, we don't do so in handle_new_monitor_update's base case, which I was trying to match everywhere. A few tests fail if we change it there (with almost-harmless message ordering changes, so nothing major, but we'd have to fix that), but I'm not convinced its a bug - a channel should never block itself, and the unblocking of the next monitor update in a channel has to come in from a different channel or from an event, not from the channel itself. So that other channel, once it makes progress, should always unblock us.
There was a problem hiding this comment.
Yeah that's what I figured, was just being overly-cautious in case we ever introduce such a monitor update that blocks the same channel.
There was a problem hiding this comment.
Yea, I think you're right that that change makes sense, but I don't think it makes sense as a backport and we should do it everywhere, not just in the new places, in a separate PR.
|
LGTM after squash |
In
handle_new_monitor_update!, we correctly check that the channel doesn't have any blocked monitor updates pending before callinghandle_monitor_update_completion!(which callsChannel::monitor_updating_restored, which in turn assumes that all generatedChannelMonitorUpdates, including blocked ones, have completed).We, however, did not do the same check at several other places where we called
handle_monitor_update_completion!. Specifically, after a monitor update completes during reload (processed via aBackgroundEventor when monitor update completes async, we didn't check if there were any blocked monitor updates before completing).Here we add the missing check, as well as an assertion in
Channel::monitor_updating_restored.