💬 stickies-v commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2348947467)
> Maybe worth pulling the below snippet into a helper so it can be reused here.

Agreed, a carved out function that just calculates and clamps `db_cache_bytes` makes most sense, and I think we should get rid of `ShouldWarnOversizedDbCache`.

💬 stickies-v commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2348956416)
> Are you anticipating an error here or is this just a way to signal that we don't really support systems with less than 1 GiB? I left this out

In my diff, I'm left shifting `total_ram` later so I wanted to ensure no UB happened. `1<<20` is 1 MiB so I don't think we need to handle that with anything more complex than an assert.

💬 hebasto commented on pull request "cmake: exclude secp256k1 from all":
(https://github.com/bitcoin/bitcoin/pull/33390#issuecomment-3292109747)
CI fails:
```
The following tests FAILED:
4 - secp256k1_noverify_tests (Not Run)
5 - secp256k1_tests (Not Run)
6 - secp256k1_exhaustive_tests (Not Run)
Errors while running CTest
```

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