💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484854164)
we're also just adding stuff to the `m_inputs` without any reservation, the first few times we will have some needless copying - we could likely alleviate that by doing a rough reservation
```C++
m_txids.reserve(block.vtx.size());
m_inputs.reserve(2 * block.vtx.size()); // rough guess
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484858152)
I understand if you don't want to bother with this, but how many of these do we expect per block?
I wonder if we want to incur the hashing cost here or if using a sorted set or a sorted vector with `std::binary_search` (or even (unsorted?) SIMD-enabled linear scan) would be faster or simpler here.

I also thought of whether we could just add these to `temp_cache` directly, but that would likely pollute the up-coming validation (unless we can differentiate these from existing inputs).

----
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484851139)
I like that we're doing this, we've usually avoided preallocating our maps - we should do it more often!

-----

I know we're providing empty caches via the parameters, but for completeness either assert that they're empty or:

```suggestion
temp_cache.Reserve(temp_cache.GetCacheSize() + m_inputs.size() + outputs_count);
```

----

I have added
```C++
Assert(temp_cache.GetCacheSize() <= temp_cache.GetCacheSize() + m_inputs.size() + outputs_count);
```
after the `f
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484905167)
nit:
```suggestion
m_worker_threads.emplace_back([this, n] {
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484907043)
nit:
```suggestion
explicit Input(const COutPoint& o LIFETIMEBOUND) noexcept : outpoint{o} {}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484898679)
This seems like a code smell that we should pay attention to. I have already brought this up a year ago in https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722286606, seems it keep biting us.
Instead of adding a new method that basically does the same, can we keep `GetCoin` to simply return the coin and move the spentness checks to the call sites?
I understand that it would likely require another PR, but it would make this one cleaner - I don't like that we need a workaround for a sc
...

💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2485001950)
[Done](https://github.com/bitcoin/bitcoin/compare/e4e4f05f18b1791a4625175c22a154cbcb94c371..0ac969cddfdba52f7947e9b140ef36e2b19c2c41)

💬 cobratbq commented on issue "Is not demanding low-R grinding still relevant for external signers?":
(https://github.com/bitcoin/bitcoin/issues/33761#issuecomment-3478379562)
This seems to do the job. Right, so high-`s` was the problem. Given that it produces signatures of both 222 bytes and 223 bytes, high `R` must indeed (still) be accepted.

✅ cobratbq closed an issue: "Is not demanding low-R grinding still relevant for external signers?"
(https://github.com/bitcoin/bitcoin/issues/33761)

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485033847)
I did try and use a sorted vector and do binary search in the workers, but it was not a measurable performance difference. In theory it should be faster since txid comparison is much faster than siphash. I can do this if you want, but I think it makes the code clearer using the `unordered_set`.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485039220)
This is a consequence of skipping the main cache and writing directly to the temp cache.

I don't think we can modify `GetCoin` to not return spent coins, it's part of the definition [Retrieve the Coin (unspent transaction output) for a given outpoint.](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L310). This also goes backwards from your change https://github.com/bitcoin/bitcoin/pull/30849 where you added TODOs to not return spent coins where our test `GetCoin`s do. I don't thin
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485040051)
It was for false sharing. I don't think it has a measurable effect, but it might on some systems? I think it's harmless to keep, since there's only one InputFetcher.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485040642)
I don't think that's actually preferable. The current way is more readable IMO.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485042527)
There are no locks... This is a lock free implementation. It is synchronized with atomics per input, and the threads are work stealing via the `m_input_head`. Or do you mean something else?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485043525)
We can't have the main thread insert a failed fetch. The coin will not be updated. I suppose the main thread could check if the coin is unspent.
We want to exit ASAP as well if we can't find an input, so we need to signal the main thread that they should exit the loop.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485058898)
In `ConnectBlock` if we encounter a missing input we abort validation immediately. It is wasted work to continue. Why should we do it here?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485063519)
Right, didn't cover this new happy path in tests. Unlikely case is covered though. Will add. Thanks.

👍 andrewtoth approved a pull request: "[IBD] coins: reduce lookups in dbcache layer propagation"
(https://github.com/bitcoin/bitcoin/pull/33602#pullrequestreview-3409084819)
utACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41

I did not verify the speedup, but it is a nice optimization that will benefit every `Flush`.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485073051)
`<cstdint>` we need because we use `int32_t`.

💬 cobratbq commented on issue "External signer: in case of error shows only "External signer failure"":
(https://github.com/bitcoin/bitcoin/issues/31004#issuecomment-3478461277)
This has indeed improved in later version. I am currently running v29.2.0 which does show output.
Personally, I think the current implementation does not take into account the cases where there is excessive output, e.g. logging entries. Instead, the first line is assumed to be relevant information concerning the error. Arguably, in such a case, the last line would be better.

It is not a serious problem. I might start stderr output with a line that refers to stderr-output and/or logging.