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

✅ hebasto closed an issue: "build: Qt deprecated-declarations warnings"
(https://github.com/bitcoin/bitcoin/issues/33571)

🚀 hebasto merged a pull request: "Modernize custom filtering"
(https://github.com/bitcoin-core/gui/pull/899)

📝 Mystique85 opened a pull request: "Refactor SHA256 x86 SHANI implementation"
(https://github.com/bitcoin/bitcoin/pull/33783)
Refactored the SHA256 x86 SHANI implementation for improved readability and minor optimization consistency.

Updated static constants to use constexpr for better compile-time evaluation.

Adjusted function signatures to use static ALWAYS_INLINE for consistent linkage and inlining.

Applied reinterpret_cast for pointer conversions, improving type safety and clarity.

No changes were made to the cryptographic logic or algorithmic behavior.

Verified against existing SHA256 test vectors t
...

📝 Mystique85 opened a pull request: "Improve SHA256 x86 SHANI code safety and consistency (no functional changes)"
(https://github.com/bitcoin/bitcoin/pull/33784)
Refactor and clean up the SHA256 x86 SHANI implementation to improve code safety, readability, and consistency without any functional or behavioral changes.

Motivation

This patch improves long-term maintainability and compiler compatibility of the SHANI-optimized SHA256 implementation used in Bitcoin Core.
It replaces C-style casts with reinterpret_cast to enforce type safety, ensures consistent use of static ALWAYS_INLINE linkage for internal helper functions, and converts static constan
...

💬 Mystique85 commented on pull request "Improve SHA256 x86 SHANI code safety and consistency (no functional changes)":
(https://github.com/bitcoin/bitcoin/pull/33784#issuecomment-3487953631)
This PR refactors the SHA256 x86 SHANI implementation to improve code safety, readability, and maintainability **without changing the cryptographic logic or performance**.

The main goals are:
- Use `constexpr` for compile-time constant initialization.
- Use `reinterpret_cast` instead of C-style casts for type safety.
- Apply `static ALWAYS_INLINE` consistently across helper functions.

The implementation has been verified against all SHA256 test vectors (bit-for-bit identical outputs).

...