💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2329956805)
In d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

`MULTIPATH_RE` is unused.

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330109213)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

Before spending, would be good to check that wallets agree on the received UTXO.
```python
# Check that the wallets agree on the received UTXO
utxo = None
for wallet in wallets:
if utxo is None:
utxo = wallet.listunspent()[0]
else:
assert_equal(utxo, wallet.listunspent()[0])
```

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330153004)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

```diff
- # Construct and import each wallet's musig descriptor
+ # Construct and import each wallet's musig descriptor that
+ # contains private key of that wallet and public keys of others
```

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330145621)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

Nit for clarity:
```diff
- exp_key_leaf = 0
+ expected_key_leaves_count = 0
```

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330311229)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

This is a pretty packed function and slightly difficult to follow - can consider splitting this function into 2 because it can be noticed that first portion is preparing the wallets for spending and the second portion is spending from the musig address while asserting on the relevant data.

<details open>
<summary>Function Splitting Diff</summary>

```diff
diff --git a
...

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2330140614)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

Nit - a candidate for the class param instead of retrieving in every case:
```python
self.def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
```

🤔 rkrux reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3196105850)
Code review 1 - d23116987d19587746d392895c06f5c426c1d0d2

Reviewed the functional test.

💬 fjahr commented on pull request "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`":
(https://github.com/bitcoin/bitcoin/pull/33312#issuecomment-3266740087)
ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920

Looks cleaner than the alternative #33281 with the limited scope.

Does it actually make sense at all to run `clang-tidy` on files that are auto generated by an upstream library? I guess that discussion is out of the scope for this PR/v30 but I am wondering what the upside of that is...

📝 fanquake opened a pull request: "guix: strip binaries in libexec"
(https://github.com/bitcoin/bitcoin/pull/33342)
#31679 moved some internal binaries to `libexec/`, but the Guix build wasn't updated to stip these binaries of their debug symbols.

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3266861514)
In 8849f574104d920c1629e756c5495e1d2c523844 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"

In commit message:
> Lastly, if the partial signatures could be
created, add our own public nonces for the private keys that we know, if
they do not yet exist.

Is is supposed to say "... partial signatures could not be created ..." instead? Based on the tone in the message used.

🤔 marcofleon reviewed a pull request: "[29.x] *san CI backports"
(https://github.com/bitcoin/bitcoin/pull/33294#pullrequestreview-3197068612)
ACK 7c6be9acae5a16956a7f8e53ae3f944a187a6713, looks okay to me

💬 fanquake commented on pull request "cmake: Install internal binaries to <prefix>/libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3266901778)
Followup to strip `libexec/` bins in ##33342.

💬 fanquake commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#issuecomment-3266908097)
cc @m3dwards

👍 ryanofsky approved a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3197074943)
Code review ACK 1d49346465de8edec85bb2d498859b4fbc346935. Makes sense to try IWYU in a few directories like this to see how stable it is and make sure it won't cause problems.

💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330621699)
In commit "ci: Do not patch `leveldb` to workaround UB in "tidy" CI job" (b495e2dc95db42e3219720f5beeb6c136d067a4a)

Could the comment clarify what the issues are? Even just a hint or pointer to more information would be helpful. if they are IWYU issues, maybe just say "to avoid IWYU issues"

💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330635225)
In commit "ci, iwyu: Treat warnings as errors for specific directories" (1d49346465de8edec85bb2d498859b4fbc346935)

I think it'd be a little better to called this ENFORCED_IWYU instead of FORCED_IWYU. Both make sense but former seems a little clearer.

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330683445)
> Not yet.

Is there anything blocking that? It'd be good if we'd atleast report issues upstream, before adding workarounds (with little-to-no documentation).

💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3266988266)
All ci jobs are passing now that the new `tool_bitcoin.py` test is disabled on windows, so this PR should be ready to review. I'll set up a vm and address the windows test problem in a followup PR, since I think the _wexecvp/stdout issue might not be trivial to fix.

<!-- begin push-24 -->
Updated 0c82805f75438c38f5a7f942df2a96ec3d61d68e -> f9685d6a1389938b0cceb31d9eef201ab3dd2e86 ([`pr/ipc-auto.23`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-auto.23) -> [`pr/ipc-auto.24`](https://gi
...

💬 mzumsande commented on issue "use -loadblock to load blk*****.dat files, but the blocks in it are not recognized":
(https://github.com/bitcoin/bitcoin/issues/33280#issuecomment-3266992579)
I looked at the code, and it appears that `loadblock` currently only works with non-xor'ed block files:
[AutoFile file{fsbridge::fopen(path, "rb")};](https://github.com/bitcoin/bitcoin/blob/3eea9fd39532eeda648e44de365fc4c9112f6fc6/src/node/blockstorage.cpp#L1253C9-L1254C1) doesn't use an obfuscation key.

I don't know if that was discussed when block obfuscation was added (FYI @maflcko).
`loadblock` is probably not a feature that is used very much these days, but unless someone wants to add `
...

🚀 fanquake merged a pull request: "[29.x] *san CI backports"
(https://github.com/bitcoin/bitcoin/pull/33294)

👍 ryanofsky approved a pull request: "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`"
(https://github.com/bitcoin/bitcoin/pull/33312#pullrequestreview-3197272526)
Code review ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920. Thanks for the fix!

It could make sense to make a similar change in the libmultiprocess repository, so the `src/ipc/libmultiprocess/` directory would be covered and this fix could be moved to `src/ipc/capnp/` instead of `src/ipc/`, but current approach seems simpler and in the long run when https://github.com/capnproto/capnproto/pull/2334 rolls out, or the LLVM isInMainFile check works better, this can be dropped.

Not running clang-
...