The target runs slowly on macOS/Windows, so it would be good if someone benchmarked it. At least the macOS 14 CI ran into timeouts consistently.
I’ve removed the fuzz inputs temporarily in https://github.com/bitcoin-core/qa-assets/commit/84cea7068728bc2c765b9da680eedcb85f9f3c1b in the meantime.
Originally posted by @maflcko in #30661 (comment)
The BUILD_FOR_FUZZING flag isn’t on for Windows and macOS CI, so the ABORT_ON_FAILED_ASSUME and FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION macros aren’t defined. The p2p_headers_presync target is doing actual PoW in CheckProofOfWorkImpl and timing out.
On the Linux CI those macros are here: https://cirrus-ci.com/task/6411626390224896?logs=ci#L831
Temporary solution would be excluding this harness for Windows and mac in the test runner. Long term we should probably separate out the CI jobs?
Makes sense. Is there any value in fuzzing this target when FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn’t set?
If not, an early return in the target could make sense when FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn’t set.
Is that better than adding -x p2p_headers_presync to the Windows and macOS test runner calls in the CI?
Using the exclude flag, as opposed to altering the harness, feels cleaner to me. Afaik they would both accomplish the same thing, so ultimately I’m okay with either.
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn’t set, then I don’t see why developers and testers should be bothered with a timeout.
Wouldn’t a developer looking to fuzz test set -DBUILD_FOR_FUZZING=ON? In which case, that macro is automatically set (along with ABORT_ON_FAILED_ASSUME) and there would be no timeout. Would future harnesses relying on FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION also do an early return?
Also, sorry if I’m misunderstanding your point. I’m not too familiar with what the best practice would be here.
Wouldn’t a developer looking to fuzz test set
-DBUILD_FOR_FUZZING=ON?
Most likely yes, but it isn’t enforced. For example, it is possible to compile the fuzz binary without DBUILD_FOR_FUZZING and execute it. Support for that is also added to the fuzz/test_runner.py script. (This is also what the failing CIs were using to call the fuzz binary).
Would future harnesses relying on FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION also do an early return?
I’d say it depends on the fuzz target and it can be decided on a case-by-case basis what seems most suitable.
I see what you’re saying. I initially thought BUILD_FOR_FUZZING was set as default (when fuzzing).
I’ve addressed it in #31028.