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

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

That'd conceptually re-introduce the CVE
...

💬 pablomartin4btc commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2615354482)
nit (if you have to retouch and agree):
```suggestion
#define detail_LogIfCategoryAcceptsLevel(category, level, ...) \
```
or `detail_LogIfCategoryIsAcceptedAtLevel`... perhaps it's clearer and more consistent with existing internal call API `LogAcceptCategory()`.

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626127018)
My understanding is that you're inverting the logic here: instead of

```c++
#define QUARTERROUND(a,b,c,d) \
X(a,b,d,16) X(c,d,b,12) X(a,d,b,8) X(c,d,b,7)
```

This does the `X(a,b,d,16)` all four times via simd, then `X(c,d,b,12)` all four times, etc. And the simd part relies on the data for those four steps being exactly +4 units apart, which is why the shuffling and unshuffling is necessary for the second round. (Okay, not four times but eight times because each vec256 is two block
...

📝 vasild opened a pull request: "netif: fix compilation warning in QueryDefaultGatewayImpl()"
(https://github.com/bitcoin/bitcoin/pull/34093)
```
src/common/netif.cpp:137:51: error: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare]
137 | for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/netlink/netlink.h:220:31: note: expanded from macro 'NLMSG_OK'
220 | #define NLMSG_OK(_hdr, _len) NL_ITEM_OK(
...

💬 vasild commented on pull request "netif: fix compilation warning in QueryDefaultGatewayImpl()":
(https://github.com/bitcoin/bitcoin/pull/34093#issuecomment-3669832624)
I am not sure why there is no "comparison of integers of different signs" on Linux:

```cpp
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len <= (len))
```
We pass `int64_t` for `len`. The first comparison checks it against `(int)sizeof(...`, but the second comparison checks it against `nlmsg_len` which is `__u32`, so no matter what type is passed for
...

💬 maflcko commented on pull request "netif: fix compilation warning in QueryDefaultGatewayImpl()":
(https://github.com/bitcoin/bitcoin/pull/34093#issuecomment-3670881044)
> I am not sure why there is no "comparison of integers of different signs" on Linux:
>
> ```c++
> #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
> (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
> (nlh)->nlmsg_len <= (len))
> ```
>
> We pass `int64_t` for `len`. The first comparison checks it against `(int)sizeof(...`, but the 3rd comparison checks it against `nlmsg_len` which is `__u32`, so no matter what t
...

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3674308009)
> And imo, annotating a bunch of lambdas with an inline attribute feels weird.

You could `#define AI __attribute__((always_inline))`, then you'd just be adding `AI` to all the lambdas, which, if nothing else, would be very modern?

> My primary goal with this code (and hopefully setting a precedent for other multi-arch simd code... poly1305 is next ;) is to be as explicit to the compiler about what we want as possible. Ideally as portably as possible.

The above was kind-of a joke, but,
...