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

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

💬 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

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(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.

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423229704)
Does this PR depend on #32313?

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423233247)
The two are related, but I made sure we can merge either of them - but the other one will likely need to be rebase after that

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2423281826)
I think that's what I also wrote in the commit message, but rephrased it slightly, let me know if it's better.

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3393840503)
Thanks for the comments, the latest push makes it even simpler, removed the temporary asserts, added unit tests instead (thanks @w0xlt), changed the order of the `AddCoin` and `EmplaceCoinInternalDANGER` commits which did indeed allow me to fixup the latest commit with the fuzz simplifications into the `AddCoin` one.

> In the last commit I'm still not sure why expected_code_path assert is being removed

The role of the variable was to verify that code execution follows only expected paths,
...