💬 fjahr commented on pull request "net: Add interrupt to pcp retry loop":
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330502356)
nit: Could add a simple test where the interrupt is instantly called to make sure the line is covered somehow.

💬 fjahr commented on pull request "net: Add interrupt to pcp retry loop":
(https://github.com/bitcoin/bitcoin/pull/33338#discussion_r2330498571)
nit: comment above lists all the conditions for the wait to exit so it should list interrupt as well.

💬 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_r2330027258)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

Can consider using this `change_addr` below like `addr` is used to have fewer moving parts.

```diff
- psbt = wallets[0].send(outputs=[{def_wallet.getnewaddress(): 5}], inputs=[utxo], change_type="bech32m")["psbt"]
+ psbt = wallets[0].send(outputs=[{def_wallet.getnewaddress(): 5}], inputs=[utxo], change_address= change_addr)["psbt"]
```

💬 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_r2330014983)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

Because the imported `musig` descriptor was an active one, the change address that is created here is also musig that leads to a `outputs` property in the decoded PSBT that is untested at the moment unlike the `inputs` section that is asserted on down below.
IMO we should check for the musig properties in the `output` section too.

💬 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_r2330079926)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"

For the last 4 patterns, I see the following error:
```
test_framework.authproxy.JSONRPCException: Can't get descriptor string. (-4)
```

if the private descs are listed immediately after importing musig descriptor:
```python
wallets[0].listdescriptors(True)["descriptors"]
```

The first 5 patterns work fine - I am guessing because of this default from the previous
...

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