💬 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 {
...
(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?
(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
```
(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
};
(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.
(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
...
(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.
(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.
(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.
(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
...
(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
...