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

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

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988027)
thx, mentioned global in a new struct-level-doc

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988253)
thx, mentioned global in a new struct-level-doc

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

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

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244991616)
> nit: I don't find the operator to be intuitive here, maybe we could call it `Advance` instead?

I think `elapse_time(4h)` or `elapse_steady(4h)` is self-explanatory. So leaving as-is for now.

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