💬 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 :/

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812807883)
I don't think there is any lock contention here if we are doing multithreaded reading?

I also think what you're suggesting would add a lot more complexity to this PR, when this is "good enough".

💬 maflcko commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#discussion_r1812811243)
Not sure about this. Hard-coding the value to `1` when `height == headers` means that the value may inconsistently jump from 1 to 0.xxxx (in a loop), when headers pre-sync is disabled and the node is fed blocks one-by-one (header before block).

Also, if the node is eclipsed (intentionally, or just accidentally due to a network config error), this may also return `1`, when the node is far behind the real network.

🤔 maflcko reviewed a pull request: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31135#pullrequestreview-2388797020)
Not sure about the current fix. It seems to "fix" one style issue in a value that is meant as an estimate, but it may be adding two new issues at the same time?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812815888)
Unsure, copied from `CScriptCheck`. If the state of the art of thread naming has advanced since that was written, please let me know!

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1812825825)
Seems erroneous, indeed.

💬 sipa commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812826580)
The C++ standard library does as far as I know have no way of renaming threads at all. `src/util/threadnames.{h,cpp}` is our wrapper around the various platform-dependent ways of doing so on supported systems.

💬 maflcko commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432289339)
CI failure is unrelated, see https://github.com/bitcoin/bitcoin/issues/30969

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1812829296)
~0 on keeping it, we're reducing loc and don't really need the coverage anymore imo

Willing to be over-ridden by other reviewers.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812838378)
Can you tell me why we need to prevent multithreaded access exactly? We could collect the values to different vectors, each one accessed only by a single thread and merge them into the cache at the end on a single thread, right?

How would `mpsc` solve this better? Do you think we need work stealing to make it perfectly parallel? Wouldn't coroutines already achieve the same?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812842053)
4x may be too much to begin with, but 1.5-2x sounds plausible, I'll help with benchmarking this once my current batches finish.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812845859)
This might be as simple as sorting by tx before we create the buckets.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812849403)
Thank you, please resolve the comment.

📝 kevkevinpal opened a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139)
This PR adds coverage for this line https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L996

If you run the following you will get no results for `submitpackage`
`grep -nri "TX decode failed" ./test/functional`

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812852143)
As mentioned before, why do we need shared outpoints here?