Test p2p_headers_presync fuzz target on macOS/Windows #30950

1. 
   maflcko commented at 10:48 am on September 23, 2024: member

   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)

2. maflcko added the label Windows on Sep 23, 2024
3. maflcko added the label macOS on Sep 23, 2024
4. maflcko added the label Tests on Sep 23, 2024
5. maflcko added this to the milestone 29.0 on Sep 23, 2024
6. 
   marcofleon commented at 12:39 pm on September 24, 2024: contributor

   Looking into this.
7. 
   marcofleon commented at 2:20 pm on September 24, 2024: contributor

   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?

8. 
   maflcko commented at 2:27 pm on September 24, 2024: member

   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.

9. 
   marcofleon commented at 4:37 pm on September 26, 2024: contributor

   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.

10. 
    maflcko commented at 4:43 pm on September 26, 2024: member

    What happens in the CI should only be secondary to what developers are exposed to. So the question would be: Why should developers and testers be let to run into the same timeout? If there is no value in running the target when FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn’t set, then I don’t see why developers and testers should be bothered with a timeout.
11. 
    marcofleon commented at 5:13 pm on September 26, 2024: contributor

    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.

12. 
    maflcko commented at 8:10 pm on September 26, 2024: member

    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.

Contributors
maflcko marcofleon

Labels
Windows macOS Tests

Milestone
29.0