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

...

💬 da1sychain commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#issuecomment-3487958150)
Good catch and thank you for the review @danielabrozzoni!! I'll go ahead and fix that.

📝 maflcko opened a pull request: "util: Allow Assert() in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785)
Without this, compile warnings could be hit about `__func__` being only valid inside functions.

```
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
```

Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r24862
...

💬 l0rinc commented on pull request "clang-tidy: Remove no longer needed NOLINT":
(https://github.com/bitcoin/bitcoin/pull/33781#issuecomment-3488018942)
I wanted to ask the same on the original PR but forgot - ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d