💬 RandyMcMillan commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-3478151804)
👀

💬 achow101 commented on issue "Is not demanding low-R grinding still relevant for external signers?":
(https://github.com/bitcoin/bitcoin/issues/33761#issuecomment-3478152626)
What is the exact error that you are getting?

There is no "demanding", we cannot demand or request external signers do anything with low-R grinding. All we can do is make a guess about whether low-R grinding will be done, and account for that when selecting inputs so that the fee can be set appropriately. The current behavior is that if an input can be signed by the wallet itself, then we assume low-R grinding will occur. If it cannot be, i.e. the wallet does not have the private keys to sign t
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2484938860)
e.g.:
```c++
/**
* This handler is used for multiple HTTP endpoints:
* - `/block/` via `rest_block_extended()`
* - `/block/notxdetails/` via `rest_block_notxdetails()`
* - `/blockpart/` via `rest_block_part` (doesn't support JSON response so `tx_verbosity` is unset)
*/
static bool rest_block(const std::any& context,
HTTPRequest* req,
const std::string& uri_part,
std::optional<TxVerbosity> tx_verbosity,

...

💬 cobratbq commented on issue "Is not demanding low-R grinding still relevant for external signers?":
(https://github.com/bitcoin/bitcoin/issues/33761#issuecomment-3478159502)
> What is the exact error that you are getting?

Not error. Every time a transaction is signed and ready in the transaction list, when the transaction is of the largest flavor, it is completely greyed out and status tells me "_unconfirmed, not in memory pool_". I cannot find any other explanation why it is always the largest variant (maximum bytes) that is rejected by the memory pool.

My intuitive guess is that the transaction is signed with a predetermined fee that assumes that either `R` or `
...

💬 achow101 commented on issue "Is not demanding low-R grinding still relevant for external signers?":
(https://github.com/bitcoin/bitcoin/issues/33761#issuecomment-3478167704)
Can you try to broadcast the problematic transaction using `sendrawtransaction`? It should give you a more specific error.

> The fee is guessed beforehand, right?

The fee is set beforehand, the size is guessed.

> So is it possible that the memory pool rejects the largest transaction because the guess is for a fee for 1 byte less?

Yes. If you set the feerate to be minrelayfee, and the transaction is larger than was estimated, then the actual feerate will be lower than minrelayfee and the tran
...

💬 cobratbq commented on issue "Is not demanding low-R grinding still relevant for external signers?":
(https://github.com/bitcoin/bitcoin/issues/33761#issuecomment-3478193974)
> Can you try to broadcast the problematic transaction using `sendrawtransaction`? It should give you a more specific error.

Will try.

> [..]
>
> > So is it possible that the memory pool rejects the largest transaction because the guess is for a fee for 1 byte less?
>
> Yes. If you set the feerate to be minrelayfee, and the transaction is larger than was estimated, then the actual feerate will be lower than minrelayfee and the transaction won't be accepted.

Not sure if this applies in 'regt
...

💬 achow101 commented on issue "Is not demanding low-R grinding still relevant for external signers?":
(https://github.com/bitcoin/bitcoin/issues/33761#issuecomment-3478202041)
> Any ref of high-S / low-S?

https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#user-content-Low_S_values_in_signatures (the BIP itself is withdrawn, but this section on low-S is relevant in this dicsussion)

💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2484961831)
I believe this case should have been removed in https://github.com/bitcoin/bitcoin/pull/25325, which was merged just after this was introduced in https://github.com/bitcoin/bitcoin/pull/27011. So it likely got missed that `ReallocateCache` is always called.

💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2484965939)
```suggestion
* If will_reuse_cache is false, the cache will retain the same memory footprint after flushing and should be destroyed to deallocate.
```

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3478211267)
@romanz do you have a proof-of-concept for the tx indexer that will utilize this? It would be good to make sure this will be sufficient and performant enough for your requirements.

🤔 l0rinc requested changes to a pull request: "validation: fetch block inputs on parallel threads >20% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3389779624)
I like how we're progressing here! I think we need a few more things and have to try out a few alternatives (I haven't given up on sorting yet, especially now with bigger dbcache) and want to see how this combines with the other optimizations (threadpool, SipHash13, map hash caching, etc), but I'm definitely getting closer and closer to an ACK :D

I'm testing full IBD locally on my servers, but those are always slower than `reindex-chainstes` since the nodes can't send the blocks fast enough - I
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2470286368)
Nit: structs are now formatted consistently on the same line

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2470285353)
nit: we could throw now to indicate we don't have unrepeatable side-effects

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2484846327)
Nit: can you please reformat the change with latest clang-format?
Nit2: this seems to belong closer to `GetCacheSize`, one reserves, the other returns the actual size

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