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

📝 maflcko opened a pull request: "util: Allow Assert() in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785)
Without this, compile warnings could be hit about `__func__` being only valid inside functions.

```
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
```

Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r24862
...

🤔 hebasto reviewed a pull request: "refactor: Add missing include in bitcoinkernel_wrapper.h"
(https://github.com/bitcoin/bitcoin/pull/33825#pullrequestreview-3438418410)
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.

[Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on https://github.com/bitcoin/bitcoin/pull/33810:
```diff
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -5,12 +5,19 @@
#ifndef BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
#define BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H

-#include <kern
...

💬 maflcko commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2509568255)
Just for ref, one could write this to please clang-format, but not sure if it is worth it:

```cpp
#define NONFATAL_UNREACHABLE() \
do { \
throw NonFatalCheckError{ \
"Unreachable code reached (non-fatal)", \
std::source_location::current(), \
}; \
} while (0)

💬 maflcko commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482)
Not sure how to reproduce locally, but this seems to be failing:

```
/__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```

https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808314#step:9:403

💬 hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522533346)
> Not sure how to reproduce locally, but this seems to be failing:
>
> ```
> /__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
> 33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
> ```
>
> https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808327#step:6:405

Does the following patch help:
```diff
--- a/src/clientversion.cpp
+++ b/sr
...

📝 maflcko opened a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869)
Otherwise, compilation with GCC-15+ will warn about it:

```
src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```

Follow-up to https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482

Can be tested via `git archive --output=/tmp/a.tar HEAD`

💬 fanquake commented on pull request "refactor: Avoid -W*-whitespace in git archive":
(https://github.com/bitcoin/bitcoin/pull/33869#issuecomment-3541325190)
ACK fa95353902b7a6f73f094e78106088ab3c16ce14

```cpp
// git will expand the next line to "#define GIT_COMMIT_ID ..." inside archives:
//
#define GIT_COMMIT_ID "84f39b282de0836ca5ad6166daafecf15e4c813d"
```

💬 janb84 commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2561205810)
I find this a bit inconsistent with this enum :

```C
typedef uint8_t btck_ValidationMode;
#define btck_ValidationMode_VALID ((btck_ValidationMode)(0))
#define btck_ValidationMode_INVALID ((btck_ValidationMode)(1))
#define btck_ValidationMode_INTERNAL_ERROR ((btck_ValidationMode)(2))
```
Where `validationmode` runs from 0 to 2 and 2 is the internal error.

Also isn't it better to expose this as an ENUM so that it can be re-used by the handlers ? (not sure because of the single use)

...

💬 rkrux commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#issuecomment-3585822978)
Ah, you are correct. I got carried away by the comment and assumed the code is like below (`LogPrintLevel` instead of `LogInfo`):

```cpp
// Deprecated unconditional logging.
#define LogPrintf(...) LogPrintLevel_(__VA_ARGS__)
```

💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612981132)
Why not write this like?
```
#define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
do {
Assume(level >= BCLog::Level::Info);
if (LogAcceptCategory((category), (level))) {
LogPrintLevel_(category, level, rate_limit, __VA_ARGS__);
}
} while (0)
```