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

💬 w0xlt commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#issuecomment-3487454229)
Approach ACK

👍 pablomartin4btc approved a pull request: "Modernize custom filtering"
(https://github.com/bitcoin-core/gui/pull/899#pullrequestreview-3418100790)
re-ACK e15e8cbadad5ce1de41ebb817b87054f8b5192f2

_( moved the early return in `TransactionFilterProxy::setSearchString` to the top )_

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3487500879)
Yeah, overflowing u64 is conceptually no different and theoretically no harder than overflowing u32. However, in practise (using real-world sizes), it can be assumed with some confidence to not happen.

I think you are right that ideally CheckedMul (https://doc.rust-lang.org/stable/std/primitive.u64.html#method.checked_mul) is moved to our `src/util/overflow.h` and all places that have a real risk of overflowing are changed to use this helper.

However, thinking about review and maintenance
...

💬 pablomartin4btc commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2491716003)
nit:
```suggestion
self.log.info(f'Test bitcoind {arg} (using a map file)')
```

🤔 pablomartin4btc reviewed a pull request: "init: Require explicit -asmap filename"
(https://github.com/bitcoin/bitcoin/pull/33770#pullrequestreview-3418202432)
utACK cfeb160baec1369452c42d05c51f2a6af76ed077

If it won't be a default asmap filename, perhaps we could remove "default" from the test function names in `test/functional/feature_asmap.py`?

Left another [nit](https://github.com/bitcoin/bitcoin/pull/33770/files#r2491716003)...

💬 sipa commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3487578047)
@maflcko Thinking out loud, and not really commentary on this PR.

But if the reasoning is that identifying the places where a "non-overflowing type" (uint64) ought to be used is easier than identifying the places where a "non-overflowing multiply" (CheckedMul) needs to be used, then one possibility is creating a wrapping integer type that just has no non-overflying multiplication (or other) operations.

This is likely far overkill for what we're trying to achieve, though.