-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't drop ChannelMonitor Events until they're processed #2369
Don't drop ChannelMonitor Events until they're processed #2369
Conversation
e99e15f
to
7b4ea82
Compare
Rebased. |
for funding_txo in mons_to_process { | ||
let mut ev; | ||
super::channelmonitor::process_events_body!( | ||
self.monitors.read().unwrap().get(&funding_txo).map(|m| &m.monitor), ev, handler(ev).await); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this first argument ever be None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently I don't think so, but in the future we probably want to support removing monitors after they're done with all their processing (and probably will do so automatically). Thus, we should handle the case where the monitor has dissapeared on us (we don't hold a lock through the loop anyway, so its good practice to not rely on any assumptions about the underlying data changing).
669280e
to
3fc31fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Largely LGTM
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2369 +/- ##
==========================================
+ Coverage 90.32% 90.95% +0.63%
==========================================
Files 106 106
Lines 54965 60536 +5571
Branches 54965 60536 +5571
==========================================
+ Hits 49647 55063 +5416
- Misses 5318 5473 +155
☔ View full report in Codecov by Sentry. |
I think I've addressed all outstanding feeback, lmk if I should squash. |
SGTM |
We currently assume the owner of `ChannelMonitor`s won't persist the `ChannelMonitor` while `Event`s are being processed. This is fine, except (a) its generally hard to do so and (b) the `ChainMonitor` doesn't even do this. Thus, in rare cases, a user could begin processing events which are, generated by connecting a transaction or a new best-block, take some time to do so, and while doing so process a further chain event, causing persistece. This could lose the event being processed alltogether, which could lose the user funds. This should be very rare, but may have been made slightly more reachable with (a) the async event processing making it more common to do networking in event handling, (b) the new future generation in the `ChainMonitor`, which now wakes the `background-processor` directly when chain actions happen on the `ChainMonitor`.
2d9e0fd
to
4206e71
Compare
Squashed without diff: $ git diff-tree -U1 2d9e0fdd3 4206e7119
$ |
We currently assume the owner of
ChannelMonitor
s won't persistthe
ChannelMonitor
whileEvent
s are being processed. This isfine, except (a) its generally hard to do so and (b) the
ChainMonitor
doesn't even do this.Thus, in rare cases, a user could begin processing events which
are, generated by connecting a transaction or a new best-block,
take some time to do so, and while doing so process a further chain
event, causing persistece. This could lose the event being
processed alltogether, which could lose the user funds.
This should be very rare, but may have been made slightly more
reachable with (a) the async event processing making it more
common to do networking in event handling, (b) the new future
generation in the
ChainMonitor
, which now wakes thebackground-processor
directly when chain actions happen on theChainMonitor
.Based on #2367