💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104493385)
in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: nit, could use the helper here:
```suggestion
if (IsRangedDerivation() || !m_path.empty()) {
```

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104489088)
in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: the byteswap comment is obsolete now
```suggestion
//! MuSig2 chaincode as defined by BIP 328
using namespace util::hex_literals;
constexpr uint256 MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
```

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104518220)
in commit 739f13986a49d8d7d5568ebcdd88665ea8423d42: could add a doxygen comment for the newly introduced parameter

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104576393)
Why is the `key_exp_index` parameter changed to pass by reference? Without that the unit tests fail, so it seems necessary, but I'm confused why this was not needed earlier already for similar constructs (e.g. for `multi()` expressions where the value is also incremented in the key expression parsing loop). Probably I'm missing some basic descriptors parsing knowledge.

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2104615891)
@hebasto compelling enough to ACK as is?

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2104621096)
Took me longer than anticipated, but I have measured different scenarios here to understand where alignment matters and where it doesn't, with gcc, clang and apple clang

GCC 14 is 38.5% faster with alignment & 64 byte unroll compared to just a 64 bit xor

<details>
<summary>------ C++ compiler .......................... GNU 14.2.0 ------</summary>

```bash
rm -rf build && cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ >/dev/nul
...

👍 theStack approved a pull request: "Musig2 fields followups"
(https://github.com/bitcoin/bitcoin/pull/32412#pullrequestreview-2864492651)
ACK 10fd2aeb62469510c115362f801c08563421c304

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2104639128)
Done

💬 pinheadmz commented on issue "getchaintips doesn't mark headers-only chain as invalid":
(https://github.com/bitcoin/bitcoin/issues/8050#issuecomment-2904516982)
I believe this issue was closed by https://github.com/bitcoin/bitcoin/pull/30666

@mzumsande

🤔 furszy reviewed a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2864553336)
Should also change [HasLegacyRecords()](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/walletdb.cpp#L544) as it would log the same error twice with this change.

✅ pinheadmz closed an issue: "getchaintips doesn't mark headers-only chain as invalid"
(https://github.com/bitcoin/bitcoin/issues/8050)

💬 pinheadmz commented on issue "getchaintips doesn't mark headers-only chain as invalid":
(https://github.com/bitcoin/bitcoin/issues/8050#issuecomment-2904551865)
Confirmed by running a test [I wrote for a draft PR](https://github.com/bitcoin/bitcoin/pull/27434/files#diff-8bd914231171fe351d1ea5bcb566576fad26da2d3d4c4c918b352871fd84809c)

before #30666:
```
[{'height': 20, 'hash': '2182ec6e445c3869f7d17bcf2bcae03c286256cf7359b3c7c834cee3ad029a3f', 'branchlen': 10, 'status': 'headers-only'}, {'height': 10, 'hash': '2e13e49c2f95b6b17c8eece52ce971ce5610a9c8cd824b7be139fcbadae6d0b2', 'branchlen': 0, 'status': 'active'}]
[{'height': 20, 'hash': '2182ec6e445c386
...

🤔 rkrux reviewed a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2864642456)
ACK 62ad4d80883191073ea39f8b8d5ccef0748414a1

> Should also change [HasLegacyRecords()](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/walletdb.cpp#L544) as it would log the same error twice with this change.

Can be removed as I checked that the only other usage of `HasLegacyRecords` is [directly inside](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/wallet.cpp#L4315) the `MigrateLegacyToDescriptor` fu
...

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104750615)
> A better option would be to use the test's temp dir as scratch space

That doesn't work with the currently used `std::string` overload of `subprocess::Popen()` because of to the space in the full path. Is using `fs::temp_directory_path()` acceptable?

👍 theStack approved a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591#pullrequestreview-2864682937)
ACK 8fcd6845052354fad80ae7e5feda3f6a2e441e12


Diff of `feature_block.py` test runs between merge-base and PR (patched out the timestamps from the [logging formatter](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/test/functional/test_framework/test_framework.py#L833) in order to compare):
```
> TestFramework (INFO): Reject block with invalid tx: InvalidOPIFConstruction
14a16,30
> TestFramework (INFO): Reject block with invalid tx: DisabledOpcode_OP_CAT

...

💬 theStack commented on pull request "test: fix and augment block tests of invalid_txs":
(https://github.com/bitcoin/bitcoin/pull/32591#discussion_r2104754535)
nit: alternatively, could just remove the line since `False` is the default anyways, but no blocker

🤔 pinheadmz reviewed a pull request: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051#pullrequestreview-2864676242)
concept ACK and untested code review 93b07997e9

I believe this also closes an 11-year-old issue: https://github.com/bitcoin/bitcoin/issues/5097

💬 pinheadmz commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r2104750539)
64b956f4220300254126b166deb97849b236c2ed

Nit: could you use `NetPermissions::ToStrings(NetPermissionFlags flags)` just to avoid hard coding things?

💬 pinheadmz commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2904687014)
> I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?

If we're considering a case where the user specifically chooses specific peers for IBD for privacy or whatever reasons, don't we just let them have the slow sync they trade-off for?

👍 rkrux approved a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596#pullrequestreview-2864719763)
ACK e5cbea416b2f63e5d99819052f3e69a6383336d6