👍 ryanofsky approved a pull request: "refactor: Compile unreachable walletdb code"
(https://github.com/bitcoin/bitcoin/pull/29315#pullrequestreview-1844216896)
Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking.

Maybe a way to make it a little clearer would be to define `use_bdb` and `use_sqlite` c++ constants like:

```
constexpr bool use_bdb{
#ifdef USE_BDB
true
#endif
};
```

Then the actual code could use `if constexpr(use_bsb)` like a normal conditional. This might be more verbose than the current approach, though, and the current approach do
...

💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399)
Aha, I finally tracked down the culprit! No wonder the benchmarks weren't making sense!

The problem was the removal of this from `crypto/common.h`:
```c++
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```

Turns out, `sha256.cpp` was relying on that. Adding that include to `sha256.cpp` (where it belongs) fixes my local benchmarks.

I'm going to put the clz changes back to match the PR title/description, but this time with the include fixed up. Hopefully after t
...

💬 vasild commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1948185285)
The code in question can be improved:

```cpp
#ifdef USE_BDB
if constexpr (true) {
return MakeBerkeleyDatabase(path, options, status, error);
} else
#endif
{
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
status = DatabaseStatus::FAILED_BAD_FORMAT;
return nullptr;
}
```

If `USE_BDB` is defined then this becomes:

```cpp
if c
...

💬 hebasto commented on pull request "Fix misleading signmessage error with segwit":
(https://github.com/bitcoin-core/gui/pull/819#discussion_r1588327277)
It is https://github.com/bitcoin/bitcoin/pull/29494 goal to include this headers unguarded :)

Effectively, this PR conflicts with it. However, I believe it will be merged soon.

Anyway, it seems reasonable in this PR to use:
```c++
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```
or
```c++
#include <config/bitcoin-config.h> // IWYU pragma: keep
```

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618645388)
This is the upstream fix: https://github.com/freebsd/freebsd-src/pull/1070.

The suggested workaround to define `typeof` is ok:

```diff
diff --git i/src/util/netif.cpp w/src/util/netif.cpp
index 845b8aed1d..1840974a83 100644
--- i/src/util/netif.cpp
+++ w/src/util/netif.cpp
@@ -9,18 +9,24 @@
#include <logging.h>
#include <netbase.h>
#include <util/check.h>
#include <util/sock.h>
#include <util/syserror.h>

+#ifdef __FreeBSD__
+#include <osreldate.h>
+#endif
+
// Linux
...

💬 nicholas-1vbw commented on issue "Risczero Fit":
(https://github.com/bitcoin/bitcoin/issues/30747#issuecomment-2320560690)
We compiled a scratch main.cpp that includes `assumptions.h` successfully, but fails with `static_assert(std::is_same_v<int, int32_t>);`

Actually, the toolchain from risc0 has different type of "int" and "int32_t", see https://github.com/risc0/toolchain/releases/tag/2024.01.05

int32_t is defined in "riscv32-unknown-elf/include/sys/_stdint.h"
```
#ifdef ___int32_t_defined
#ifndef _INT32_T_DECLARED
typedef __int32_t int32_t ;
#define _INT32_T_DECLARED
#endif
#ifndef _UINT32_T_DECLARED
...

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349317870)
>Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is not set, so it's not only in the build system?

I guess we could (e.g. in `bitcoind.cpp`) do something like:

```c++
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
static_assert(false, "bitcoind can't be build with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION");
#endif
```

But I would be surprised if our existing tests do not already
...

💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497)
I think this could be de-duplicated and made a little cleaner (same for the magic byte check):

```c++
bool checksum_ok{memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0};
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
checksum_ok = checksum_ok || (hdr.pchChecksum[0] & 1) != 0;
#endif

if (!checksum_ok) {
...
```

💬 promag commented on pull request "qt6, test: Handle deprecated code":
(https://github.com/bitcoin-core/gui/pull/839#discussion_r1786676537)
2b5fd8fd24026ecf2fac7af6657e2363bfe3706c

An alternative to reduce duplicate code and the diff is to define `QVERIFY_EXCEPTION_THROWN`, something like?

```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
#define QVERIFY_EXCEPTION_THROWN(a, b) QVERIFY_THROWS_EXCEPTION(b, a)
#endif
```

🤔 promag reviewed a pull request: "qt6: Handle different signatures of `QANEF::nativeEventFilter`"
(https://github.com/bitcoin-core/gui/pull/840#pullrequestreview-2349357223)
I think small changes to make code compatible with both versions are fine until support for qt5 is dropped.

Code review ACK 80761afced12c774a1768fb27a3975d456981ae0.

Alternative is to use type alias
```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
using result_t = qintptr*;
#else
using result_t = long*;
#endif

💬 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
};

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849132153)
Sorry, I forgot to include the diff which updates the macro (and 1 instance to show it compiles):

<details>
<summary>git diff on 6c9121f790</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 9e6bf127db..67248349e2 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -31,9 +31,9 @@
#define BITCOINKERNEL_WARN_UNUSED_RESULT
#endif
#if !defined(BITCOINKERNEL_BUILD) && defined(__GNUC__) && BITCOINKERNEL_GNUC_PREREQ(3,
...

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002955096)
I'm not exactly sure how we got to the `min_alloc` of `9` on these platforms, but we can probably simplify the platform dependent code to:
```C++
#if defined(__arm__) || SIZE_MAX == UINT64_MAX
constexpr size_t min_alloc{9};
#else
constexpr size_t min_alloc{0};
#endif
```

💬 rkrux commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2042044806)
Few lines below.

https://github.com/bitcoin/bitcoin/pull/31250/commits/150a83ca4fcad4e158cef3e6752b3a84ffb7d0b6#diff-7b13e03c457b62251e26d2c366180c262a3fe9da45995277a9a9c1c5abbaae6cR415-R419

```cpp
#ifndef USE_BDB
if (!(flags & WALLET_FLAG_DESCRIPTORS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without bdb support (required for legacy wallets)");
}
#endif
```

I don't believe this is required anymore, the inner condition can never be true now as `WALLET_FLAG_
...

💬 Zeegaths commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-2977260556)
Thank you, I will do just that

On Mon, 16 Jun 2025 at 18:12, Winnie Gitau ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/init.cpp
> <https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2150261914>:
>
> > @@ -157,7 +157,7 @@ static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
> #endif
>
> static constexpr int MIN_CORE_FDS = MIN_LEVELDB_FDS + NUM_FDS_MESSAGE_CAPTURE;
> -static const char* DEFAULT_ASMAP_F
...

💬 Zeegaths commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3106720428)
Fixed!

On Tue, 22 Jul 2025 at 23:44, Winnie Gitau ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/init.cpp
> <https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2223787842>:
>
> > @@ -438,6 +438,18 @@ static void registerSignalHandler(int signal, void(*handler)(int))
> }
> #endif
>
> +std::string GetDocumentationUrl(const std::string& doc_path)
> +{
> + if (CLIENT_VERSION_MINOR == 99) {
> + std::
...

💬 hebasto commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362554073)
This should work:
```suggestion
std::optional<size_t> GetTotalRAM()
{
auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
#ifdef WIN32
if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
#else
if (long p{sysconf(_SC_PHYS_PAGES)}, s{sysconf(_SC_PAGESIZE)}; p > 0 && s > 0) return clamp(1ULL * p * s);
#endif
return std::nullopt;
}

```

See CI outputs for:
- Fr
...

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387108889)
Could do potentially do something like...
```C++
#if defined(WAIT_FOR_DEBUGGER) && defined(NDEBUG)
#error "Should only enable WAIT_FOR_DEBUGGER in debug builds"
#endif
```
...though there could be scenarios where one wants to debug release builds as well.

*/contrib/guix/symbol-check.py* could also potentially analyze the binary after the fact. LLM says that the *lief* module can find calls to `prctl` when dynamically linking and otherwise one could add a dependency on the *capstone* mod
...