⚠️ Raimo33 opened an issue: "bench: broken CSV format, commas in benchmark names"
(https://github.com/bitcoin/bitcoin/issues/33331)
Currently there are a couple of benchmarks that have commas in their name:

- SHA256D64_1024_AVX2 using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
- SHA256D64_1024_SHANI using the 'sse4(1way),sse41(4way)' SHA256 implementation
- SHA256D64_1024_SSE4 using the 'sse4(1way),sse41(4way)' SHA256 implementation
- SHA256D64_1024_STANDARD using the 'standard' SHA256 implementation
- SHA256_32b_AVX2 using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
- SHA256_32b_SHANI u
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2328739222)
Ah, I seem to have only applied this to `RevertBlock` but not `CustomAppend`, both have this identical LOC. Fixed.

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2328743555)
> const uint256 prev_hash_serialized{utxo_stats.hashSerialized}; seems like it would be better.

Taken

📝 fjahr opened a pull request: "common: Make arith_uint256 trivially copyable"
(https://github.com/bitcoin/bitcoin/pull/33332)
Makes `arith_uint256`/`base_uint` trivially copyable by removing the custom copy constructor and copy assignment operators. Removing of the custom code should not result in a change of behavior since `base_uint` contains a simple array of `uint32_t` and compiler generated versions of the code could be better optimized.

This was suggested by maflcko here: https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3186533494

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3263892384)
> I haven't checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.

Correct, I confirmed this with `static_assert(std::is_trivially_copyable_v<DBVal>)`. I am not sure how to test that there are additional effects from this.

> we should probably make it trivially copyable, but this seems unrelated.

This seems like a good change to me but I have[ opened a separate PR](https://github.com/bitcoin/bitcoin/pull/33332) for
...

💬 systemic-threat commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3263902960)
> Gloria destroying bitcoin.

Yep. I've done some programming; I'm not knowledgeable enough to contribute to Bitcoin Core - yet - but I know about good design, and I know how to spot bad actors.

📝 l0rinc opened a pull request: "RFC: coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333)
### Summary

Oversized allocations can cause out-of-memory errors or heavy swapping, [grinding the system to a halt](https://x.com/murchandamus/status/1964432335849607224).

### Fix

Added a minimal system helper to query total physical RAM on Linux/macOS/Windows.

`CalculateCacheSizes()` now emits a startup warning if the configured `-dbcache` exceeds a cap derived from system RAM.
If total RAM < 2 GiB, cap is `DEFAULT_KERNEL_CACHE` (`450MiB`), otherwise it's 75% of total RAM.
We're n
...

💬 l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2328799855)
I agree the type name is weird, but it's wrong now that it's not a lits (I understand you mean it's a conceptual list, not a type, but I also meant a conceptual queue, not a list) - but we can adjust that later, I understand the conflict argument. Reverted the name

💬 l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2328799867)
I remember you said that, but we're not popping from the jobs - but the assert is better anyway, thanks for the hint, added you as coauthor.

💬 l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2328799879)
Done

🤔 sipa reviewed a pull request: "Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3194519714)
Some comments already, mostly to get familiar with the overall flow of changes.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328774181)
In commit "Do not allow mempool clusters to exceed configured limits"

Optimization: instead of constructing an `std::set` of deduplicated input txids, I think it's more efficient to do the `GetIter`/`m_to_add.find` steps for all inputs. `TxGraphImpl` is likely more efficient at deduplicating `AddDependency` calls than the caller is using `std::set` here.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328765241)
In commit " Create a txgraph inside CTxMemPool"

Introduce a named constant for these values? At least for the `10'000`, which survives into the final commit of this PR.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328760872)
In commit "Allow moving CTxMemPoolEntry objects, disallow copying"

This is taking a redundant extra step: `emplace_back` allows constructing the `CTxMemPoolEntry` object directly in-place inside `mempool_entries`, but by invoking the constructor, it's constructed in the call site, and then moved in place:
```c++
mempool_entries.emplace_back(fuzzed_data_provider, tx, current_height);
```

With this, there is (as of this commit) no need to support moving `CTxMemPoolEntry` anymore, though f
...

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328763664)
In commit "Make CTxMemPoolEntry derive from TxGraph::Ref"

Nit: you can invoke the default constructor using just `{}`:
```c++
AddToMempool(pool, CTxMemPoolEntry({}, tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
```

here, and throughout this commit. Just a nit, as I see these invocations are rewritten anyway in a later commit.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328773469)
In commit "Do not allow mempool clusters to exceed configured limits"

Tiny optimization: instead of constructing and returning a `std::vector<const TxGraph::Ref*> ret` here, it's possible to invoke `m_pool->m_txgraph->AddDependency` directly.

If done, it's probably clearer to inline `CTxMemPool::ChangeSet::CalculateParentsOf` into `CTxMemPool::ChangeSet::ProcessDependencies` below.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328797770)
In commit "Select transactions for blocks based on chunk feerate"

I don't think this TODO belongs here, as it's functionality that could be added as an optimization to `TxGraphImpl` without `BlockAssembler` needing to be aware.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328795859)
In commit "Select transactions for blocks based on chunk feerate"

Small optimization: move the definition of `mempool_txs` out of the `while` loop here, and just `.clear()` it here, to avoid an allocation per included chunk.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328784119)
In commit "Limit mempool size based on chunk feerate"

This structured binding will, if I understand it correctly, cause `worst_chunk` to be a **copy** of the vector returned in the `m_txgraph->GetWorstMainChunk()` invocation:
```c++
auto& [worst_chunk, feeperweight] = m_txgraph->GetWorstMainChunk();
```

(with `&` after `auto`) will avoid the copy.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328794751)
In commit "Select transactions for blocks based on chunk feerate"

Since `CFeeRate` now just encapsulates a `FeePerVSize`, it should be unnecessary to invoke `GetFee` (which involves an integer division), and instead it should be possible to compare `chunk_feerate_vsize` with `m_options.blockMinFeeRate.m_feefrac` directly, though I don't think there is an exposed way of doing so currently. Adding a `CFeeRate::GetFeePerVSize()` should be sufficient.