💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095911559)
I'm against throwing in general - especially for constructors, but we're already reading files in there and already throwing via `HandleError` - changed and moved to the related refactoring commit.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095911656)
✨ ₘₐ𝓖ᵢ𝒸 ✨

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095911786)
In my defense, I was hungry 🥩

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095912020)
The `new`/`old` are wrong indeed - but the explanation for why we're using a different constructor here needs local explanation - reworded, thanks

💬 hebasto commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2095916084)
In its current state, this PR is not necessary for introducing the UTF-8 code page support.

✅ hebasto closed a pull request: "Set minimum supported Windows version to 1903 (May 2019 Update)"
(https://github.com/bitcoin/bitcoin/pull/32537)

👍 stickies-v approved a pull request: "doc: remove // for ... comments"
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2851071402)
ACK e4aee0b3a23a9609e16bd7c8eda618dcf0a8b1db

The new includes seem arbitrary, they usually still don't represent the full include list (according to [IWYU](https://api.cirrus-ci.com/v1/task/4921334028828672/logs/ci.log)), and could be dropped, but I verified them against the IWYU run and mostly seem fine (I added comments for the ones that seem incorrect.


Found a couple more include comments in `util/fs_helpers.cpp`

💬 stickies-v commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#discussion_r2095904411)
nit: `#include <limits.h>` can be removed according to IWYU I suspect because we also `#include <limits>`

💬 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) {
```