🤔 furszy reviewed a pull request: "wallet: Fix logging of wallet version"
(https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2851013472)
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa

💬 furszy commented on pull request "wallet: Fix logging of wallet version":
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2891295993)
> With legacy wallets removed, this wouldn't really be possible to test as descriptor wallets all are version 169900.

I was thinking of checking the version during migration, when we load the wallet into memory at the beginning of the process. But np. Small nit.

💬 hebasto commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2891309997)
> From what I can tell, compilers on Linux systems, will be defining `_GNU_SOURCE`...

Is this behaviour documented? If not, why rely on incidental behaviour that might change?

> ... which results in `glibc` defining `_POSIX_C_SOURCE` to `200809L`...

I don't see any causality here: passing `-U_GNU_SOURCE` to an arbitrary compiler on my machine still results in `_POSIX_C_SOURCE` being set to `200809L`.

💬 pinheadmz commented on pull request "[28.x] Backport #31407":
(https://github.com/bitcoin/bitcoin/pull/32563#issuecomment-2891325049)
codesigning hung forever at one point. I SIGINT it and got a possibly helpful error:

```
--> ./detached-sig-create.sh <redacted>
WARNING: Part of the file was not parsed: 37803 bytes
Enter the passphrase for <redacted>:
Enter the passphrase for <redacted>:
WARNING: Part of the file was not parsed: 37803 bytes
Code signature created
WARNING: Part of the file was not parsed: 37803 bytes
WARNING: Part of the file was not parsed: 37803 bytes
Code signature applied
WARNING: Part of the f
...

💬 hebasto commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2095901559)
You're right. I've misread docs...

📝 hebasto converted_to_draft a pull request: "Set minimum supported Windows version to 1903 (May 2019 Update)"
(https://github.com/bitcoin/bitcoin/pull/32537)
This PR sets the minimum supported Windows version to 1903 (May 2019 Update).

This version is the minimum [required](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page#set-a-process-code-page-to-utf-8) to support the UTF-8 code page (CP_UTF8), which is necessary for https://github.com/bitcoin/bitcoin/pull/32380.

Additionally, the `symbol-check.py` script has been amended to verify application manifests for supported OS value.

📝 laanwj opened a pull request: "Use subprocess library for notifications"
(https://github.com/bitcoin/bitcoin/pull/32566)
Builds on #29868 (to have windows support).

This switches the `-*notify` options to use the subprocess library, which is currently used for only the external signer. The main advantage of this is that file descriptor leaks are avoided (#32343). This could also add future flexibility in notifications, for example to bypass the shell (and launch commands with their arguments directly, for safety), or do something with command output (like log it). But for now, this is meant to be one-to-one.

...

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095911064)
This is the error I got previously for constexpr string: https://cirrus-ci.com/task/4531348515323904?logs=ci#L1428
Changed it back to private, thanks.

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