💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2244450698)
Ah yes, in that case:

```
@param[out] has_hardened whether hardened derivation was found
```

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2244453674)
The version in https://github.com/bitcoin/bitcoin/pull/29675/commits/24bcdd27fd6c32646eb87eb8bea6f539bbc54e42 is fine.

💬 maflcko commented on pull request "refactor: remove unused `ser_writedata16be` and `ser_readdata16be`":
(https://github.com/bitcoin/bitcoin/pull/33093#issuecomment-3138711573)
lgtm ACK 0431a690c3a498a1e728c9df34a132ac16177a04

💬 luke-jr commented on pull request "doc: move `cmake -B build -LH` up in Unix build docs":
(https://github.com/bitcoin/bitcoin/pull/33088#issuecomment-3138752685)
(might make sense to mention `ccmake` too?)

💬 instagibbs commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3138794818)
crACK c1574381168573c561ebddf1945d2debefb340f7

💬 instagibbs commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-3138821243)
ACK ea17a9423fb431a86d36927b02d3624f654fd867

I think the doc update gives a decent explanation how this can be used. Code cleanup is nice.

💬 instagibbs commented on issue "doc: Mempool Policy documentation Outdated since TRUC":
(https://github.com/bitcoin/bitcoin/issues/32067#issuecomment-3138855557)
You are correct that this no longer holds with TRUC. The caveat can be added.

💬 fanquake commented on pull request "[28.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33076#issuecomment-3138907944)
Guix Build (aarch64):
```bash
9ef8edcd797ec28b927c50bcab2bb1ab31f5990ff08671081e6a62391c22c089 guix-build-5492e1be3b40/output/aarch64-linux-gnu/SHA256SUMS.part
d269e0b8610943a4eb061088c139f9840ebbdeb3a1884a2d1f2d13967b01ddf6 guix-build-5492e1be3b40/output/aarch64-linux-gnu/bitcoin-5492e1be3b40-aarch64-linux-gnu-debug.tar.gz
4fc9874cd91344348602a91b6ab680293f6aa1afb5bab54147bdcfe3c69c8777 guix-build-5492e1be3b40/output/aarch64-linux-gnu/bitcoin-5492e1be3b40-aarch64-linux-gnu.tar.gz
899d2d
...

💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2244676459)
Second commit was a bit sloppy it seems😔 I'll clean up this function in the next PR

🤔 Sjors reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3074175438)
Reviewed the non-base commits up to e6889565af3afed567e08976b4bc0f0381d0bf8 _sign: Add CreateMuSig2Nonce_, but I skipped over 25c4fd4e50a6d230c959fb0072df04ef7093aa0a _sign: Add CreateMuSig2AggregateSig_.

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244536549)
In 1d4fdeb63792988b4fb93cf8e84142a3a5b34a39 _sign: Include taproot output key's KeyOriginInfo in sigdata_: can you update the description of `taproot_misc_pubkeys` to mention that this can now also include the output key?

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244607495)
In f4b4c704e40cc11663dbdaa156b1775be668a15f _signingprovider: Add musig2 secnonces_: maybe add an `Assume` here?

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244604441)
In f4b4c704e40cc11663dbdaa156b1775be668a15f _signingprovider: Add musig2 secnonces_: needs guard?

```cpp
if (!musig2_secnonces) return std::nullopt;
```

Ditto for `DeleteMuSig2Session`

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244500941)
In 5ff19c37af53f6ebcd69a9973ededcbadd69d3bd _pubkey: Return tweaks from BIP32 derivation_: in BIP32 this is I_L which isn't given a name. In libsecp it's called tweak, so I guess that's fine.

Although maybe `bip32_tweak` is better, so as not to confuse it with the tweaks in taproot.
https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#public-parent-key--public-child-key

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244611331)
In f4b4c704e40cc11663dbdaa156b1775be668a15f _signingprovider: Add musig2 secnonces_: I guess there's no realistic scenario where two different sessions actually need to be merged, but maybe add a comment to point this out.

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244656095)
In ae6889565af3afed567e08976b4bc0f0381d0bf8 _sign: Add CreateMuSig2Nonce_: it's not clear to me how careful we need to be regarding parallel sessions.

If it's not a big deal then I guess the current choice is fine. Since the same key can be used in multiple tap leaves, we need a unique nonce per leaf, which sighash takes care of. We might have multiple participant keys, which `part_pubkey` takes care of.

`script_pubkey` isn't enough to cover address reuse, but _for now_ `ComputeSchnorrSign
...

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2244627473)
In ae6889565af3afed567e08976b4bc0f0381d0bf8 _sign: Add CreateMuSig2Nonce_: maybe sanity check that `part_pubkey` is in `pubkeys`?

👍 rkrux approved a pull request: "test: Use the same mocktime when migrating in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/pull/33104#pullrequestreview-3074495794)
tACK 95c11728f423e1c655439f9248a314f083ef68ef

I did notice the error mentioned in #33096 for the first time yesterday and it didn't appear again on rerunning.

The fix in itself seems fine to me. Prior to this PR, the `test_default_wallet` subtest was setting mock time twice (one directly and another indirectly via the `migrate_and_get_rpc` function), which was confusing too. This fix streamlines the flow in addition to the fix.

Suggested couple changes but can be done later as the issue coul
...

💬 rkrux commented on pull request "test: Use the same mocktime when migrating in wallet_migration.py":
(https://github.com/bitcoin/bitcoin/pull/33104#discussion_r2244737840)
This comment should be removed as `migrate_and_get_rpc` does this internally now.

💬 rkrux commented on pull request "test: Use the same mocktime when migrating in wallet_migration.py":
(https://github.com/bitcoin/bitcoin/pull/33104#discussion_r2244750342)
Can consider the following removal because `migrate_and_get_rpc` seems to be checking for these more or less.

```diff
- backup_path = self.master_node.wallets_path / backup_filename
- assert backup_path.exists()
- assert_equal(str(backup_path), res['backup_path'])
```

https://github.com/bitcoin/bitcoin/blob/8283af13fe869defce1a93fc0935d0b0244edd45/test/functional/wallet_migration.py#L144-L152