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

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2123779414)
in commit 9e3a1d05bf066df31a6e124638046e70669eb470: should this be
```suggestion
if (!all_bip32) {
error = "musig(): musig() derivation requires all participants to be xpubs";
```
instead? Ranged would mean ending with `/*` IIUC, which is not necessarily the case.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2123834199)
I believe that if somebody is using `-debug=bench` or `-debug=leveldb`, the fact that they are using a category should bypass the rate limit if `LogDebug` is used. I think the mitigation should be in place whether or not we're in IBD -- if we have any inbound peers, we need to be cautious.

💬 maflcko commented on pull request "doc: Remove build instruction for running `clang-tidy`":
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2935293250)
Shouldn't this be excluded in the tidy CI task as well?

Also, would be good to mention that this will now print errors about the missing auto-generated files.

⚠️ fanquake opened an issue: "guix: nsis (Windows installer creator) is broken upstream"
(https://github.com/bitcoin/bitcoin/issues/32674)
This will become a issue when we want to bump the time-machine next, and it'd be nicer if this is fixed and shipped upstream, rather than us having to work around the issue:

```bash
# guix --version
guix (GNU Guix) 4b9d14378fcc3d8dd4eea36b541fe87e198fd7b8

# guix build --no-substitutes nsis-x86_64
Source/Plugins.cpp:278:18: required from ‘static void PrintPluginDirsHelper::print(const C&, const char*) [with C = std::map<std::__cxx11::basic_string<wchar_t>, std::__cxx11::basic_string<wchar_t>,
...

💬 pinheadmz commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2935375333)
@achow101 re: slow preferred peer "causing congestion" -- wouldn't this only affect the single user who explicitly set the preferred peer?

💬 pinheadmz commented on issue "Potential data race on fHavePruned flag":
(https://github.com/bitcoin/bitcoin/issues/21627#issuecomment-2935386395)
I haven't been focusing on this, wanna take a stab at it?