💬 HowHsu commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3292198687)
I can confirm that this issue is true, during my testing for my PR： [https://github.com/bitcoin/bitcoin/pull/32791](url) .
To test ConnectBlock() realistically, I think we can leverage `CVerifyDB::VerifyDB()`, it is called on node init. It mainly replays
the latest `-check_blocks` blocks in the best chain when you set `-check_level` to 4. Considering cache state, you have to manually
set it at the begining (flush it for no-cache case, pre-load entries for cache-hit case, .etc).Surely you have to
...

💬 pinheadmz commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3292272617)
- Running 30.0rc1 on 32-bit ARM RPi with LND
- Running 30.0rc1 on x86 Ubuntu 20 `bitcoin-node` with Sv2 template provider and my own [idiotic block-art web stuff](https://thebitcoinblockclock.com/btc/)
- Checked codesigned GUI on macos/ARM, will go through the testing guide
- Will run IBD on Debian 12 VM as well with `-coinstatsindex=1`

👋 pinheadmz's pull request is ready for review: "Remove unnecessary casts when calling socket operations"
(https://github.com/bitcoin/bitcoin/pull/33378)

📝 ryanofsky opened a pull request: "test: Prevent disk space warning during node_init_tests"
(https://github.com/bitcoin/bitcoin/pull/33391)
mzumsande pointed out https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3286964369 that this test was print a warning:

```
Warning: Disk space for "/tmp/test_common bitcoin/node_init_tests/init_test/bf78678cb7723a3e84b5/blocks" may not accommodate the block files. Approximately 810 GB of data will be stored in this directory.
```

Fix by setting regtest instead of mainnet network before running the test.

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3292311207)
re: https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3286964369

> Maybe covered by the previous comments, but running `test_bitcoin` on master now prints out a warning, caused by `node_init_tests`:
>
> ```
> Running 671 test cases...
> Warning: Disk space for "/tmp/test_common bitcoin/node_init_tests/init_test/bf78678cb7723a3e84b5/blocks" may not accommodate the block files. Approximately 810 GB of data will be stored in this directory.
> ```

Thanks! Should be fixed in #333
...

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