The calculation for the test chain’s work (total_work) should be outside of the loop. Previously, total_work was being miscalculated due to multiple additions of work from the same headers. Now, each header’s work is only counted once, providing an accurate total.
#30918 followup
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | dergoegge, instagibbs, glozow, mzumsande |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Can be tested by running FUZZ=p2p_headers_presync ./fuzzbuild/src/test/fuzz/fuzz chainwork_crash.txt
Should fail on master:
0Assertion failed: (total_work < chainman.MinimumChainWork()), function p2p_headers_presync_fuzz_target, file p2p_headers_presync.cpp, line 207.
and pass on this PR.
0 for afl. (The libfuzzer one has up to 100 exec/s). I checked the build logs (https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core) for FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION and it seems to be set.
According to https://oss-fuzz.com/fuzzer-stats?group_by=by-day&fuzzer=afl_bitcoin-core_p2p_headers_presync&project=bitcoin-core the exec/s is 0 for afl.
This is because oss-fuzz doesn’t use our afl++ harness but rather the afl libfuzzer driver. When using this driver, afl++ will fork after the initialization code is run (https://github.com/AFLplusplus/AFLplusplus/blob/d21fb1a558b25c4f46692fa999c0028dfe0eecc0/utils/aflpp_driver/aflpp_driver.c#L393), which might end up not working for harnesses that e.g. create temporary directories or threads during init. You can check the afl++ oss-fuzz logs and you’ll see that all inputs in the corpus timeout. This is the case for a few of our harnesses and as consequence they aren’t fuzzed with afl++ on oss-fuzz.
210
211- // This test should never create a chain with more work than MinimumChainWork.
212- assert(total_work < chainman.MinimumChainWork());
213- }
214+ // This test should never create a chain with more work than MinimumChainWork.
215+ assert(total_work < chainman.MinimumChainWork());
nit: You could get rid of total_work as a variable and just do:
0 assert(CalculateClaimedHeadersWork(all_headers) < chainman.MinimumChainWork());
all_headers then I could do this. I do like the variable for clarity, and I’d say this assertion is mainly to make the test easier to read. So I’ll keep it as is.
