💬 brunoerg commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2491199977)
084bfbc1ec7f8f64f54d231bb641285622311b59: nit: you can get rid of `num_iterations`.
```suggestion
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10) {
```

💬 stringintech commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#issuecomment-3486895544)
3e7d272 to 67740df - rebased and resolved conflict with #30595.

📝 fanquake opened a pull request: "depends: add zeromq patch to fix mingw CMake file install location"
(https://github.com/bitcoin/bitcoin/pull/33778)
Currently the zeromq CMake files for mingw are installed to `x86_64-w64-mingw32/CMake`, when they should be in `x86_64-w64-mingw32/lib/cmake`, so take the patch from upstream that puts them in the right place. Noticed this while looking at other depends changes, that might cleanup / remove dirs.

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3486939962)
I think we've never seen a header-version specific bug in the windows-cross compilation, so trying to fit the major versions in the CI on a best-effort basis seems sufficient for now? If more specific matching is needed for better testing a specific scenario, or for llvm builds, it seems sufficient to do as a follow-up?

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