💬 fjahr commented on issue "Intermittent CI network issue downloading assets.json from GitHub":
(https://github.com/bitcoin/bitcoin/issues/33599#issuecomment-3393671508)
#32575 is now completely stuck on this, multiple jobs fail even after close/re-open. Seems like this has gotten worse if this was only hitting single jobs previously...

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423143691)
Yes.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423144701)
Indeed unrelated.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423148286)
Without `m_quality` and `m_setindex` private, I think this would be confusing. They feel like they belong together.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423151243)
Unrelated, indeed.

Also, I wouldn't be surprised if the cost of a CTZ to find the next set bit, and the less predictable access pattern, is greater than the cost of performing two ANDs on unused positions. In either case, I don't think this matters.

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2423151639)
Thanks for the review @kannapoix

mocktime is no longer needed in this test, as it passes without it. The block generation and stats comparison occur within the same run, ensuring consistency. Remove the mocktime lines entirely for simplicity.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423153015)
I think of the `MIN_INTENDED_TX_COUNT` and `MAX_TX_COUNT` as the best emulation of having `static` versions of `MinIntendedTxCount()` and `MaxTxCount()` (because in C++, class methods cannot be virtual, so they can't be made part of the `Cluster` formal interface). So we're stuck with two versions: a polymorphic instance member function, and a non-polymorphic class member variable.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423159000)
The first two here are sort of expected, as they're only in instantiations with the MultiIntBitSet as set type. There is another fuzz test that verifies the behavior of that instance as correct, and specifically, equivalent to IntBitSet. At higher levels `IntBitSet` is the only one used in actual production code, while `MultiIntBitSet` is only used in tests (for example, in the txgraph fuzz simulation).

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423160407)
I don't think so; it needs a different approach.

The current approach copies in linearization order. A faster approach (which would need additional logic in `DepGraph` too) would return the DepGraphIndex order of transactions, and applies offsets.

I'll consider this in my (not yet pushed) updates to #32545, which introduce a substantial change: allowing linearizations to be non-topological. That currently requires merging in two phases (first copy all transactions, then all dependencies).
...

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423160627)
Ok, resolving.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166549)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166682)
Added a comment in the txgraph.h definition of the function.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166756)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166844)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423167152)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423167564)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423167919)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423168296)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423169215)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3393696660)
Not going to address any further nits, unless I need to repush for other reasons.