💬 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

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