👍 ryanofsky approved a pull request: "mining: add applySolution() to interface"
(https://github.com/bitcoin/bitcoin/pull/33374#pullrequestreview-3224409033)
Concept ACK, this does seem like a more general solution than 33372.

💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348861453)
> re: [#33374 (comment)](https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348144204)

Looking at the PR more, since this is just adding a new method and should be backwards compatible, probably nothing in my comment above applies here, other than that I think it's reasonable for applySolution to follow submitSolution in the .capnp file like it does in the .h file, and for the methods to not be renumbered or sorted by number.

💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348937808)
> Another thing that's not great here is that `m_block_template->block` is mutated.

It does seem nicer to not to mutate the block template. And as long as entire block is being serialized and sent over the socket anyway, probably the cost of copying it is not significant.

But one thought is that maybe serializing whole block is not necessary, and this could just return CBlockHeader?

Another idea is that this could be changed to use output parameters, and the client could decide what inf
...

📝 purpleKarrot opened a pull request: "cmake: exclude secp256k1 from all"
(https://github.com/bitcoin/bitcoin/pull/33390)
Instead of setting the EXCLUDE_FROM_ALL target property, pass EXCLUDE_FROM_ALL to `add_subdirectory()`.

This has the following advanteges:

* It is shorter (obviously).
* Target properties are set only in the `CMakeLists.txt` file that defines the target.
* Install rules defined in the subdirectory are excluded as well. This is what we want, because secp256k1 is linked statically.

💬 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