💬 maflcko commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2095953566)
thx. I don't want to check any explicit values, so I pushed something else.

🤔 l0rinc reviewed a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2851134975)
I thought we're [sunsetting 32 bit](https://github.com/bitcoin/bitcoin/issues/32375) support slowly (it's why I was originally confused by the PR description).
I agree with others that the limits are arbitrary, we should only warn if the values obviously don't fit into a `size_t`.

💬 l0rinc commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095947213)
Same, can be constexpr and could use more descriptive bitness check:
```suggestion
constexpr auto max_db_cache{SIZE_MAX == UINT32_MAX ? MAX_32BIT_DBCACHE : std::numeric_limits<size_t>::max()};
```

💬 l0rinc commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095948822)
Agree with @Sjors, which would also simplify `max_db_cache` to just `std::numeric_limits<size_t>::max()`

💬 l0rinc commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095942544)
we could make it an `if constexpr` as well and use a more descriptive way of checking for 32 bitness:
```suggestion
if constexpr (SIZE_MAX == UINT32_MAX && *mb > MAX_32BIT_MEMPOOL_MB) {
```

💬 l0rinc commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095953423)
... and I think it should be a `size_t` instead

💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2096016006)
> Then again, some people are just forking the project on master and tag some past version on it. That could be equally confusing, no?

Hopefully that is rare. Though, just including the full commit hash in bug reports shouldn't hurt.

💬 fanquake commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891504575)
Concept ACK. Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.

💬 stickies-v commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2891504937)
re-ACK 7193245cd66791216d4e586ca09302b26d4b7528

💬 fanquake commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2096035158)
Is anything enforcing this, or is the assumption that any Windows 10 system is this at least version? What happens if it isn't?

💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2096044476)
> Is anything enforcing this, or is the assumption that any Windows 10 system is at least this version?

The latter.

> What happens if it isn't?

My guess is that the application won't properly handle any symbols outside its default code page.

💬 ryanofsky commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891540520)
I think goals of this PR should be achievable without too much work, but unfortunately the implementation as of 39a3b5be3d138c8f3f7cc4d5bd22c1bd42f4d525 isn't really compatible with IPC so a different approach needs to be taken. There are two problems currently:

1. First, the pure virtual methods cannot be marked const. In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may acc
...

💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2891542065)
re-ACK 33332829333b589420f8038541d04ec6970f051d

💬 laanwj commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891561571)
i'm not 100% sure `ENABLE_SUBPROCESS` needs to exist as an option at all, btw, it may be something to always enable, but maybe not on mobile/sandboxed platforms so wasn't sure whether to make that decision here.

💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2096072843)
> I can't see any

Most usages have a hash available, but I'm not sure about e.g. [`LoadExternalBlockFile`](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5166) - but even if I would, I'd prefer tackling it separately, I'm not comfortable adding new exceptions to consensus code.

💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891585620)
Concept ACK.

> Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.

I agree. And we are not going to switch to C++26 any time soon when #32380 will be required.

✅ ryanofsky closed an issue: "rpc: actually deprecate `rpcuser` & `rpcpass`"
(https://github.com/bitcoin/bitcoin/issues/29240)

🚀 ryanofsky merged a pull request: "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory"
(https://github.com/bitcoin/bitcoin/pull/32423)

💬 theStack commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891709530)
Concept ACK

👍 willcl-ark approved a pull request: "doc: remove // for ... comments"
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2851493004)
reACK 7193245cd66791216d4e586ca09302b26d4b7528