contrib: Add deterministic-fuzz-coverage

maflcko commented at 4:23 pm on February 10, 2025: member

The goal of this script is to detect and debug the remaining fuzz determinism and stability issues (https://github.com/bitcoin/bitcoin/issues/29018).

DrahtBot commented at 4:23 pm on February 10, 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/31836.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	marcofleon, brunoerg
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.

DrahtBot added the label Scripts and tools on Feb 10, 2025

brunoerg commented at 4:24 pm on February 10, 2025: contributor

Concept ACK on having a script to check fuzz stability issues

maflcko commented at 4:29 pm on February 10, 2025: member

cc @marcofleon @dergoegge (Checking in to see if you have something similar already?)

marcofleon commented at 7:15 pm on February 10, 2025: contributor

Nice. We do have something similar, although it could likely be refined more. You can see it here. It should show all the places where hit count between two coverage reports differs.

I generated the json files with

0llvm-cov export ./fuzzbuild/src/test/fuzz/fuzz -instr-profile=coverage.profdata > coverage.json

and then run it like this

0python3 compare_coverage.py p2p_headers_presync.json p2p_headers_presync2.json

I ran both scripts on p2p_headers_presync (which I remembered has some subpar stability) and am seeing a difference in results. This PR’s script is saying the harness passes the coverage test while the one I linked is showing differences in hit count. I can take a look some more tomorrow.

maflcko force-pushed on Feb 11, 2025

maflcko commented at 11:17 am on February 11, 2025: member

This PR’s script is saying the harness passes the coverage test while the one I linked is showing differences in hit count.

Thanks, good catch! Fixed off-by-one

maflcko commented at 11:32 am on February 11, 2025: member

Nice. We do have something similar, although it could likely be refined more. You can see it here. It should show all the places where hit count between two coverage reports differs.

Nice. Happy to pull it in here, but I think it is similar to diff --unified, so I used that here for now. Not sure if your script handles sub-line regions, but in this pull they can trivally be enabled by adding the -show-line-counts-or-regions arg to llvm-cov.

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

I’ve created #31841 to make it easier to test this on a real patch.

marcofleon commented at 4:49 pm on February 11, 2025: contributor

Is there a disadvantage to running the harness twice using -runs=1 or even more and then comparing the coverage reports? Vs going through the inputs one by one.

Separately (but maybe related?), I ran this script again on p2p_headers_presync and there was no failure. If you look at these two coverage reports and go to line 1126, you’ll see the different hit counts:

https://marcofleon.github.io/coverage/p2p_headers_presync/coverage/root/bitcoin/src/net_processing.cpp.html https://marcofleon.github.io/coverage/p2p_headers_presync2/coverage/root/bitcoin/src/net_processing.cpp.html

Using your script, should that not show up on at least one of the inputs?

To reproduce:

0LLVM_PROFILE_FILE="p2pcoverage.profraw" FUZZ=p2p_headers_presync ./fuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/p2p_headers_presync -runs=1
1llvm-profdata merge --sparse p2pcoverage.profraw -o p2pcoverage.profdata 
2llvm-cov show ./fuzzbuild/src/test/fuzz/fuzz -instr-profile=p2pcoverage.profdata -format=html -output-dir=p2p_headers_presync

edit: I guess if the target is unstable then maybe it wouldn’t fail every time. That’s also why I was thinking about executing the tests using -runs=n and then comparing from there.

maflcko commented at 4:58 pm on February 11, 2025: member

Is there a disadvantage to running the harness twice using -runs=1 or even more and then comparing the coverage reports? Vs going through the inputs one by one.

I am thinking that sometimes the non-determinism is specific to one input. For example, if a piece of code is completely unreachable by one input, it will never be non-deterministic, so running over that seed to reproduce the non-determinism is just busy-work.

edit: But I guess there could also be non-determinism when running over all fuzz inputs combined, and only then. Maybe I should add this to the script as well?

edit: I guess if the target is unstable then maybe it wouldn’t fail every time. That’s also why I was thinking about executing the tests using -runs=n and then comparing from there.

Yes, non-determinism is non-deterministic. Though, I can add an option to be able to set runs=2, and possibly another setting to focus on one fuzz input file?

in contrib/devtools/README.md:22 in fa517fe3a9 outdated

17+-fcoverage-mapping'`. Both llvm-profdata and llvm-cov must be installed. Also,
18+the qa-assets repository must have been cloned. Finally, a fuzz target has to
19+be picked before running the tool:
20+
21+```
22+RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/corpora-dir fuzz_target_name )

marcofleon commented at 4:58 pm on February 11, 2025:

Accidental parentheses at the end here?

maflcko force-pushed on Feb 11, 2025

maflcko commented at 9:00 pm on February 11, 2025: member

Fixed the typo. I’ll add the other features and a fix for the m_next_inv_to_inbounds non-determinism that you mentioned in a follow-up, if that is all right?

maflcko force-pushed on Feb 12, 2025

maflcko commented at 8:30 am on February 12, 2025: member

Went ahead and added the other check (as well as comments to explain the reasoning). I hope this addresses all outstanding feedback.

marcofleon commented at 4:22 pm on February 13, 2025: contributor

Thanks for adding that, I think it makes the script more complete. The only thing missing is the runs=1 arg in run_single for when the entry is the full corpus dir. Right now it just runs continuously. But other than that, lgtm.

maflcko commented at 4:29 pm on February 13, 2025: member

Ah, I didn’t test with libFuzzer. I guess to support that I’ll have to port https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/test/fuzz/test_runner.py#L170

I’ll do that tomorrow.

contrib: Add deterministic-fuzz-coverage fa3e409c9a

maflcko force-pushed on Feb 14, 2025

maflcko commented at 9:50 am on February 14, 2025: member

Force pushed to add support for executables linked with libFuzzer

dergoegge commented at 11:53 am on February 17, 2025: member

Concept ACK

marcofleon commented at 1:31 pm on February 17, 2025: contributor

Tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d

DrahtBot requested review from brunoerg on Feb 17, 2025

DrahtBot requested review from dergoegge on Feb 17, 2025

brunoerg approved

brunoerg commented at 1:52 pm on February 17, 2025: contributor

tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d

0...
1--- /Users/brunogarcia/projects/bitcoin-core-dev/build_fuzz/fuzz_det_cov.show.0.txt       2025-02-17 10:47:20
2+++ /Users/brunogarcia/projects/bitcoin-core-dev/build_fuzz/fuzz_det_cov.show.1.txt       2025-02-17 10:47:20
3...
4The coverage was not determinstic between runs.
5The fuzz target input was /Users/brunogarcia/projects/bitcoin-core-dev/qa-assets/fuzz_corpora/coinselection/006855e77c95d6d4b21ae19a32eb9b21346c2cc5.
6Exiting.

in contrib/devtools/README.md:22 in fa3e409c9a

17+-fcoverage-mapping'`. Both llvm-profdata and llvm-cov must be installed. Also,
18+the qa-assets repository must have been cloned. Finally, a fuzz target has to
19+be picked before running the tool:
20+
21+```
22+RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/corpora-dir fuzz_target_name

brunoerg commented at 1:53 pm on February 17, 2025:

nit: maybe could mention fuzz_corpora directly

0RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name

brunoerg commented at 1:59 pm on February 17, 2025: contributor

Also, worths adding a .gitignore for the tool (especially for the target folder)?

maflcko commented at 2:07 pm on February 17, 2025: member

Thanks for the two nits. I’ll address them in one of the related follow-ups that will touch this area anyway.

fanquake merged this on Feb 18, 2025

fanquake closed this on Feb 18, 2025

maflcko deleted the branch on Feb 18, 2025

maflcko commented at 8:31 pm on February 18, 2025: member

Done in https://github.com/bitcoin/bitcoin/pull/31901

contrib: Add deterministic-fuzz-coverage #31836

Code Coverage & Benchmarks

Reviews