💬 instagibbs commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1812876565)
two empty string might be simpler?

🤔 furszy reviewed a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2388943653)
Cool idea.

Slightly orthogonal topic:
Since the inputs fetcher call is blocking, instead of creating a new set of worker threads, what do you think about re-using the existing script validation ones (or any other unused worker threads) by implementing a general-purpose thread pool shared among the validation checks?
The script validation checks and the inputs fetching mechanism are never done concurrently because you need the inputs in order to verify the scripts. So, they could share work
...

💬 sipa commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432418782)
So overall, this is effectively changing from estimating progress based on time, to estimating progress based on comparing with the best headers chain. I think that's a reasonable thing to do, but I would suggest doing it more completely and cleanly, and add some safeguards.

* Guess an "end of chain timestamp" as the maximum of:
* The timestamp of the active chain tip
* The best headers tip's timestamp if it has more work than minchainwork; the current timestamp otherwise.
* Use that t
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2432481201)
> implementing a general-purpose thread pool shared among the validation checks?

Nice, yes that would be great! That would simplify this PR a lot if it could just schedule tasks on worker threads and receive the responses, instead of implementing all the sync code itself.

> https://github.com/bitcoin/bitcoin/pull/26966 introduces such structure inside https://github.com/bitcoin/bitcoin/commit/401f21bfd72f32a28147677af542887518a4dbff, which we could pull off and use for validation.

Conce
...

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1303702691)
We use `uint32_t` for `block_height` in the previous commit. Any reason we use `int` here?

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1812965559)
If we receive an interrupt, we don't also want to wait for the work queue to be empty right?

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1812986201)
Can we modify this to be more generic and return a type? And add logic to collect all returned values into a shared vector which can then be atomically swapped out by an observer? Possibly not in this PR, but if this will be split out into a generic thread pool.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1812987095)
working on a test and fix for this.

💬 maflcko commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2432543952)
Would be good to address https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274 as well. Otherwise there will be another follow-up?

⚠️ ismaelsadeeq opened an issue: "RPC: `getblockstats` might not return the *effective* percentile fee rate"
(https://github.com/bitcoin/bitcoin/issues/31140)


One of the responses from the `getblockstats` RPC is `feerate_percentiles`, which returns the following:
```zsh
"feerate_percentiles": [ (json array, optional) Feerates at the 10th, 25th, 50th, 75th, and 90th percentile weight unit (in satoshis per virtual byte)
n, (numeric) The 10th percentile feerate
n, (numeric) The 25th percentile feerate
n, (numeric) The 50th percentile feerate
n,
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1813017536)
> coroutines are just programming paradigm, not magic

That's also what I was counting on! :D

In RocksDB they have [high and low priority work](https://github.com/facebook/rocksdb/wiki/RocksDB-Tuning-Guide#parallelism-options) (I assume that's just added to the front or the back of a background work deque) – this would align well with @furszy's suggestion for mixing different kinds of background work units.

I haven't used the C++ variant of coroutines either, but my thinking was that sin
...

💬 maflcko commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1813038485)
Are you still working on this?

💬 sipa commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1813041419)
> I haven't used the C++ variant of coroutines either, but my thinking was that since they can theoretically yield execution when waiting for IO (and resume later), this would allow threads to focus on other tasks in the meantime.

That needs async I/O, and is unrelated to coroutines, as far as I understand it. Coroutines just help with keeping track of what to do when the reads come back inside rocksdb.

As long as LevelDB (or whatever database engine we use) internally does not use async I
...

💬 edilmedeiros commented on issue "TestFramework: TestShell.reset() will always fail":
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2432635781)
Looks similar, indeed, but I think #30714 did not covered `TestShell().setup().

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1813071345)
Oh sorry, I forgot to reply to this. We discussed this in person last week and this is indeed an issue. Happy to take your fix @naumekogs

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2432721650)
I'm not in love with the `DEBUG` separation, but I'm fine with it if others are.

ACK 49f73071a8ec4131595090a619d6198a5f8b7c3c

`git range-diff 4f33e9a85c3a8f546deddc2f78cf74bceff30315~4..4f33e9a85c3a8f546deddc2f78cf74bceff30315 aba9ce0eb6e67945f2209a17676f356fe9748
7e6..928538e3782ccbb22f54f4a3b1152b1faba95471`

<details>
<summary>Diff</summary>

```patch
1: 1f2d0ffd56 < -: ---------- move-only: ConstevalFormatString tfm::format to tinyformat.h
2: c937ebf1ce = 1: efa6f3f372 tiny
...

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813106158)
Thanks for pointing out how the logging looks in this PR -- my intent was to preserve the existing behavior in the single transaction replacement case, and not introduce a new line break. (I think the behavior of `LogDebug()` changed recently in #30929, which made what I wrote here no longer do what I expected.)

So we're all on the same page, here's what the logging looks like today (I grabbed this output from the functional tests `feature_rbf.py` and `mempool_package_rbf.py`), in the single
...

💬 Christewart commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432746552)
concept ACK

💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2432766510)
> Would be good to address [#30793 (comment)](https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) as well. Otherwise there will be another follow-up?

I agree that this is a worthwhile update. I'll take a deeper look and see if it might fit nicely into this PR (for less code churn). If not, then it could become its own PR.

> I really think `ORPHAN_MAX_RETENTION_TIME` should be called`ORPHAN_TX_EXPIRE_TIME`

Updated locally. Will push after looking into the above.

>
...

💬 tdb3 commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432803849)
> > Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
> > https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
> > Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orpha
...