💬 fanquake commented on pull request "ci: Move DEBUG=1 to centos task":
(https://github.com/bitcoin/bitcoin/pull/32560#discussion_r2095821189)
Any reason to set this to `-g2`, rather than `-g3` (the default for the Debug build type)?.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095821939)
> Also, I'd say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We'd want to quickly reject those, after assessing the bug has nothing to do with any commit in a release or master branch of Bitcoin Core.

I did not think about this yet. I guess that could save a bug report round-trip, or at least help with reducing confusion a bit. Then again, some peopl
...

💬 laanwj commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2095847319)
From what i could find, this tag covers every version of Windows 10 and 11, not just version 1903+.

👍 ryanofsky approved a pull request: "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2850988140)
Concept ACK bde1fc1289443785e4538c757631d2fa19d5ecd8. Seems like this makes code clearer and faster without changing complexity, and adds tests.

⚠️ fanquake opened an issue: "doc: references to unshipped documentation"
(https://github.com/bitcoin/bitcoin/issues/32565)
There are currently multiple places in the `--help` or error output of `bitcoind`, that we reference markdown documentation, that we don't ship with the binaries. i.e:

https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L525

https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L551

https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L1900

https://github.com/bitcoin/bit
...

🤔 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>`