💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3486757229)
re-ACK a2365f130c36f386caa8470528f39a34ac394798

Since my last review of e1f139b, commits 8bf2269efa and b094d38454 were absorbed into #33201 which added direct coverage of IPC via a functional test. This also allowed some simplification of `test/functional/interface_ipc_mining.py` which this PR adds.

I initially wanted to suggest folding this test into `interface_ipc.py`, but the latter has a dependency on PyCap.

💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3486760754)
@151henry151

Sorry, but one more rebase is needed.

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486783430)
post-merge ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969

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