💬 maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#issuecomment-3586696431)
lgtm ACK d09e9306cab86c0917cc0ae198ef1993d764d106

💬 billymcbip commented on pull request "Add a separate ScriptError for empty pubkeys encountered in Tapscript":
(https://github.com/bitcoin/bitcoin/pull/33961#issuecomment-3586700291)
Also updated`feature_taproot.py` and ran `build/test/functional/feature_taproot.py`.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569534729)
The `(void)` here does not rely on the return value. Or do you mean something else?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569536332)
Ah, yes the `Reset` now just short circuits it. So this is not a very good benchmark anymore. Will fix to access all coins.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569545118)
Yes, this is a queue! An MPSC queue to be exact. I don't think it's possible to not store this as an instance member. I don't think we can use a `std::deque` though because we can't actually mutate the container. The benefit of a deque would be to actually push and pop, but here we set up the container before kicking off the worker threads.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569548184)
> did we benchmark 32-bit vs 64-bit prefixes

I did do microbenchmarks with 32-bit, and it was not more performant. Also, there is no `uint256::GetUint32`, so we would either just cast the 64 bits to 32 or have to write a new method on `uint256`. I don't think it's worth exploring this more.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569551740)
We don't actually throw away the ephemeral view anymore. We just reset the state each time. This is a big performance improvement since we don't have to reallocate anything. If we didn't do this we would need an external thread pool as well, since the threads are owned by the `CoinsViewCacheAsync`. I think it's a fair tradeoff for one extra line here.
Perhaps calling it `ephemeral_view` is a misnomer though. I'm not sure what a better name would be right now.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569553505)
There are some already in the codebase. They are added in C++20, why not use them? Maybe in future compiler versions the hints will become more useful for optimization?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569561092)
I use this instead of an `m_is_fetching` or equivalent boolean state. We don't want to stop the barrier if we haven't started it. We don't start above if the block has less than 2 txs, and we don't start if it is an invalid block with 2 or more txs that have zero inputs. After we stop the threads, we clear the inputs as well so we don't stop again.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569564192)
> Would it be possible to first refactor PushNodeVersion in a preparatory commit with no behavioral changes, and then add the private-broadcast specialization in a follow-up commit?

Done. Without a lambda.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569567709)
we wouldn't need it if `Flush` were a void itself, like `Reset`, right?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569574051)
> We don't actually throw away the ephemeral view anymore

Hah, I missed that in the latest push, thanks. 👍

> calling it ephemeral_view is a misnomer though

Yeah :)

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569578980)
Yes, but if the other PR is merged, then this will still compile with no errors or warnings right?

💬 billymcbip commented on issue "Add unit tests for Taproot code in src/script/interpreter.cpp":
(https://github.com/bitcoin/bitcoin/issues/23279#issuecomment-3586837302)
> SigVersion::TAPSCRIPT code paths in ExecuteWitnessScript

I've added a unit test for the empty pubkey error in Tapscript signature validation: https://github.com/bitcoin/bitcoin/pull/33961.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569586837)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569596852)
Done

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569597731)
> why not use them

https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709821764
https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1923995451
https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559477612

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569600697)
Done

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569603708)
you will have a conflict where you're making Flush abstract and they're making it void

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569611664)
Added new lines.

> and sort the groups for predctability

hmm?