p2p_headers_presync fuzz target times out when FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn’t set #30950

issue maflcko openend this issue on September 23, 2024
  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.

  13. maflcko removed the label Windows on Oct 1, 2024
  14. maflcko removed the label macOS on Oct 1, 2024
  15. maflcko renamed this:
    Test p2p_headers_presync fuzz target on macOS/Windows
    p2p_headers_presync fuzz target times out when FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn't set
    on Oct 1, 2024
  16. marcofleon commented at 6:02 pm on October 3, 2024: contributor

    I see what you’re saying. I initially thought BUILD_FOR_FUZZING was set as default (when fuzzing).

    I’ve addressed it in #31028.


maflcko marcofleon

Labels
Tests

Milestone
29.0


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me