fuzz: Use immediate task runner to increase fuzz stability #31841

Leaking a scheduler with a non-empty queue from the fuzz initialization phase into the fuzz target execution phase is problematic, because it messes with coverage data. This in turn is problematic, because it leads to:

• Decrease in fuzz target execution stability (non-determinism when running the fuzz target).
• Decrease in fuzz input merge stability (non-determinism when selecting a minimum set of fuzz input to reach maximum coverage), which leads to qa-assets bloat.

Fix one such issue. Tracking issue: #29018

Can be tested via: RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake $PWD/../b-c-qa-assets/fuzz_corpora/ partially_downloaded_block.

The failure is non-deterministic (obviously) and will show coverage in validation signals such as UpdatedBlockTip before this change and will have this one fixed after this change.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31841.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Actually, this is incomplete. It will handle the coverage generated in the notification itself (f()), but there is no guarantee that the re-lock is handled as well when the context ends happens before fuzz target execution starts after init. Relevant scope for the re-lock:

https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/scheduler.cpp#L56-L61

The change here should be an improvement, but I wonder if there is a trivial one-line complete solution.

On alternative would be to inline the scheduler in tests (make it synchronous), but the patch would be larger.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin/bitcoin/runs/37046883391}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

On alternative would be to inline the scheduler in tests (make it synchronous), but the patch would be larger.

Looks like this will lead to deadlocks due to lock order inversions between cs_main and cs_wallet in the unit tests. I'll try to apply it to the fuzz tests only. If there are still issues in this pull, or in the future, they could be worked around for the affected fuzz target.

CI passed and coverage is as expected:

• This pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/faad87189015e559/733f70b21be22925/fuzz.coverage/index.html
• Before this pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/6a46be75c43e6489/733f70b21be22925/fuzz.coverage/index.html

Concept ACK

(rebased without changes to make testing easier)

cc @marcofleon

ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f

Seems fine to me to use ImmediateTaskRunner for fuzz tests. I checked stability for partially_downloaded_block and saw an increase from 90% to 94%.

Am I correct in saying that this change makes calling something like SyncWithValidationInterfaceQueue() in fuzz targets effectively a no-op?

edit: quick nit, I think the PR title means to say "use immediate task runner" yes?

Am I correct in saying that this change makes calling something like SyncWithValidationInterfaceQueue() in fuzz targets effectively a no-op?

Yes, they could be removed now. Happy to include that here, but they should also be harmless.

Happy to include that here

No need, just confirming for my own understanding.

Code review ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f

The only downside to this is that we diverge from the production setup and e.g. bugs that only occur when fuzzing and using the SerialTaskRunner would be masked by this change. We could consider having a compile or runtime option to enable the serial runner and run both versions in CI.

The only downside to this is that we diverge from the production setup and e.g. bugs that only occur when fuzzing and using the SerialTaskRunner would be masked by this change.

Right, but I wonder what type of bug this could possibly uncover, given that the fuzz targets are mostly single threaded and require some kind of sync (like SyncWithValidationInterfaceQueue) when they use the task runner.

Obviously, bugs in CScheduler itself could be missed. Though, if we are worried about that, a CScheduler fuzz target should be added. Unrelated to this pull, if we want to test multiple threads to detect lock issues and data races, I presume we'd have to add a specific fuzz target for those as well and add a tsan-fuzz CI task.

Post-merge ACK