💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812439172)
what's the purpose of this block format?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812548418)
I know it's not trivial request, but can we add a test for this class which fetches everything in parallel and sequentially and assert that the result is equivalent?
And preferably also a benchmark, like we have it for https://github.com/bitcoin/bitcoin/blob/master/src/bench/checkqueue.cpp.
I would gladly help here, if needed.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812531761)
We're mostly creating the buckets randomly here, so each thread will need access to basically all of the keys.
Since we have an idea of how LevelDB works here (i.e. Sorted String Table), we could likely improve cache locality (would likely be most beneficial on HDDs) and minimize lock contention by splitting the reads by sorted transactions instead.

🤔 l0rinc requested changes to a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2388117544)
Concept ACK

We need to find better default values for SSD and HDD and I'd be interested in how coroutines would perform here instead.

Note that the cache warming is visible in the flame graphs as well:

`FetchCoin` delegating often to the db before the change (calling `GetCoin` often):
<img width="1342" alt="image" src="https://github.com/user-attachments/assets/04e90383-bb95-4314-a1a2-2aae0ca2b222">

First `FetchCoin` finds the values in the map:
<img width="1340" alt="image" src="
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812440117)
Isn't this a leftover a hack for non-owning LevelDB thread or is this really the best way to name threads in a cross-platform way?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812534028)
I'm wondering if we really need to (b)lock here or whether we could we create a [read-only snapshot](https://github.com/google/leveldb/blob/main/doc/index.md#snapshots) instead and avoid stalling?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812560853)
does this comment add any value here?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812560601)
for consistency (see: `explicit CCheckQueue(unsigned int batch_size, int worker_threads_num)`) and simplicity (`m_input_fetcher{/*batch_size=*/128, static_cast<size_t>(options.worker_threads_num)}`, and to follow modern C++ directions where sizes seem to be preferred as signed values, see: https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766881296), please consider making these int(s) instead.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812443509)
nit: is there any reason you prefer adding `{}` at the end of the vector inits?

💬 laanwj commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432084577)
i think doing this is fine, it's reported often enough--however, wouldn't it make sense to implement the functionality in `GuessVerificationProgress` itself so that other places (like the GUI and logging) consistently show the same value?

🤔 murchandamus reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2388662457)
ACK f863f5a4f991ccfdf865d39c73269b196eeb4902

💬 murchandamus commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1812744440)
I’m a bit surprised, why would we have been signaling replaceability for TRUC transactions in the first place? The only nodes that accept V3 transactions would be upgraded to understand TRUC at the same time, so shouldn’t we create TRUC transactions without signaling replaceability?

💬 murchandamus commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1812747019)
It seems to me that we would still want to test that TRUC transactions that don’t signal replaceability can be replaced. Obviously the assert_equal here that checks that the node does not use fullrbf would fail, but the rest of the test seems like it could be kept.

💬 jonatack commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432156438)
@laanwj I looked at doing it in `GuessVerificationProgress` but worried that reviewers might prefer not to touch it or have the logging output change, and so initially proposed the smaller-scoped, user-facing-only approach. It looked like the GUI only would call it from the console via the RPCs updated here (but may have overlooked something). Will try the approach @sipa suggested in https://github.com/bitcoin/bitcoin/pull/31135#discussion_r1812643857.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812779405)
This is blocking so we can access the queue of shared outpoints that we need to fetch from. It is not blocking for LevelDB, we access the db once we are out of the critical section.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812781136)
Yes, I can add these but I am waiting for some more conceptual support.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812785573)
It won't process twice, but it could pass in an empty vector, which is ignored if you look at `Add` implementation.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812794214)
I think it would be similar in complexity, we would still need all the locking mechanisms to prevent multithreaded access.

What would really be great is if we had a similar construction to Rust's `std::sync::mpsc`.

💬 fjahr commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432238813)
Concept ACK but shouldn't this go through a deprecation cycle?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812797796)
I'm not sure it would warrant the complexity I think this batch size is "good enough" for now. In a follow up we could maybe add ways to set this with configs to experiment if there really is more optimal settings.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812800076)
Adding more threads will require more memory, which is one reason to not use many more.

I did a benchmark using 64 threads on the same 16 vcore machine, and it was slightly slower :/