Repository: bitcoin/bitcoin (language: cpp)

Issue:
p2p_headers_presync fuzz target times out when FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION isn't set
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 https://github.com/bitcoin/bitcoin/issues/30661#issuecomment-2363634967_
            


Discussion / hints:
Looking into this.
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.
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.
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 https://github.com/bitcoin/bitcoin/pull/31028.

Fix patch:
diff --git a/CMakeLists.txt b/CMakeLists.txt
index e68af79b51558..6938eaa5a86cb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -240,11 +240,6 @@ if(BUILD_FOR_FUZZING)
   set(BUILD_GUI_TESTS OFF)
   set(BUILD_BENCH OFF)
   set(BUILD_FUZZ_BINARY ON)
-
-  target_compile_definitions(core_interface INTERFACE
-    ABORT_ON_FAILED_ASSUME
-    FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-  )
 endif()
 
 include(ProcessConfigurations)
diff --git a/src/pow.cpp b/src/pow.cpp
index 6c8e7e5d98a6b..eae5b65cac436 100644
--- a/src/pow.cpp
+++ b/src/pow.cpp
@@ -9,6 +9,7 @@
 #include <chain.h>
 #include <primitives/block.h>
 #include <uint256.h>
+#include <util/check.h>
 
 unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params& params)
 {
@@ -138,11 +139,8 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
 // the most signficant bit of the last byte of the hash is set.
 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
 {
-#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-    return (hash.data()[31] & 0x80) == 0;
-#else
+    if (g_fuzzing) return (hash.data()[31] & 0x80) == 0;
     return CheckProofOfWorkImpl(hash, nBits, params);
-#endif
 }
 
 bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params& params)
diff --git a/src/util/check.cpp b/src/util/check.cpp
index e1956042c300e..57ff78f7a089a 100644
--- a/src/util/check.cpp
+++ b/src/util/check.cpp
@@ -14,6 +14,8 @@
 #include <string>
 #include <string_view>
 
+bool g_fuzzing = false;
+
 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func)
 {
     return strprintf("Internal bug detected: %s\n%s:%d (%s)\n"
diff --git a/src/util/check.h b/src/util/check.h
index 187187e593ecb..28c6247b91cc7 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -13,6 +13,8 @@
 #include <string_view>
 #include <utility>
 
+extern bool g_fuzzing;
+
 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func);
 
 class NonFatalCheckError : public std::runtime_error
@@ -42,7 +44,7 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std:
 template <bool IS_ASSERT, typename T>
 constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
 {
-    if (IS_ASSERT || std::is_constant_evaluated()
+    if (IS_ASSERT || std::is_constant_evaluated() || g_fuzzing
 #ifdef ABORT_ON_FAILED_ASSUME
         || true
 #endif


Test patch:
diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index bba2dd8e3a393..654e1f8be2470 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -102,6 +102,8 @@ void ResetCoverageCounters() {}
 
 void initialize()
 {
+    g_fuzzing = true;
+
     // By default, make the RNG deterministic with a fixed seed. This will affect all
     // randomness during the fuzz test, except:
     // - GetStrongRandBytes(), which is used for the creation of private key material.