💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790792236)
> Can you explain the patch? What is the actual problem, and how does this fix it? Seems a bit odd to have to patch version detection checks inside Qt, can the correct values not be passed in from the outside?

In macOS SDK, the problematic `os/activity.h` header relies on the `OS_LOG_TARGET_HAS_10_12_FEATURES` macro:
```c++
#if OS_LOG_TARGET_HAS_10_12_FEATURES
#ifndef OS_ACTIVITY_OBJECT_API
#define OS_ACTIVITY_OBJECT_API 1
#endif // OS_ACTIVITY_OBJECT_API
#else
#if OS_ACTIVITY_OBJECT_A
...

💬 fanquake commented on issue "Building a wallet with legacy support fails on OpenBSD 7.4":
(https://github.com/bitcoin/bitcoin/issues/28963#issuecomment-1834640327)
> In file included from ../dist/./../cxx/cxx_db.cpp:13:
> ./db_cxx.h:59:10: fatal error: 'iostream.h' file not found

The issue is some code like this in bdb:
```cpp
#ifdef HAVE_CXX_STDHEADERS
#include <iostream>
#include <exception>
#define __DB_STD(x) std::x
#else
#include <iostream.h>
#include <exception.h>
#define __DB_STD(x) x
#endif
```

For some reason, configure fails to detect C++ header availability, and `HAVE_CXX_STDHEADERS` is not defined. Is `g++` and a C++ standard
...

⚠️ DMUNEY45 opened an issue: "> In file included from ../dist/./../cxx/cxx_db.cpp:13:"
(https://github.com/bitcoin/bitcoin/issues/28982)
> In file included from ../dist/./../cxx/cxx_db.cpp:13:
> ./db_cxx.h:59:10: fatal error: 'iostream.h' file not found

The issue is some code like this in bdb:
```cpp
#ifdef HAVE_CXX_STDHEADERS
#include <iostream>
#include <exception>
#define __DB_STD(x) std::x
#else
#include <iostream.h>
#include <exception.h>
#define __DB_STD(x) x
#endif
```

For some reason, configure fails to detect C++ header availability, and `HAVE_CXX_STDHEADERS` is not defined. Is `g++` and a
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1609954791)
bebfafcf98d8ed1432407b7603991d54f7cc26c2: it would be useful to have a quick recap here of which strategy is used by `QueryDefaultGateway` for which OS.

I think it will be (slightly) more readable to have a single `QueryDefaultGateway` implementation which then calls `QueryDefaultGatewayWindows`, `QueryDefaultGatewayMac` and `QueryDefaultGatewayLinuxBSD`.

It can start with:

```cpp
Assume(network == NET_IPV4 || network == NET_IPV6)
```

and end with

```cpp
#else return std::nullo
...

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1651128863)
Would have written it closer to something like:
```suggestion
fs::path absolutePath;
#ifdef WIN32
if (const char* homedrive = std::getenv("HOMEDRIVE"), * homepath = std::getenv("HOMEPATH");
homedrive != nullptr && homedrive[0] != 0 && homepath != nullptr && homepath[0] != 0)
absolutePath = fs::path(homedrive) / fs::path(homepath);
else
absolutePath = fs::path("C:\\Users\\Satoshi");
#else
if (const char* home = std::getenv("HOME"); home != nullptr
...

⚠️ brunoerg opened an issue: "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated"
(https://github.com/bitcoin/bitcoin/issues/30957)
Similar to #28019. The following instruction is outdated and doesn't work:
```diff
$ git apply << "EOF"
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 8195bceaec..cce2b31ff0 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -90,8 +90,12 @@ typedef char* sockopt_arg_type;
// building with a binutils < 2.36 is subject to this ld bug.
#define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
#else
+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
+#
...

💬 maflcko commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2376413368)
Should be trivial to rebase it, no?


```diff
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 366c648ae7..1f3bf2b018 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -92,8 +92,12 @@ typedef char* sockopt_arg_type;
// building with a binutils < 2.36 is subject to this ld bug.
#define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
#else
+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
+#define MAIN_FUNCTION HFND_FUZZING_ENTRY_FUNCTION_CXX(int
...

🤔 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

💬 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?

💬 real-or-random commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429613882)
@fanquake Can you elaborate on the motivation for this PR? The code in the `#else` should work for any gcc and clang, on any platform. While it uses an asm block, there's no actual asm instruction inside that could be platform-specific.

See also its platform-independent usage in Linux:
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux/string.h#L369-L376
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux
...

💬 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 "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1981191286)
nit: Less confusing for every branch in the block to result in `#define`:
```suggestion
#else
```

💬 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
```

💬 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

...

💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3206135519)
Diff to reproduce on Linux:


```diff
diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
index be7f1ee5a2..ada3280dbc 100644
--- a/src/util/fs_helpers.cpp
+++ b/src/util/fs_helpers.cpp
@@ -200,7 +200,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
}
ftruncate(fileno(file), static_cast<off_t>(offset) + length);
#else
-#if defined(HAVE_POSIX_FALLOCATE)
+#if 0
// Version using posix_fallocate
off_t nEndPos = (off_t)offset + length;

...

💬 purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2288681559)
I think the deep nesting can be avoided with the approach mentioned in https://habla.news/u/purplekarrot.net/cmake-import-export

Also, I don't think the `#else` branch is needed. To my knowledge, there is no platform that supports C++20 but neither dllexport nor visibility.

💬 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 :/

-
...

💬 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
...

💬 hebasto commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362723602)
> I did not go with `#else` since I am not sure that will compile and work on all possible platforms that are not Windows.

Right. We could end up with macros for all tested platforms.