💬 hodlinator commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1887121592)
This is about excluding test-only code from production (currently fuzz-only since *check_globals.h/cpp* is in *fuzz/util/*). Not sure why that would be undesirable. Even though the conditional adds verbosity, it adds context and decreases need for comments.

Might be worth introducing a `TESTS_ENABLED` `#define`... but I'm guessing that has been discussed before?

💬 maflcko commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1888046321)
> Might be worth introducing a `TESTS_ENABLED` `#define`... but I'm guessing that has been discussed before?

I don't think it has. Maybe a separate issue or pull request can be created?

Whatever is done here also applies to the function immediately above, which is unrelated, so I'll leave this as-is for now.

💬 hodlinator commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1888106180)
Interesting that it hasn't really been discussed. There are trade-offs to adding it, but yeah, might experiment with adding it and create an issue.

Correct, `MakeRandDeterministicDANGEROUS()` should not exist unless `TESTS_ENABLED` was `#define`d (in that hypothetical scenario).

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

💬 brunoerg commented on pull request "rpc: collect transaction fees on generateblock":
(https://github.com/bitcoin/bitcoin/pull/31775#discussion_r1938131196)
```
19:24:04.713] /ci_container_base/src/node/miner.cpp:150:9: error: 'exclusive_locks_required' attribute cannot be applied to a statement
[19:24:04.713] 150 | EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
[19:24:04.713] | ^ ~
[19:24:04.713] /ci_container_base/src/threadsafety.h:31:54: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
[19:24:04.713] 31 | #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_requir
...

💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1970203305)
Just below here... the `#define` for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.

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

💬 hebasto commented on pull request "Bugfix: Only use git for build info if the repository is actually the right one":
(https://github.com/bitcoin/bitcoin/pull/32217#issuecomment-2783296138)
Tested 903fc971ed6b13da7260f5e5a8e3e9e7372b2bd7 on Ubuntu 24.10.

When building from my local Bitcoin Core repository:
- on the master branch @ 8cb6ab0b971a27ba255ad6a48959a8c7b84c00f3:
```
$ cmake --build build -t bitcoin_clientversion
[1/3] Generating bitcoin-build-info.h
$ cat build/src/bitcoin-build-info.h
#define BUILD_GIT_COMMIT "8cb6ab0b971a"
```
- this PR:
```
$ cmake --build build -t bitcoin_clientversion
[3/3] Linking CXX static library lib/libbitcoin_clientversion.a
$ c
...

👍 laanwj approved a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445#pullrequestreview-2824986604)
Code review ACK dfeac76b951fc2d28e0832b176f3c8ad595697f1

For example:
```sh
$ export SOURCE_DATE_EPOCH=1
$ cmake -B build
$ grep YEAR build/src/*.h
build/src/bitcoin-build-config.h:#define COPYRIGHT_YEAR 1970
```
In the guix build it sets `SOURCE_DATE_EPOCH` based on the last commit
```
SOURCE_DATE_EPOCH="${SOURCE_DATE_EPOCH:-$(git -c log.showSignature=false log --format=%at -1)}"
```
So ostensibly this works out the correct date deterministically.

Should test an actual guix bui
...

💬 laanwj commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2101043927)
fwiw: `GetACP` is a windows system function implemented in `kernel32.dll`, defined in the `winnls.h` header. There shouldn't be a mingw-specific definition.

`CP_UTF8` is also defined in that header and has the following value:
```c++
#define CP_UTF8 65001
```

`1252` is codepage [windows-1252](https://en.wikipedia.org/wiki/Windows-1252), a legacy encoding.

👍 hodlinator approved a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2876232435)
crACK b97fc15df593d42296015218db197f377a71461c

The custom `WINDRES_PREPROC` #define was only read in clientversion.h. The built-in `RC_INVOKED` #define in the resource compiler is more straight forward to use to skip over sections of files that compiler does not support.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112726648)
In commit "feature: Add TxOrphanageImpl"

Many of the includes here seem unused:

```diff
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -5,33 +5,20 @@
#ifndef BITCOIN_NODE_TXORPHANAGE_IMPL_H
#define BITCOIN_NODE_TXORPHANAGE_IMPL_H

-#include <coins.h>
-#include <consensus/amount.h>
-#include <indirectmap.h>
#include <logging.h>
-#include <net.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
-#include <sync.h>
-#include <util/epoc
...

💬 fanquake commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2918940425)
https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223
```bash
[12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
[12:12:21.305] 23 | AssertLockNotHeld(mutex);
[12:12:21.305] | ^
[12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
[12:12:21.305] 141 | #define AssertLockNotHeld(cs
...

💬 theuni commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2919662378)
> https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223
>
> ```shell
> [12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
> [12:12:21.305] 23 | AssertLockNotHeld(mutex);
> [12:12:21.305] | ^
> [12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
> [12:12:21.305] 141 | #define A
...

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

💬 edwargix commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2285678340)
please forgive me if I'm misunderstanding this, but can't this logic be simplified to:
```suggestion
#if defined(_WIN32)
#define BITCOINKERNEL_API __declspec(dllexport)
#elif defined(__GNUC__)
#define BITCOINKERNEL_API __attribute__((visibility("default")))
```

💬 davidgumberg commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308685773)
Feel free to mark resolved, this suggestion is actually incorrect, since when `recv_result == 0` the `NLMSG_OK` check [below](https://github.com/bitcoin/bitcoin/blob/4c531782569b42fc47478a468f4a79e0e2d93946/src/common/netif.cpp#L110) would fail:


```c
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len <= (len))
```
https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c
...

💬 ajtowns commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3345222853)
Ah, there we go; these look like relevant lines:

```
$ HOSTS=powerpc64-linux-gnu contrib/guix/guix-build
...
xcb_auth.c:74:37: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
74 | #define AUTH_PROTO_MIT_MAGIC_COOKIE "MIT-MAGIC-COOKIE-1"
| ^~~~~~~~~~~~~~~~~~~~
xcb_auth.c:80:5: note: in expansion of macro 'AUTH_PROTO_MIT_MAGIC_COOKIE'
80 | AUTH_PROTO_MIT_MAGIC_COOKIE,
| ^~~~~~~~~~~~~
...

💬 hodlinator commented on pull request "refactor: replace `const char*` exceptions with `std::runtime_error`":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2412967418)
I don't get it, https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401 logs:
`
test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
`

Looking at where the error message comes from - https://www.boost.org/doc/libs/1_87_0/boost/test/tools/old/interface.hpp, it has:
```C++
#define BOOST_CHECK_THROW_I
...

💬 ajtowns commented on pull request "refactor: Construct g_verify_flag_names on first use":
(https://github.com/bitcoin/bitcoin/pull/33600#discussion_r2427318037)
I had a look at this approach too. With C++20, I believe you can ensure the array is sorted at compile time with:

```c++
template<typename V, size_t N>
consteval auto SortedArray(const std::array<V,N>& pairs)
{
std::array<V,N> result = pairs;
std::sort(result.begin(), result.end());
return result;
}
#define FLAG_NAME(flag) std::pair<std::string_view, script_verify_flag_name>{#flag, SCRIPT_VERIFY_##flag}
constexpr auto g_verify_flag_names = SortedArray(std::array{
FLA
...