💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568644363)

Same idea as in the bench: would it be feasible to fuzz this against an actual LevelDB-backed view instead of a synthetic cache, or is that too expensive / complicated for now?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568598322)
We’re [moving towards Flush not returning a value](https://github.com/bitcoin/bitcoin/pull/33866/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR257) – can we avoid adding new code that relies on its return? Not sure it’s possible before that other PR lands, but worth keeping in mind.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568610778)
nit: now that this is `CoinsViewCacheAsync`, maybe rename the thread prefix from "inputfetcher.%i" to something like "coinsviewcacheasync.%i" or similar, to reflect the current design.

📝 billymcbip opened a pull request: "Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961)
We currently have two callsites for `SCRIPT_ERR_PUBKEYTYPE`:
- Policy error behind the `SCRIPT_VERIFY_STRICTENC` flag: https://github.com/bitcoin/bitcoin/blob/4de26b111f4d9b116d0d4930f8bbd69889b3cf75/src/script/interpreter.cpp#L220
- Consensus error: https://github.com/bitcoin/bitcoin/blob/4de26b111f4d9b116d0d4930f8bbd69889b3cf75/src/script/interpreter.cpp#L368

It would be good for readability and testability to have separate errors for both cases, as they are distinct. We have more granula
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569278996)
Changed to use `count_seconds()`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569321223)
Yes, changed to use `pnode.LogIP(fLogIPs)`.

📝 ANtutov opened a pull request: "refactor: replace manual promise with SyncWithValidationInterfaceQueue"
(https://github.com/bitcoin/bitcoin/pull/33962)
BroadcastTransaction() now waits for validation callbacks using the built-in validation_signals>SyncWithValidationInterfaceQueue() instead of creating a local std::promise and scheduling a lambda. This removes an unnecessary allocation and uses the canonical API.

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