💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123614842)
This PR enables failing fast for executables in pwsh:

https://github.com/bitcoin/bitcoin/pull/32672

💬 i-am-yuvi commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2934979547)
Concept ACK

💬 ryanofsky commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2935063889)
re: theuni https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2923398293

> For context, my primary motivation here is preparing for the follow-up commits that will actually remove the `lock`/`unlock` methods completely, in order to force the exception-safe RAII wrappers. See these two commits: [theuni@d4b0918](https://github.com/theuni/bitcoin/commit/d4b09184ae8ce38862a758423653f606e8b96045) and [theuni@1f2a9af](https://github.com/theuni/bitcoin/commit/1f2a9afdb302dd3f1686fe3461b9d791
...

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123711340)
@davidgumberg force pushed a change so that it behaves how you expected it to: https://github.com/bitcoin/bitcoin/compare/a453e66c827f260da597bf70273bcd0da2313774..7f8393ad745e6bc44dc9481428ea5979ed31a3c9

🤔 hodlinator reviewed a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2891238265)
Concept ACK 12248ee08026263b70b2a0e6857552c475614207

💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123683812)
Regarding removing this support for building from source, I'm not sure about doing that. Yes, we use CMake for newer releases, but the older ones use Auto-tools, no?

💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123124268)
nit: Why rename `platform` to `host`? Seems better to keep them separate to avoid conflating with args.host, or at least leave a small motivation in the commit message.

💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123092808)
nit: Could print to stderr here and elsewhere for touched lines with error messages.
```suggestion
print(f"Failed to extract the {tag} tarball", file=sys.stderr)
```

💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123700602)
nit: Could use more modern f-strings:
```suggestion
archive = f'bitcoin-{tag[1:]}-{host}.{archive_format}'
archiveUrl = f'https://bitcoincore.org/{bin_path}/{archive}'

print(f'Fetching: {archiveUrl}')
```

🤔 rkrux reviewed a pull request: "wallet, rpc: Return normalized descriptor in parent_descs"
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2892155604)
ACK 0def84d407facd319b52826d013cad0d5fc8dbf5

Apparently I didn't have the fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8 commit in my local, maybe I forgot to check it out earlier.
```bash
git range-diff a759a22...0def84d
```

Good job replacing the loop in the test with the regex.

💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123684230)
Nit for the comment: This _removes_ the xpub by substituting it with emptiness, the resultant `desc_verify` contains everything in the descriptor but the xpub.

💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123692569)
```diff
- aspostrophe
+ apostrophe
```

💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123691895)
I checked that this Regex is fine because the origin fingerprint can only be a hex string `(\da-f)` and the `origin_part` is found even if the `desc_verify` doesn't contain hardened marker such as in `wpkh([58d258be/84/1/0]/0/*`. That's why I believe the subsequent `"h" in origin_part` check is required.

💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123713913)
`r"tpub\w+?(?=/)"`

The `\w` will accept an xpub with underscore as well that is not allowed, but ok.

👍 willcl-ark approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2892263099)
ACK 3a350c8a1b51e140e2689e10dca338470e8deef2

Tested that this works. I additionally tested rebasing #32595 on it, where it also works well.

💬 willcl-ark commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2935137626)
guix hashes:

```
src/core/bitcoin on  pr-32665 [$] via △ v3.31.6 via 🐍 v3.12.10 via ❄️ impure (nix-shell-env) took 50m49s
❯ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
32f567efe7c9d428083d9eab15e9fb215e2860132b3a31a60f288d21d3ed3644 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/SHA256SUMS.part
13bfeec126cef80d97a7b90f9cec32962cce8b3bbf445006f1233e1b45a74d43 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/bi
...

📝 maflcko opened a pull request: " clang-tidy: Apply modernize-deprecated-headers "
(https://github.com/bitcoin/bitcoin/pull/32673)
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is confusing, when it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).

The check is currently disabled for headers, to exclude subtree headers.

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2123760984)
in commit 9e3a1d05bf066df31a6e124638046e70669eb470: nit: the `Expr` call seems to be a no-op here, considering that we already extracted the function expression with the parens-splitting above
```suggestion
std::span<const char> expr(split.at(0).begin(), split.at(0).end());
if (!Func("musig", expr)) {
```

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2123695013)
in commit 16a88829e92c2676cae6b6e0acb75d2d26795a6d:
```suggestion
//! Derivation path
const KeyPath m_path;
```
as using non-ranged derivation is also possible (e.g. `musig(...)/1`, we also have a [test for that](https://github.com/achow101/bitcoin/blob/afcd397bc791d047508adb9d151d21384d7b49e3/src/test/descriptor_tests.cpp#L1118)); the BIP 390 specification can be interpreted in a way that only ranged derivation is possible though (https://github.com/bitcoin/bips/blob/master/bip-039
...

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2123808039)
in commit 9e3a1d05bf066df31a6e124638046e70669eb470: nit: A more thorough check would be to detect duplicates of actual resulting pubkeys, rather than only the expression strings (i.e. for a keypair `(priv,pub)`, the expression `musig(priv,pub)` would now still be valid).