💬 stickies-v commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#discussion_r2095909033)
nit: `chrono` and `string` are not required according to [IWYU](https://api.cirrus-ci.com/v1/task/4921334028828672/logs/ci.log)

💬 stickies-v commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#discussion_r2095917052)
nit: `cstring` and `string` includes can just be dropped according to [IWYU](https://api.cirrus-ci.com/v1/task/4921334028828672/logs/ci.log)

💬 stickies-v commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#discussion_r2095910935)
nit: the `span.h` include can now be removed according to [IWYU](https://api.cirrus-ci.com/v1/task/4921334028828672/logs/ci.log)

👋 l0rinc's pull request is ready for review: "script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532)

💬 maflcko commented on pull request "ci: Move DEBUG=1 to centos task":
(https://github.com/bitcoin/bitcoin/pull/32560#discussion_r2095935838)
No reason. I'd say anything is fine here.

Initially this was set in 14788fbadac3aa352eb51da81ecdfa01208ca33c, but we have a larger cache now.

👋 hebasto's pull request is ready for review: "Modernize use of UTF-8 in Windows code"
(https://github.com/bitcoin/bitcoin/pull/32380)

💬 maflcko commented on pull request "Use WitnessV0KeyHash in TestAddAddressesToSendBook":
(https://github.com/bitcoin-core/gui/pull/875#issuecomment-2891405513)
can someone pls assign 30.0 milestone?

💬 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