#36496 coins: avoid redundant prevout preparation allocations
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36496 coins: avoid redundant prevout preparation allocations | Waiting on author | 0 (+1) | Positive | 32 | |||
|
|
|
|
| |||
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
Persists and reuses heap storage across blocks in CoinsViewOverlay during parallel prevout prefetching. It keeps the intra-block transaction ID hash set and worker task vector alive across blocks, skips inserting the final transaction ID into the filter, and caps dispatched worker tasks to the queued input count while skipping condition variable notification on empty task lists.
Problem: CoinsViewOverlay::StartFetching reallocates and reseeds an unordered_set of transaction IDs and reallocates a task vector for every block connected. It also inserts the block's final transaction ID even though no subsequent transaction can spend it, and it schedules more worker tasks than available inputs when input queues are small.
Evidence of importance:
- No benchmark numbers or profiling traces are included in the PR description or commit messages to substantiate the performance impact of these allocation savings.
Risk of a mistake: medium (validation). A mistake in tracking earlier intra-block transaction IDs or prefetch task execution could lead to missing or incorrect prevout prefetching during block connection.
Changed since the previous assessment: Replaced the previous modification of ThreadPool::Submit parameter types with moving the persistent task vector directly, and added a check in ThreadPool::Submit to skip condition variable notification when the submitted futures list is empty.
Reviewability
Paused · Waiting on author
Open objection from hodlinator with no author reply for 10 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: response to hodlinator's suggestion to check m_inputs.size() before logging warning 2026-10-22
Agreement
Positive
support without stated reasons, no open objection (hodlinator, andrewtoth, andrewtoth)
Thread notes: hodlinator suggested reverting their earlier ThreadPool modification and adding an Assume check; author reverted the ThreadPool changes and hodlinator dropped the Assume request. hodlinator also raised an open suggestion to check m_inputs.size() to avoid an odd warning message on empty tasks.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| hodlinator (member) | suggestion, interface | Allow non-const l-values to be passed into ThreadPool::Submit() and remove std::move(m_tasks) | fixed · fix pushed · author implemented the suggestion, credited hodlinator as co-author, and handled empty tasks internally | answered | yes | 2026-10-13: 'It would probably be more honest/correct to allow non-const l-values to be passed into Submit() and remove the move() here.' Status from: 2026-10-15: l0rinc: 'Thanks, added you as coauthor and also handled the empty tasks internally' |
| hodlinator (member) | suggestion, approach | Revert the suggestion allowing non-const l-values in ThreadPool::Submit and instead add an Assume on m_tasks capacity | accepted · fix pushed · author reverted the ThreadPool change and explained why the Assume was unnecessary; hodlinator dropped the request | answered | yes | 2026-10-22: 'it may be safer to revert my suggestion and instead opt for adding `Assume(m_tasks.capacity() >= std::min(workers_count, m_inputs.size()))` with an explanatory comment' Status from: 2026-10-22: hodlinator: 'Not going to insist again though.' |
| hodlinator (member) | suggestion, interface | Bring back the m_inputs.size() check to avoid logging a weird warning when m_tasks is empty | open | answered | no | 2026-10-22: 'How about bringing the `m_inputs.size()` check back for that?' |
Support:
- hodlinator (member) (Concept ACK, 2026-10-13): (no reason given) [not substantive]
- andrewtoth (member) (ACK, 2026-10-15): (no reason given) [not substantive]
- andrewtoth (member) (ACK, 2026-10-19): (no reason given) [not substantive]
Participants:
- hodlinator (member, support, 6 statements 2026-10-13 to 2026-10-22): Concept ACK; suggested reverting ThreadPool changes and suggested avoiding weird warning when m_tasks is empty
- andrewtoth (member, support, 2 statements 2026-10-15 to 2026-10-19): ACKed earlier commit and reACKed latest commit
Files
0 lines under test/bench/ci.
- src/coins.cpp +21/-24
- src/coins.h +10/-2
- src/util/threadpool.h +1/-1
Card
The short text the ranking pass compares across PRs.
Reduces per-block allocation overhead in CoinsViewOverlay during parallel prevout prefetching. It reuses the intra-block transaction ID filter and worker task vector across blocks, omits the block's final transaction ID from the filter, and caps worker tasks to the queued input count. No performance benchmarks are provided to measure the impact of the avoided allocations.
Data
thread read through 2026-10-22 (10 statements), head 0977948b35
labels: UTXO Db and Indexes
Categories
Validation
P4 · speedup
Avoids redundant heap allocations and task dispatches in CoinsViewOverlay across blocks during prevout prefetching. Per the description, 'no benchmark numbers or profiling traces are included in the PR description or commit messages to substantiate the performance impact', making it an unmeasured micro-optimization.
Membership: Labeled 'UTXO Db and Indexes' and modifies coins prefetching and cache management in src/coins.cpp and src/coins.h.
Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 0