💬 djkazic commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3292126185)
Upgraded from v29.1 to v30.0rc1 on two nodes. Everything has been stable for the last 2 days.

👍 TheCharlatan approved a pull request: "Add a "tx output spender" index"
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-3224343678)
ACK 845b54defebc1987b9d654ea7b611a4ddebe345a

I previously wrote software that had to add a bunch of extra logic to accurately react when a certain output was spent. This would have made my life considerably easier.

💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2348820869)
Nit: Mark this and `CreateKeyPrefix` as `static`.

💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2348904680)
Similar to the blockfilterindex, I would suggest a brief comment here on the schema. Something like (though this reads a bit clunky):
The database stores a siphash of a transaction out point in pairs with the flat file position on disk the transaction is stored in. It is key-only and supports retrieving a transaction by one of its input's out point.

💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2348828075)
Nit (format): Extra whitespace on this block.

💬 TheCharlatan commented on pull request "test: Add submitblock test in interface_ipc":
(https://github.com/bitcoin/bitcoin/pull/33380#issuecomment-3292195172)
Updated 557644ee9499583b6d00efda289fb65e8359e084 -> 0a26731c4cc182e887ce959cdd301227cdc752d7 ([interface_ipc_submitblock_0](https://github.com/TheCharlatan/bitcoin/tree/interface_ipc_submitblock_0) -> [interface_ipc_submitblock_1](https://github.com/TheCharlatan/bitcoin/tree/interface_ipc_submitblock_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/interface_ipc_submitblock_0..interface_ipc_submitblock_1))

* Added @Sjors's [suggested patch](https://github.com/bitcoin/bitcoin/pull
...

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