Skip to content

Conversation

dergoegge
Copy link
Member

Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.

In both jobs the fuzz binary is built with -DBUILD_FOR_FUZZING to enable Assume assertions as well as FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 11, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)
  • #31157 (Cleanups to port mapping module post UPnP drop by darosior)
  • #31130 (Drop miniupnp dependency by darosior)
  • #28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@dergoegge dergoegge marked this pull request as ready for review October 11, 2024 14:51
@DrahtBot DrahtBot added the Tests label Oct 11, 2024
@maflcko
Copy link
Member

maflcko commented Oct 14, 2024

Not sure about rushing this. Maybe wait for a fix for #31057 first, after which this may or may not be needed?

@maflcko
Copy link
Member

maflcko commented Oct 28, 2024

Can this be closed?

@dergoegge
Copy link
Member Author

I still think this would make sense but there seems to be no interest. Also #30950 has been resolved.

@dergoegge dergoegge closed this Oct 28, 2024
ryanofsky added a commit that referenced this pull request Nov 5, 2024
…ZZING=ON to fuzz

fafbf8a Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target (MarcoFalke)
fae3cf0 ci: Temporarily disable macOS/Windows fuzz step (MarcoFalke)

Pull request description:

  `g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See #31178

  One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future.

  Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to be set when executing any fuzz target.

  Fixes #31178

  Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like #31073

ACKs for top commit:
  marcofleon:
    Tested ACK fafbf8a
  davidgumberg:
    I still ACK fafbf8a for fixing the regression measured in #31178.
  ryanofsky:
    Code review ACK fafbf8a but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one.
  dergoegge:
    utACK fafbf8a

Tree-SHA512: 124fc2e8b35e0c4df414436556a7a0a36cd1bec4b3000b40dcf2ab8c85f32e0610bf7f70d2fd79223d62f3c3665b6c09da21241654c7b9859461b8ca340d5421
@maflcko
Copy link
Member

maflcko commented Nov 5, 2024

This may be re-considered after a rebase. However, I wonder if the bloat can be reduced, assuming GHA has a matrix feature, so that the non-fuzz and fuzz-build can share most of the GHA config and steps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants