💬 brunoerg commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2431705621)
Concept ACK

🤔 stickies-v reviewed a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2388250835)
Concept ACK

👍 laanwj approved a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2388406290)
Code review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
Not the easiest refactor to review, carefully checked that there are no behavior differences, but i think the change absolutely worth it for the more readable API, and the removal of ambiguity.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1812617677)
Yeah will break existing grepping scripts, but I think we will need to do this anyway to improve the logging for package replacements.

I think we should log the new package or new transaction replacement only once after logging all the replaced transactions:

1. First, log all replaced transactions with their ID, witness, fee, and size.
2. Then:
- For a single transaction replacement, log its ID, witness, fee, and size.
- For a package replacement, log that a new package has been
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2431934694)
I will wait for some conceptual support before writing tests for `InputFetcher`.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1812631628)
Its not specified, I think we have to specify tracing api for packages;

Should be something like

#### Tracepoint `mempool:replaced_by_package`

Is called when a transaction in the node's mempool is getting replaced by a package.
Passes information about the replaced transaction and the replacement package.

Arguments passed:
1. Replaced transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
2. Replaced transaction virtual size as `int32`
3. Replaced
...

💬 kevkevinpal 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#discussion_r1812631980)
Sounds good to me! Updated in [ad38457](https://github.com/bitcoin/bitcoin/pull/31040/commits/ad384572cad35a248ab0322e9f8855bbed024f0b)

💬 kristapsk commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2431955857)
Concept ACK

💬 sipa commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#discussion_r1812643857)
Would it make sense to pass `chainman.m_best_header->nTime` to `GuessVerificationProgress()` (which then uses that instead of the current time)? That would make the result always consistently report progress w.r.t. the headers tip, instead of w.r.t. the current time.

💬 kevkevinpal commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432008025)
Concept ACK

There have been instances where there have been breakage when using newer minor versions of miniupnp aswell to the other mentioned reasons
https://github.com/bitcoin/bitcoin/issues/30266 -> https://github.com/bitcoin/bitcoin/pull/30283 -> https://github.com/bitcoin/bitcoin/pull/30301

💬 kevkevinpal commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432021055)
Might be worth adding a release note?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812514096)
can we name the variable such that we don't need this comment?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812439172)
what's the purpose of this block format?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812548418)
I know it's not trivial request, but can we add a test for this class which fetches everything in parallel and sequentially and assert that the result is equivalent?
And preferably also a benchmark, like we have it for https://github.com/bitcoin/bitcoin/blob/master/src/bench/checkqueue.cpp.
I would gladly help here, if needed.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812531761)
We're mostly creating the buckets randomly here, so each thread will need access to basically all of the keys.
Since we have an idea of how LevelDB works here (i.e. Sorted String Table), we could likely improve cache locality (would likely be most beneficial on HDDs) and minimize lock contention by splitting the reads by sorted transactions instead.

🤔 l0rinc requested changes to a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2388117544)
Concept ACK

We need to find better default values for SSD and HDD and I'd be interested in how coroutines would perform here instead.

Note that the cache warming is visible in the flame graphs as well:

`FetchCoin` delegating often to the db before the change (calling `GetCoin` often):
<img width="1342" alt="image" src="https://github.com/user-attachments/assets/04e90383-bb95-4314-a1a2-2aae0ca2b222">

First `FetchCoin` finds the values in the map:
<img width="1340" alt="image" src="
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812440117)
Isn't this a leftover a hack for non-owning LevelDB thread or is this really the best way to name threads in a cross-platform way?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812534028)
I'm wondering if we really need to (b)lock here or whether we could we create a [read-only snapshot](https://github.com/google/leveldb/blob/main/doc/index.md#snapshots) instead and avoid stalling?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812560853)
does this comment add any value here?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812560601)
for consistency (see: `explicit CCheckQueue(unsigned int batch_size, int worker_threads_num)`) and simplicity (`m_input_fetcher{/*batch_size=*/128, static_cast<size_t>(options.worker_threads_num)}`, and to follow modern C++ directions where sizes seem to be preferred as signed values, see: https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766881296), please consider making these int(s) instead.