💬 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

💬 hebasto commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#discussion_r2492055139)
I'd say since the following https://github.com/bitcoin/bitcoin/pull/32881 :)

💬 hebasto commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#discussion_r2492063103)
Thanks! Fixed.

💬 hebasto commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3488036459)
The [feedback](https://github.com/bitcoin/bitcoin/pull/32773#discussion_r2491364489) from @m3dwards has been addressed.

💬 l0rinc commented on pull request "Improve SHA256 x86 SHANI code safety and consistency (no functional changes)":
(https://github.com/bitcoin/bitcoin/pull/33784#issuecomment-3488041714)
@Mystique85 the PR is lacking a purpose and it's obviously vibecoded, while it's not clear what problem it's solving and why this alternative would be any better.
Please only open PRs that are solving a real problem and make sure the solution is not this subjective.

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#issuecomment-3488042681)
Can be tested via the following diff:


```diff
diff --git a/src/logging.cpp b/src/logging.cpp
index 2ed6835197..890e461cb9 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -20,6 +20,7 @@
using util::Join;
using util::RemovePrefixView;

+const int g_FOO{Assert(false)};
const char * const DEFAULT_DEBUGLOGFILE = "debug.log";
constexpr auto MAX_USER_SETABLE_SEVERITY_LEVEL{BCLog::Level::Info};

```

Or with a function name (slight change in behavior):

```diff
diff --
...

💬 maflcko commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2492071472)
Fixed in https://github.com/bitcoin/bitcoin/pull/33785. You should be able to push your previous `*Assert(WITH_LOCK(...))` after it.