💬 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

📝 theStack opened a pull request: "test: remove obsolete `get_{key,multisig}` helpers from wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/33782)
This small cleanup PR is a late follow-up to #31250 (commit c847dee1488a294c9a9632a00ba1134b21e41947). These helpers are unused and wouldn't work anymore, as they call a legacy wallet RPC (`dumpprivkey`). They were only ever used for testing the `importmulti` RPC, which also doesn't exist anymore. Functional tests that need to create key pairs and derive various output script types from them can use `get_generate_key` (introduced in #16528, commit f193ea889ddb53d9a5c47647966681d525e38368) instea
...

💬 maflcko commented on pull request "Modernize custom filtering":
(https://github.com/bitcoin-core/gui/pull/899#issuecomment-3487436178)
re-review ACK e15e8cbadad5ce1de41ebb817b87054f8b5192f2 🌿

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK e15e8c
...