💬 billymcbip commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2574659138)
Noob C++ question: When is it better to return a `nullptr` vs. throwing `std::out_of_range("index out of bounds")`?. Wouldn't it be cleaner to throw?

💬 billymcbip commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2574667690)
Noob question: How come you're not throwing directly in this function (`throw std::out_of_range("output_index is out of bounds"`)?

👍 l0rinc approved a pull request: "validation: fetch block inputs on parallel threads >40% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3521928328)
The new version cleverly uses `m_inputs` being empty as the shared shutdown signal (handling both 'no work' and 'shutdown' cases). This finally allowed us to eliminate the `m_request_stop` flag I disliked :).

It also benchmarks real-world I/O latency via a real LevelDB acces, while the fuzz tests use in-memory LevelDB now - sweet!
The new design basically falls back to synchronous fetching gracefully in cases of collisions or delays (we may want to test that specifically).

Regarding 'faster IB
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574407137)
`deterministic` sounds like something we should do in a benchmark, but it just predefines the salts for testability:
https://github.com/bitcoin/bitcoin/blob/9c24cda72edb2085edfa75296d6b42fab34433d9/src/util/hasher.cpp#L22-L25

As far as I can tell we don't need here, so we can likely remove that constructor arg completely:
```C++
explicit CoinsViewCacheAsync(CCoinsViewCache& cache, const CCoinsView& db) noexcept
: CCoinsViewCache{&cache}, m_db{db}, m_barrier{WORKER_THREADS + 1}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574387786)
I understand that `size() > 0` may be more descriptive than `!empty()`, but there's a dedicated method for this case (there are a few more cases):
```suggestion
if (m_inputs.empty()) return;
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574435070)
Should we maybe document that this assumes that `ConnectBlock` will fetch the inputs in the same order?

nit: could we use the current formatting for new code?

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

Should we mention any parallelism here?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574437547)
As https://corecheck.dev/bitcoin/bitcoin/pulls/31132 hints, this could also be passed as const reference.

nit: formatting

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574385347)
nit:
```suggestion
~CoinsViewCacheAsync() override
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574685431)
I'm not sure I understand how we're actually testing this.
`NoAccessCoinsView` is designed to abort on access, so in the shortid collision scenario how does it simulate going to disk?
Shouldn't we assert here that the number of collisions coincides with the number of simulated disk reads?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574457335)
[we should](https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=31132-69310ec0035351e486cfb51cb66492639f6f8b47&id=aureleoules_bitcoin&open=AZrV9y4tqc704xdUcnxD&tab=why) likely init it here instead:
```suggestion
std::barrier<> m_barrier{WORKER_THREADS + 1};
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574535324)
```suggestion
BOOST_CHECK_EQUAL(coin.IsSpent(), spent);
```

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574934048)
I think some invalid block tests exercise this. If a block is invalid then the outputs of any txs are not added to the utxo set.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574936137)
If there is a missing new line github will show a red arrow at the end of the file.

💬 mdabdulkaderhossain144-ux commented on something "":
(https://github.com/bitcoin/bitcoin/commit/2e27bd9c3af91eb9fcc626fe65d065df0a80974d#commitcomment-171659028)
0xfdDB0e3EDbc6ffC1cF63f0c47d0Db9c9EA7EC671

💬 mdabdulkaderhossain144-ux commented on something "":
(https://github.com/bitcoin/bitcoin/commit/2e27bd9c3af91eb9fcc626fe65d065df0a80974d#commitcomment-171659046)
2e27bd9c3af91eb9fcc626fe65d065df0a80974d

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574956644)
> isn't this too small for our purposes?

I don't see why we need any randomness or larger values for these?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574968743)
Hmm not sure how we'd test this with a unit test. It surely gets exercised by fuzzing though.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574970212)
It gets called by functional tests though.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574973805)
> before/after

Not sure there are any before results we can use for this.

> can you reproduce 4 threads saturating the parallelism factor with it?

How would I know if I do that?