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

📝 Raimo33 opened a pull request: "node: optimize CBlockIndexWorkComparator"
(https://github.com/bitcoin/bitcoin/pull/33334)
## Summary

This PR optimizes `CBlockIndexWorkComparator::operator()` by removing 4 redundant branches.

The previous implementation used multiple separate comparisons with explicit branches for greater-than and less-than cases, resulting in unnecessary code paths.

The new implementation consolidates comparisons into single inequality checks and reduces complexity while preserving its original behavior. This change is particularly beneficial for loading blocks from files and reindexing.

...

💬 Raimo33 commented on pull request "coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation":
(https://github.com/bitcoin/bitcoin/pull/31449#issuecomment-3264113178)
Code review ACK 61cde55c56952327d06297a133c30f6877eb62e6

last commit conflicts with https://github.com/bitcoin/bitcoin/pull/33184/ but I'd argue this PR could be merged first.
My current knowledge prevents me from understanding the changes in `test/functional/data/rpc_getblockstats.json` and `test/functional/rpc_getblockstats.py`. Also it seems 433942e840e0b70d5872e5a5f8d84a42fa3ff05a could be split up further

💬 Raimo33 commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#issuecomment-3264116563)
The reason `BlockToJsonVerboseWrite` and `WalletIsMineDescriptors` are slower is due to the fact that they benefit from not computing a comparison twice.

For them specifically, this would be better:
```diff
// First sort by most total work, ...
- if (pa->nChainWork != pb->nChainWork) {
- return pa->nChainWork < pb->nChainWork;
- }
+ if (pa->nChainWork > pb->nChainWork) return false;
+ if (pa->nChainWork < pb->nChainWork) return true;
```

as in the best case scenario it only com
...

📝 sipa opened a pull request: "txgraph: randomize order of same-feerate distinct-cluster transactions"
(https://github.com/bitcoin/bitcoin/pull/33335)
Before, the implicit ordering of transactions in a TxGraph's main graph is sorted by:
- feerate, high to low
- `Cluster`, low to high `m_sequence`
- linearization order within clusters

However, since `m_sequence` values are assigned deterministically and predictably, the ordering of same-feerate distinct-Cluster transactions can reveal something about the order they were inserted into `TxGraph`.

In #28676, this ordering (through `TxGraph::CompareMainOrder`) is used to decide the order o
...

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328896505)
See #33335.

👍 andrewtoth approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3194671681)
re-ACK 4327327dd07457621fe1a9c88dfbf6fccb0e8854

Changes from my last ACK dbd76e68c3fd86814e96be85a2d23eb542df6e55 (`git range-diff 5d17e64a02...dbd76e68c3 7cc9a08706...4327327dd0`):

- ee75225369: grammar
- ead32f37c8: remove txid conversions to resolve conflict
- 9ec10ceb59: convert `Txid` to `uint256`, update import from `util/transaction_identifier.h` to `primitives/transaction_identifier.h` to resolve conflict
- dbd76e68c3: add functional test for rejecting scheduling private broadca
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2328909174)
grammar nit:
```suggestion
self.log.info("Sending a pair of transactions with the same txid but different valid wtxids via RPC")
```