Hold in-flight monitor updates until background event processing#4377
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4377 +/- ##
==========================================
- Coverage 86.01% 86.01% -0.01%
==========================================
Files 156 156
Lines 102857 102879 +22
Branches 102857 102879 +22
==========================================
+ Hits 88476 88492 +16
- Misses 11871 11875 +4
- Partials 2510 2512 +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:
|
lightning/src/ln/channelmanager.rs
Outdated
| for event in background_events.drain(..) { | ||
| match event { | ||
| BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { | ||
| // We previously assumed background events would eventually be processed prior |
There was a problem hiding this comment.
Are you sure we need this change in this case? The whole point of RegeneratedOnStartup is that we don't care if we lose it cause it'll be regenerated when we restart.
There was a problem hiding this comment.
Yeah I misinterpreted how this specific case works.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
wpaulino
force-pushed
the
hold-in-flight-updates-until-background-event-processing
branch
from
2a15c45 to
c880d85
Compare
We previously assumed background events would eventually be processed prior to another `ChannelManager` write, so we would immediately remove all in-flight monitor updates that completed since the last `ChannelManager` serialization. This isn't always the case, so we now keep them all around until we're ready to handle them, i.e., when `process_background_events` is called. This was discovered while fuzzing `chanmon_consistency_target` on the main branch with some changes that allow it to connect blocks. It was triggered by reloading the `ChannelManager` after a monitor update completion for an outgoing HTLC, calling `ChannelManager::best_block_updated`, and reloading the `ChannelManager` once again. A test is included that provides a minimal reproduction of this case.
wpaulino
force-pushed
the
hold-in-flight-updates-until-background-event-processing
branch
from
c880d85 to
55d9ef0
Compare
3 participants
We previously assumed background events would eventually be processed prior to another
ChannelManagerwrite, so we would immediately remove all in-flight monitor updates that completed since the lastChannelManagerserialization. This isn't always the case, so we now keep them all around until we're ready to handle them, i.e., whenprocess_background_eventsis called.This was discovered while fuzzing
chanmon_consistency_targeton the main branch with some changes that allow it to connect blocks. It was triggered by reloading theChannelManagerafter a monitor update completion for an outgoing HTLC, callingChannelManager::best_block_updated, and reloading theChannelManageronce again. A test is included that provides a minimal reproduction of this case.