💬 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.
(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.
(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.
(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).
(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).
...
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3393696660)
Not going to address any further nits, unless I need to repush for other reasons.
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423172535)
> I'm a little unclear what this swap is trying to do.
> Also I don't understand why m_mutex is locked twice in this function with a notify in between
The idea behind the swap was more about ensuring the queue is empty after joining the threads. Mainly to avoid any lingering future that would block callers indefinitely. That's why I used the "sanity" wording there.
And did it after joining the threads because we are currently waiting for all pending tasks to be completed before stopping the
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423172535)
> I'm a little unclear what this swap is trying to do.
> Also I don't understand why m_mutex is locked twice in this function with a notify in between
The idea behind the swap was more about ensuring the queue is empty after joining the threads. Mainly to avoid any lingering future that would block callers indefinitely. That's why I used the "sanity" wording there.
And did it after joining the threads because we are currently waiting for all pending tasks to be completed before stopping the
...
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423193226)
> I don't understand the need for the sanity cleanup. Shouldn't the logic from `WorkerThread` guarantee that `m_work_queue` is empty at this point, so that we could just assert that here?
Yeah, right now it is not really necessary. Have explained the context and the rationale here: https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423172535
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423193226)
> I don't understand the need for the sanity cleanup. Shouldn't the logic from `WorkerThread` guarantee that `m_work_queue` is empty at this point, so that we could just assert that here?
Yeah, right now it is not really necessary. Have explained the context and the rationale here: https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423172535
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423212929)
nice catch. Fixed.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423212929)
nice catch. Fixed.
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423229107)
A unit test would check for regression on every test run.
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423229107)
A unit test would check for regression on every test run.