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

1. 
   maflcko commented at 1:42 pm on February 11, 2025: member

   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.

2. 
   DrahtBot commented at 1:42 pm on February 11, 2025: contributor

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

   Code Coverage & Benchmarks

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	dergoegge

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

3. DrahtBot added the label Tests on Feb 11, 2025
4. maflcko force-pushed on Feb 11, 2025
5. 
   maflcko commented at 4:47 pm on February 11, 2025: member

   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.

6. maflcko renamed this:
   test: Clear scheduler after init to increase fuzz stability
   test: Use serial task runner to increase fuzz stability
   on Feb 11, 2025
7. DrahtBot renamed this:
   test: Use serial task runner to increase fuzz stability
   test: Use serial task runner to increase fuzz stability
   on Feb 11, 2025
8. maflcko force-pushed on Feb 11, 2025
9. maflcko force-pushed on Feb 11, 2025
10. maflcko force-pushed on Feb 11, 2025
11. maflcko force-pushed on Feb 11, 2025
12. DrahtBot added the label CI failed on Feb 11, 2025
13. 
    DrahtBot commented at 7:27 pm on February 11, 2025: contributor

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

    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.

14. maflcko renamed this:
    test: Use serial task runner to increase fuzz stability
    fuzz: Use serial task runner to increase fuzz stability
    on Feb 11, 2025
15. 
    maflcko commented at 7:47 pm on February 11, 2025: member

    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.

16. maflcko force-pushed on Feb 11, 2025
17. maflcko force-pushed on Feb 11, 2025
18. DrahtBot removed the label CI failed on Feb 11, 2025
19. 
    maflcko commented at 12:44 pm on February 13, 2025: member

    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
20. fuzz: Use serial task runner to increase fuzz stability fa06513380
21. maflcko force-pushed on Feb 19, 2025
22. 
    dergoegge commented at 10:36 am on February 19, 2025: member

    Concept ACK
23. 
    maflcko commented at 10:38 am on February 19, 2025: member

    (rebased without changes to make testing easier)

Contributors
maflcko DrahtBot dergoegge

Labels
Tests