#35751 validation: use parallel input prevout fetching in TestBlockValidity
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35751 validation: use parallel input prevout fetching in TestBlockValidity | Ready | 0 (+1) | Blocked | 14 + 25 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This PR updates TestBlockValidity to use the reusable CoinsViewOverlay via ConnectBlockView rather than allocating a temporary CCoinsViewCache. This enables parallel input prevout fetching during block template validation and prevents cache misses from populating the main coins cache. It also updates the AssembleBlock benchmark to fill blocks to maximum weight to accurately evaluate the performance impact.
Problem: TestBlockValidity previously constructed an ephemeral CCoinsViewCache on top of CoinsTip, missing out on the parallel input fetching implemented in #35295 and polluting the main cache with lookups from dummy checks. Miners and Stratum v2 services creating or checking block templates experience slower validation latency as a result.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: address reentrancy safety concerns and prefetching memory allocation risk, and gather real-world benchmark data 2026-10-02
Agreement
Blocked
objection that needs a change, nobody has replied (Yudis-bit)
Thread notes: Added Sjors's benchmark suggestion and supporting review r:5649676670. Sjors's inline comment rc:4375580822 regarding splitting ConnectBlockView into a pure refactor commit was omitted as it pertains solely to commit organization.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| l0rinc (member) | suggestion, usefulness | provide a reproducible way to test the performance claim using deterministic proposals or a microbenchmark | answered · author updated the AssembleBlock benchmark and provided before/after measurements | answered | yes | 2026-07-22: 'Could you provide a reproducible way to test the performance claim, ideally using a deterministic set of mainnet block proposals or a microbenchmark?' Status from: 2026-07-29: andrewtoth: 'I've included an additional commit at the start that improves the AssembleBlock benchmark by filling the block to max we' |
| sedited (member) | objection, approach | slowdown in AssembleBlock benchmark when using overlay view | fixed · fix pushed · author pushed a commit updating the benchmark to fill blocks to max weight, resolving the measured regression | changed | yes | 2026-07-23: 'I see it in the report too, and can measure the slowdown locally as well.' Status from: 2026-07-29: andrewtoth: 'I've included an additional commit at the start that improves the AssembleBlock benchmark by filling the block to max we' |
| sedited (member) | suggestion, approach | introduce a heuristic to skip background fetching if the block does not spend many coins | answered · author declined, explaining blocks spending few coins are already fast | answered | yes | 2026-07-23: 'Maybe it would be worthwhile to introduce a heuristic where we skip background fetching if the block does not spend a lot of coins?' Status from: 2026-07-29: andrewtoth: 'what we really care about is that slow blocks are faster. A block spending few coins is already very fast, and so speedi' |
| optout21 (contributor) | suggestion, maintenance | use the newly introduced ConnectBlockView() helper for m_connect_block_view at line 3055 | fixed · fix pushed · author updated the callsite to use ConnectBlockView() | answered | yes | 2026-08-07: 'Could this also use the newly introduced ConnectBlockView()?' Status from: 2026-08-08: andrewtoth: 'Done.' |
| l0rinc (member) | objection, safety | reentrant calls to TestBlockValidity reusing ConnectBlockView while a synchronous callback is active during a fetch | answered · author proposed an assert to guard against reentrancy; reviewer agreed in rc:3866882847 but the fix has not yet been pushed | changed | yes | 2026-08-16: 'I think the reentrancy issue needs to be addressed before this can be merged.' Status from: 2026-08-23: andrewtoth: 'We can prevent this by calling Assert(overlay_view.GetCacheSize() == 0); inside TestBlockValidity.' |
| l0rinc (member) | suggestion, correctness | assert the max weight claim in the benchmark and adjust FAN_OUT to make it true | open | answered | no | 2026-08-16: 'can we assert the claim (and adjust the FAN_OUT to make it true? Just make sure the number is not 110, we're still sensitive to that...)' |
| sedited (member) | suggestion, usefulness | measure real block creation timings side-by-side with master on a similar mempool | answered · author agreed to gather real-world data and marked the PR as draft | answered | yes | 2026-10-02: 'I think it would be interesting to add some more real block creation timings, i.e. run this side-by-side with master on hopefully a similar mempool and see if there is a difference.' Status from: 2026-10-02: andrewtoth: 'Yes, good idea. I will try and gather some real world data with this. Marking as draft until I figure out the above.' |
| l0rinc (member) | suggestion, maintenance | avoid repeated mempool checks during fixture setup in the AssembleBlock benchmark by passing -checkmempool=0 | open | answered | no | 2026-10-07: 'Nit: the fixture checks the entire growing mempool after each insertion, we could probably disable those checks.' |
| l0rinc (member) | objection, safety | StartFetching can retain up to ~250 MiB of coins on invalid block templates before validation failure, creating a potential memory allocation risk without PoW protection | contested · reviewer pushed back that untrusted submitters do not need PoW or valid signatures to trigger prefetching and coin allocation | answered | yes | 2026-10-07: 'Before #35295, coins were fetched as validation reached each transaction, so any failure avoided fetching later transactions, but `StartFetching()` can now retain those coins before validation rejects' Status from: 2026-10-08: l0rinc: 'These fetches happen before input validation, so the submitter doesn’t need PoW or valid signatures to trigger them.' |
| Yudis-bit (none) | objection, safety | Assert(overlay_view.GetCacheSize() == 0) will not prevent reentrancy while prefetching is in flight, leading to an Assert(m_futures.empty()) abort inside StartFetching | open | changed | no | 2026-10-11: 'overlay_view.GetCacheSize() == 0 won't catch reentrancy while prefetching is in flight because worker threads write coins into m_inputs, not cacheCoins.' |
| Sjors (member) | suggestion, usefulness | add relevant benchmarks to the PR description | open | answered | no | 2026-10-26: 'It would be good to add some relevant benchmarks in the PR description.' |
Support:
- Sjors (member) (Concept ACK, 2026-07-23) [read]: making the checkBlock() IPC faster, intended to be called frequently by the Job Declarator Server role in Stratum v2
- optout21 (contributor) (2026-08-07) [read]: Another performance improvement, a must-have follow-up of #35295/#34164/#31132, looks good.
- Sjors (member) (2026-10-26) [both: checkBlock(), TestBlockValidity]: noted avoiding inserting fetched entries into the main cache protects against DoS/cache stuffing from untrusted miner templates, and measured that disk prefetching while awaiting missing transactions can save up to a full second during block checking
Participants:
- l0rinc (member, objection, 13 statements 2026-07-22 to 2026-10-08): requested changes over potential reentrancy and pushed back on untrusted input / memory allocation risks in TestBlockValidity
- sedited (member, objection, 4 statements 2026-07-22 to 2026-10-02): withdrew initial ACK after measuring an AssembleBlock regression; suggested testing real block creation timings
- Sjors (member, support, 3 statements 2026-07-23 to 2026-10-26): Concept ACK for speeding up checkBlock IPC for Stratum v2; tested prefetching and noted benefits against cache stuffing from untrusted miner data
- optout21 (contributor, support, 2 statements 2026-08-07 to 2026-08-07): supported performance improvement follow-up; suggested reusing ConnectBlockView
- Yudis-bit (none, objection, 1 statements 2026-10-11 to 2026-10-11): objected that GetCacheSize() == 0 will not prevent reentrancy aborts while prefetching is in flight and suggested checking !m_futures.empty() or falling back to a local cache
Dependencies
Enables:
- Stratum v2 template validation efficiency (#35671)
Files
27 lines under test/bench/ci.
- src/bench/block_assemble.cpp +25/-2
- src/validation.cpp +6/-5
- src/validation.h +8/-0
Uncertainties
- Whether the agreement state should be considered Mild rather than Disputed given that both reviewer and author agreed on the resolution.
Card
The short text the ranking pass compares across PRs.
This PR updates TestBlockValidity to reuse the CoinsViewOverlay introduced in PR 35295, enabling parallel prevout fetching and preventing cache-miss pollution in the main UTXO cache during template checks. It speeds up block template validation in AssembleBlock benchmarks by approximately 10 percent, which primarily benefits mining pool operators and Stratum v2 template checking workflows. The problem is a minor performance optimization rather than a critical consensus bottleneck. Review is currently paused while waiting on the author to push an agreed-upon reentrancy assertion requested by l0rinc. There are no blocking dependencies.
Data
thread read through 2026-10-26 (30 statements), head 2ba5628cb9
labels: Validation · draft
Categories
Validation
P2 · speedup (ranking pass: Moved down: ~10% gain confined to mining template validation, narrower than the other P2 performance work)
Parallel input prevout fetching significantly reduces block template validation latency while preventing cache pollution of CoinsTip. As noted in review discussion, prefetching can save up to a full second during block checking, delivering substantial performance benefits for mining template checks and Stratum v2 workflows.
Membership: Labeled Validation, modifying TestBlockValidity and view overlay management in src/validation.cpp and src/validation.h.
Factors: security/stability 1, bug 0, performance 2, user value 1, leverage 2