🤔 optout21 reviewed a pull request: "test: don't throw from the destructor of DebugLogHelper"
(https://github.com/bitcoin/bitcoin/pull/33388#pullrequestreview-3224790999)
ACK 7f87cddd36f0456be52d795a53887d53294f824e

I've reviewed, I've executed the unit tests.
The change is Test only.
The relevant change is in `DebugLogHelper` destructor, exception throwing is replaced by print and `abort()`.
Other changes improve documentation, parameters of the class.
Changes are not in separate commits, but changes are small and test-only.

🤔 enirox001 reviewed a pull request: "test/refactor: use test deque to avoid quadratic iteration"
(https://github.com/bitcoin/bitcoin/pull/33313#pullrequestreview-3224804403)
reACK 75e6984

🤔 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-3223415048)
Halfway through the code review - a06017dfce7ce72afbebe6f68d9a29cf72d26593

💬 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_r2348217241)
In 10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

```diff
diff --git a/src/key.cpp b/src/key.cpp
index 005a913236..c312f0713f 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -373,7 +373,7 @@ std::vector<uint8_t> CKey::CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uin
return {};
}

- // Serialize nonce
+ // Serialize pubnonce
std::vector<uint8_t> out;
out.resize(66);
if (!secp256k1_musig_pubnonce_serialize(secp256k
...

💬 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_r2348325505)
In https://github.com/bitcoin/bitcoin/commit/6b01d7379b9e29760abd7b549a04db43937a7b8d "sign: Add CreateMuSig2Nonce"

For the `pubnonce` that we know is of a fixed size of `66` bytes, can't we use a fixed size array instead everywhere, which I find to be more expressive for this use case? I suppose there is no case when we need to use the vector specific properties for a `pubnonce`.

```cpp
std::vector<uint8_t>
```

💬 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_r2348335253)
In 10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

I believe by using a fixed size array for pubnonce as mentioned in an earlier comment can avoid the need for this kind of check.

💬 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_r2348274053)
In https://github.com/bitcoin/bitcoin/commit/10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

We can prefer to fail the process if the `secnonce` was not deleted for some reason.

```diff
@@ -166,6 +166,7 @@ bool MutableTransactionSignatureCreator::CreateMuSig2PartialSig(const SigningPro
partial_sig = std::move(*sig);

// Delete the secnonce now that we're done with it
+ assert(!secnonce->get().IsValid());
provider.DeleteMuSig2Session(sess
...

💬 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_r2348357357)
In 645fcaa83108e6a0faed2c49c72ef710f0231407 "Add MuSig2SecNonce class for secure allocation of musig nonces"

While not opposed to the idea of encapsulating `MuSig2SecNonceImpl` inside `MuSig2SecNonce`, I don't fully understand the need for it. Both the class have copy constructors deleted and the `Get, Invalidate, IsValid` functions directly call the corresponding functions of the `MuSig2SecNonceImpl` impl class without any other code in between.

What would be the downside of having only o
...

💬 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_r2348961437)
In a06017dfce7ce72afbebe6f68d9a29cf72d26593 "test: Test MuSig2 in the wallet"

In the `SignTaproot` function in `src/script/sign.cpp`, key path spending is tried first and then script path spending that makes the key path spending the de-facto spending path in these tests.

Consider updating these tests to allow testing for script path spending in case of a valid key path as well.

<details open>
<summary>Conditional Script path spending suggestion</summary>

```diff
diff --git a/test
...

💬 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_r2348456444)
In https://github.com/bitcoin/bitcoin/commit/10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

Might be opinionated:
I think having a `struct` for the `pubnonce` can add some structure in the overall MuSig signing code. The `66` size checks spread across `CreateMuSig2PartialSig` & `CreateMuSig2AggregateSig` functions seem distracting and IMO can go inside the constructor of the struct that internally can use `secp256k1_musig_pubnonce`.
At the moment, treatment of
...

💬 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_r2348311867)
In 6b01d7379b9e29760abd7b549a04db43937a7b8d "sign: Add CreateMuSig2Nonce"

Can consider creating the constant now in the `musig.h` dedicated file, even though there is no pubnonce object yet - ref https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049062217

```diff
- // Serialize nonce
+ // Serialize pubnonce
std::vector<uint8_t> out;
- out.resize(66);
+ out.resize(MUSIG2_PUBNONCE_SIZE);
```

💬 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_r2348997742)
In 10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

Same `s/hash/sighash` suggestion in both `key.cpp` and `key.h`.

💬 Crypt-iQ commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3292557230)
ACK 7f87cddd36f0456be52d795a53887d53294f824e

💬 purpleKarrot commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3292587091)
> Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages.

This is not precisely what happens. If an exception is emitted from a function marked `noexcept(false)`, the current terminate handler will be invoked. It does not matter whether the destructor
...

🤔 Eunovo reviewed a pull request: "test: Prevent disk space warning during node_init_tests"
(https://github.com/bitcoin/bitcoin/pull/33391#pullrequestreview-3225126615)
Tested ACK https://github.com/bitcoin/bitcoin/pull/33391/commits/bdf01c6f61262cd6e211ead3c0dbc66ccb48b32f:

The message does not appear when you build https://github.com/bitcoin/bitcoin/pull/33391/commits/bdf01c6f61262cd6e211ead3c0dbc66ccb48b32f and run the unit tests, but it appears on master.

Since the test is focused on initialisation and shutdown logic, the effect of using REGTEST chaintype here is minimal.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3292695737)
Thanks for the review @stringintech

Updated e450549ae94736fc727d0d8a4a7a6a80e768ecd2 -> 0fc068b735d267c7ef4a3b23e32dab1771df2509 ([kernelApi_64](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_64) -> [kernelApi_65](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_65), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_64..kernelApi_65))

* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346797667), removed asserti
...

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3292821103)
> Updated the map to that of the latest run: [asmap/asmap-data#30](https://github.com/asmap/asmap-data/pull/30)

FYI for reviewers, new maps are available in [asmap-data](https://github.com/asmap/asmap-data/) and will continue to be updated over the next few months but I don't plan to update the map data that is included here for at least a while. This is by choice: If this had been merged for v30 then this map data might have been included and it would have been the default until the release
...

💬 quapka commented on issue "BIP32: Maximal length of derivation paths and depth of extended keys":
(https://github.com/bitcoin/bitcoin/issues/32201#issuecomment-3292848589)
Linking https://github.com/bitcoinj/bitcoinj/issues/3683, to keep things in one place.

💬 janb84 commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3292868065)
Please squash your commits, like described in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 Sjors commented on pull request "test: Add submitblock test in interface_ipc":
(https://github.com/bitcoin/bitcoin/pull/33380#issuecomment-3292933671)
ACK 0a26731c4cc182e887ce959cdd301227cdc752d7