Fuzz reloading with a stale monitor in chanmon_consistency #3113
Conversation
TheBlueMatt
force-pushed
the
2024-04-async-monitor-fuzz
branch
from
fd0ecbd to
7446a79
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80%
==========================================
Files 119 119
Lines 97811 102825 +5014
Branches 97811 102825 +5014
==========================================
+ Hits 87883 93217 +5334
+ Misses 7364 7108 -256
+ Partials 2564 2500 -64 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
force-pushed
the
2024-04-async-monitor-fuzz
branch
2 times, most recently
from
b7a57bc to
61aef9b
Compare
fuzz/src/chanmon_consistency.rs
Outdated
| &|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }), | ||
| 0xf1 => | ||
| complete_monitor_update(&monitor_a, &chan_1_funding, | ||
| &|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }), |
There was a problem hiding this comment.
Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?
There was a problem hiding this comment.
We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.
There was a problem hiding this comment.
We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that
Right, I imagined the latter.
but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.
Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.
TheBlueMatt
force-pushed
the
2024-04-async-monitor-fuzz
branch
from
3a944d2 to
c05e508
Compare
TheBlueMatt
force-pushed
the
2024-04-async-monitor-fuzz
branch
from
c05e508 to
9d0c1a1
Compare
|
Addressed feedback. |
Unfortunately the new test now reliably fails: |
TheBlueMatt
force-pushed
the
2024-04-async-monitor-fuzz
branch
from
384d691 to
2a0c900
Compare
|
Ugh, sorry, pushed right before bed last night and didn't test enough :/ |
|
LGTM, feel free to squash IMO. |
Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the `chanmon_consistency` fuzzer. While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both synchronously and asynchronously, we need to consider a channel as unblocked based on the `ChannelManager` monitor update queue, rather than by checking the `update_id`s. Consider the case where a channel is updated, leading to a `ChannelMonitorUpdate` which completes asynchronously. The update completes, but prior to the `ChannelManager` receiving the `MonitorEvent::Completed` it generates a further `ChannelMonitorUpdate`. This second update completes synchronously. As a result, when the `MonitorEvent` is processed, the event's `monitor_update_id` is the first update, but there are no updates queued and the channel should be free to return to be unblocked. Here we fix this by looking only at the `ChannelManager` update queue, rather than the update_id of the `MonitorEvent`. While we don't anticipate many users having both synchronous and asynchronous persists in the same application, there isn't much cost to supporting it, which we do here. Found by the chanmon_consistency target.
TheBlueMatt
force-pushed
the
2024-04-async-monitor-fuzz
branch
from
2a0c900 to
920d96e
Compare
|
Squashed without further changes. |
Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the
chanmon_consistencyfuzzer.While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through
chanmon_consistencyruns.