💬 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

💬 fanquake commented on pull request "build: create Depends build type for depends and use it by default for depends builds":
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-3139342071)
40a01e9d30b0c7deabd5996867f248637f3d1a08 has been merged via #32584.

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244981257)
It is just 50 lines, so it should be manageable. Anyone is free to review it file-by-file, if they want to split it up, but I am not sure about splitting it up too much?

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244981786)
thx, done

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244982186)
seems unrelated? But i am happy to review a pull request changing it

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244982876)
Just to make review easier, because you agree that the commit is already large in https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233735207. Will leave as-is for now.

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244984778)
> Looking at the code I understand it of course, but maybe something like:
>
> ```c++
> ElapseTime elapse_time{};
> elapse_time.Advance(777s);
> ```

Not sure. This is doing something else, as explained above by yourself? So this replacement is not correct to apply here.

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244985411)
> are we planning on using the new declared object?

Yes, it can trivially be used, if there is need to. Possibly it isn't used in this specific instance any further, but it seems beneficial to allow it and also beneficial to be consistent in the naming and usage.

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244987390)
I think it should be allowed to have it set previously, as there may be valid use-cases. Using it in multiple threads is not a thing right now and I can't imagine a use-case in the future. However, if there is one, it seems fine to support it and allow it as well.