💬 marcofleon commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1790584206)
Agreed, that's a better way to do it. Let me know what you think about d470af336a3f66230e9c68ffc924caad0ad84980. Another possibility I was thinking of was something like:
```
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
#define CONDITIONAL_REGISTER(name, target, opts) FuzzFrameworkRegisterTarget(name, target, opts)
#else
#define CONDITIONAL_REGISTER(name, target, opts)
do {
...

💬 maflcko commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1791549320)
```suggestion
constexpr bool build_for_fuzzing{
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
{true};
#else
{false};
#endif
```

nit: Could rename, so that it can be re-used in the future for other stuff, if needed?

💬 theuni commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2403348003)
If we're going to be peppering the code with these, could we add a safety harness to make sure it's never being used accidentally for a live node?

Maybe somewhere in init.cpp or bitcoind.cpp:
```c++
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
exit(1);
#endif
```

💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449665430)
Then I think the only solution is to make it a build-time option again:

```cpp
constexpr bool g_fuzzing{
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
true
#else
false
#endif
};

💬 hodlinator commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1886924031)
Skip in production
```suggestion
if constexpr (G_FUZZING) {
g_used_g_prng = true;
}
```
or use `#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.

Maybe an optimizer would remove it as a dead-store, but better not to rely on that.

💬 hodlinator commented on pull request "qa debug: Add --debugnode/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2609977320)
> Ideally we wouldn't be adding debug code, to production binaries, to facilitate using an IDE.

Agree it should not be in production binaries, should be `#ifdef`ed away. Added as another reason for draft status.

> Can you configure your IDE to do something like the equivalent of using `gdb`/`lldb`, and `process attach --name bitcoind --waitfor`? So it just grabs bitcoind once it spins up?

I've never seen something like that in an IDE/editor, but I'll have a look, might exist a debugger
...

💬 hodlinator commented on pull request "qa debug: Add --debugnode/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2616686520)
Added tentative `#ifdef` solution in latest push. CMake part could probably be improved since now I think the `#define` leaks into the CMake projects public interface, which should not be necessary.

💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048555899)
I don't think we need to introduce checks here, we already use `clock_gettime`. You could just #ifdef `CLOCK_THREAD_CPUTIME_ID` at the callsite.

💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048557177)
I don't think we check for any other Windows functions like this, I think you can just #ifdef WIN32 at the callsite.

💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049555771)
The simplest way I could find is to call it directly (not sure what the fuzzer did, don't have access):
```C++
BOOST_AUTO_TEST_CASE(feefrac_div_round_down_overflow)
{
#ifdef __SIZEOF_INT128__
constexpr auto divisor{2};
constexpr auto expected_result{std::numeric_limits<int64_t>::max()};
constexpr auto overflow_numerator{__int128{expected_result} * divisor + 1};
BOOST_CHECK_EQUAL(FeeFrac::Div(overflow_numerator, divisor, /*round_down=*/true), expected_result);
#else

...

💬 shahsb commented on pull request "fuzz: Suppress abort message on Windows":
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2072293276)
```suggestion
#ifdef _WIN32
```

`_WIN32` is a compiler flag and could be used for both windows platform i.e windows_x86 (32-bit) as well as windows_x64 (64-bit)

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077565200)
nit: Wrap in `#ifdef WIN32`?

💬 hebasto commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079675289)
"Define to 1 if '*ifaddrs' are available" implies that "it remains undefined otherwise". To achieve this, I suggest:
```suggestion
/* Define to 1 if '*ifaddrs' are available. */
#cmakedefine HAVE_IFADDRS 1
```

This also allows to replace the following `#if HAVE_IFADDRS` with `#ifdef HAVE_IFADDRS`.

💬 fanquake commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2098299332)
I think you can just drop these `#ifdef`s around our own headers. Same for in `RunCommandParseJSON` below.

💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2252321022)
Per offline discussion, removed the `#ifdef DEBUG_LOCKCONTENTION` since the category masks are now properly reset between iterations (thanks!).

💬 janb84 commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282214659)
Nit, Both define lines are a change in the `#ifdef` style (also from the previous generated file). Personally I do not care which style is followed. Just noting that this is a derivation.

```suggestion
#define UNUSED __attribute__((unused))
```

💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347458082)
If any of you feel strongly, I can change it, but it was a deliberate choice from my part.
See https://godbolt.org/z/o3sKqefT5 for an ugly error on Windows that also convinced me to just use the shorter version. I could of course hack it with:
```C++
#ifdef _WIN32
#include <windows.h>
#undef max
#else
```
or just hope that https://github.com/bitcoin/bitcoin/blob/9f744fffc39d93c9966dec1d61f113a7521983ad/CMakeLists.txt#L261 fixes it, but `SIZE_MAX` seemed simpler and shorter in this case :/

-
...