👍 theStack approved a pull request: "test: add option to skip large re-org test in feature_block"
(https://github.com/bitcoin/bitcoin/pull/33003#pullrequestreview-3417493966)
Concept and code-review ACK 8810642b571e1d8482375e962a1129b691d5d226

💬 theStack commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2491234209)
nit: this part could be moved inside the if-block below as well, as there is no need to disable v2transport if the re-org is skipped (though it currently doesn't matter much, as re-orging is the last part of the functional tests anyway)

💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486958734)
The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).

💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486967777)
Concept ACK.

💬 fanquake commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486969063)
> The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).

Could you clarify. Reading those docs, `<prefix>/(cmake|CMake)/` is only valid on Windows installation trees. Not Unix, which is what is being changed here?

💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486973223)
> > The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
>
> Could you clarify. Reading those docs, `<prefix>/(cmake|CMake)/` is only valid on Windows installation trees. Not Unix, which is what is being changed here?

You're right. Already updated my comment.

📝 hebasto opened a pull request: "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors"
(https://github.com/bitcoin/bitcoin/pull/33779)
Now seems like a good time to update the includes in `src/kernel`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3487100711)
`c6f46943d1...ada059e714`: rebase due to conflicts

💬 m3dwards commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#discussion_r2491364489)
I think this line can be dropped now `test/util` is no longer there since #32697?

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3487120311)
There are a lot of code moves in this PR. I put the following in my `~/.gitconfig`:

```gitconfig
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```
it makes reviewing such mechanical moves easier.

💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3487224723)
> The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
>
> UPD. ... on Windows, but not when cross-compiling.

On the master branch, `find_package(ZeroMQ)` succeeds when cross-compiling for Windows. Here is an excerpt from the build log in debug mode:
```
Prepending the following roots to each prefix:

CMAKE_FIND_ROOT_PATH

/home/hebasto/dev/bitcoin/depends/x86_64-w64-mingw32

CMAKE_SYSR
...

📝 fanquake opened a pull request: "guix: disable libsanitizer in Linux GCC build"
(https://github.com/bitcoin/bitcoin/pull/33780)
This causes issues when building against newer glibcs (i.e 2.42), and isn't needed in any case.

```bash
../../../../gcc-14.3.0/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:483:31: error: invalid application of ‘sizeof’ to incomplete type ‘__sanitizer::termio’
483 | unsigned struct_termio_sz = sizeof(struct termio);
| ^~~~~~~~~~~~~~~~~~~~~
```

Extracted from #25573.

💬 maflcko commented on pull request "random: scope environ extern to macOS, BSDs and Illumos":
(https://github.com/bitcoin/bitcoin/pull/33714#discussion_r2491476516)
Actually, the `NOLINT` was fixed and can be removed? You've confirmed that it is undeclared on the listed platforms, so it can't be hit by `readability-redundant-declaration`

💬 brunoerg commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3487272706)
> Unit test is nice to demonstrate usage and test some cases, but I really like the coverage fuzz test provide like catching some subtle bugs that could be missed in review.

I agree with @ryanofsky about having unit test - fuzzing doesn't exclude the need of having unit tests. Fuzzing depends on the provided inputs, unit tests are usually faster, great for regression, demonstrate usage (especially edge cases), etc. Also, the general goal of fuzzing is catching assertion failures, integer over
...

💬 maflcko commented on pull request "guix: disable libsanitizer in Linux GCC build":
(https://github.com/bitcoin/bitcoin/pull/33780#issuecomment-3487281858)
lgtm ACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07

📝 hebasto opened a pull request: "clang-tidy: Remove no longer needed NOLINT"
(https://github.com/bitcoin/bitcoin/pull/33781)
From https://github.com/bitcoin/bitcoin/pull/33714/files#r2491476516:
> Actually, the `NOLINT` was fixed and can be removed? You've confirmed that it is undeclared on the listed platforms, so it can't be hit by `readability-redundant-declaration`

💬 hebasto commented on pull request "random: scope environ extern to macOS, BSDs and Illumos":
(https://github.com/bitcoin/bitcoin/pull/33714#discussion_r2491540401)
See https://github.com/bitcoin/bitcoin/pull/33781.

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3487335789)
rebased after #30595

💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3487350364)
Rebased to resolve the conflict with the merged bitcoin/bitcoin#30595.

💬 maflcko commented on pull request "clang-tidy: Remove no longer needed NOLINT":
(https://github.com/bitcoin/bitcoin/pull/33781#issuecomment-3487353719)
lgtm ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d