#32575 consensus: Remove special treatment for single threaded script checking
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #32575 consensus: Remove special treatment for single threaded script checking | Ready | 0 (+1) -1 | Disputed | 96 + 17 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
Routes all block script checking in ConnectBlock through CCheckQueue regardless of worker thread count, removing the separate inline execution path for single-threaded verification. CheckInputScripts is refactored into helpers (EnsureTxData and GetScriptCacheEntry) and stripped of its pvChecks output parameter, leaving it solely for inline script checks. In addition, script execution cache insertion during TestBlockValidity (fJustCheck=true) is now performed explicitly after all checks pass, enabling cache population in multithreaded runs as well.
Problem: CheckInputScripts previously operated in two modes: running scripts inline if pvChecks was null, or appending CScriptCheck items to a vector if pvChecks was provided. This dual behavior made the function confusing to audit and led to an inconsistency where script execution cache insertion in TestBlockValidity was bypassed whenever worker threads were active because pvChecks was populated.
Evidence of importance:
- Commit 40552601f3 notes: 'Previously, script execution cache population during TestBlockValidity (fJustCheck=true) only worked in the single-threaded case, because CheckInputScripts guarded the cache insertion with !pvChecks. In the multithreaded case, pvChecks was always set, so the cache was never populated. This inconsistency has existed since the script execution cache was introduced.'
- Author description states: 'This dual use makes the function hard to grasp and its naming confusing. This PR refactors this code to be more readable and improves documentation aside from unifying the single threaded and the multithreaded cases by letting checkqueue handle single threaded script checking as well.'
Scope: ConnectBlock no longer invokes CheckInputScripts; it directly uses GetScriptCacheEntry and builds CScriptCheck objects for the check queue.
Risk of a mistake: medium (consensus, validation). Mistakes in script check queuing or validation cache storage during block connection could fail to enforce script checks or improperly cache failed validations, risking acceptance of invalid blocks.
Changed since the previous assessment: No structural changes to the approach; commits were refined to encapsulate script execution cache access in ValidationCache and extract helper functions.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: document script cache miss trade-off on next touch 2026-09-22
- decision: resolution of l0rinc's Approach NACK on mixing consensus behavior change with refactoring 2026-04-14
Agreement
Disputed
objection that needs a change, the objector pushed back after a reply (l0rinc)
Thread notes: Added ismaelsadeeq's Concept ACK review.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| theuni (member) | suggestion, approach | make CCheckQueueControl work with 0 worker threads and always use it to unify single-threaded and multithreaded script checking | fixed · fix pushed · implemented by author after benchmarking confirmed no penalty | answered | yes | 2025-05-22: 'An alternative would be to make CCheckQueueControl work with 0 worker threads (maybe it already does?) and just always use it. That would completely unify the approach here.' Status from: 2026-02-01: fjahr: 'Checkqueue can run without worker threads as well but I still expected that some changes would be needed. That doesn't s' |
| mzumsande (member) | suggestion, approach | populate pvChecks outside CheckInputScripts so CheckInputScripts can also call PrepareInputScriptChecks without duplicating checks | fixed · fix pushed · author pushed updated version adopting the suggestion | answered | yes | 2025-05-22: 'did you consider the alternative approach of doing the pvChecks population not here (but in ConnectBlock() or a separate function), so that CheckInputScripts could also call PrepareInputScriptChecks a' Status from: 2025-06-19: fjahr: 'I have implemented this at the cost of duplicating the cache entry calculation.' |
| mzumsande (member) | question, correctness | whether it is intended behavior that mining RPCs add entries to the script cache only in the -par=1 case | answered · theuni replied that it is very unlikely to be intended, and caching was later analyzed and restored | answered | yes | 2025-05-22: 'I wonder if the current behavior is intended - it seems very strange to me that mining rpcs would add entries to the script cache, but only in the -par=1 case.' Status from: 2025-06-17: theuni: 'I think it's very unlikely that that behavior is intended :)' |
| sedited (member) | suggestion, approach | eliminate the separate single-threaded path and create a checkqueue without workers | fixed · fix pushed · author implemented checkqueue without worker threads | answered | yes | 2025-11-30: 'Looking at this patch, I am asking myself why we're keeping the single threaded / no checkqueue case in the first place. This seems to introduce a vector of jobs for both cases now, so maybe we could ' Status from: 2026-02-01: fjahr: 'Checkqueue can run without worker threads as well but I still expected that some changes would be needed. That doesn't s' |
| sedited (member) | question, approach | why the code was extracted into a separate function | answered · author explained it improves readability and makes behavior changes easier to review | answered | yes | 2025-11-30: 'Why is this made a separate function?' Status from: 2026-02-27: fjahr: 'I just found it nicer in terms of documentation and readability to extract this into a function. It isn't strictly neces' |
| l0rinc (member) | objection, approach | combining consensus-critical behavior change with structural refactoring increases the risk of subtle consensus bugs | contested · l0rinc maintained the Approach NACK arguing the PR should be split and opened alternative PR #34875 | changed | yes | 2026-03-05: 'Approach NACK. The recent consensus bug in the BIP110 soft fork proposal in this exact area indicates we need to be extra careful, which isn't the case in this PR yet. I left a ton of suggestions as I' Status from: 2026-04-14: l0rinc: 'The updated results don't change my review - my objections are about mixing a behavior change with a refactor in consens' |
| l0rinc (member) | suggestion, maintenance | encapsulate cache access methods inside ValidationCache rather than extracting a local helper | fixed · fix pushed · author added methods to ValidationCache in a separate commit | answered | yes | 2026-03-05: 'I could get behind properly encapsulating the cache and exposing only the contains/insert methods' Status from: 2026-03-11: fjahr: 'I agree that it is nicer to this in ValidationCache, I have taken these changes as a separate commit' |
| l0rinc (member) | suggestion, interface | return the cache key instead of a boolean to avoid recomputing the cache entry hash | fixed · fix pushed · author changed the function to return the hash | answered | yes | 2026-03-05: 'It returns a bool, which forces us to recompute the cache key. What if we adjust it to return the cache key instead' Status from: 2026-03-11: fjahr: 'I have changed this function to return the hash so we don't have to recalculate it.' |
| l0rinc (member) | objection, approach | potential single-threaded validation performance regression from using checkqueue with no worker threads | accepted · benchmarks confirmed no performance regression | changed | yes | 2026-03-13: 'what makes you say this doesn't introduce any regression? I've been trying to measure the -assumevalid=0 -par=1 case for almost a week now. master finished in 60 hours; this branch is still running...' Status from: 2026-04-06: l0rinc: 'I managed to run -par=1 on a different server, it has very similar performance.' |
| darosior (member) | objection, correctness | dropping script execution caching in TestBlockValidity introduces validation delays at block submission time | fixed · fix pushed · fjahr restored script caching in TestBlockValidity | changed | yes | 2026-03-17: 'Besides, i think this is a regression to remove script execution caching from TBV. This introduces additional delay at block submission time, doing work that was already performed at block template ge' Status from: 2026-03-23: fjahr: 'I have now re-introduced caching in the same commit and fixed the commit message accordingly.' |
| l0rinc (member) | suggestion, correctness | add upfront test coverage demonstrating thread-mode behavior before refactoring | answered · author noted that existing functional and fuzz tests already cover -par=1 and declined to duplicate tests | answered | yes | 2026-04-06: 'Missing upfront test coverage to demonstrate no behavior change - see my alternative which adds thread-mode tests first before any refactoring, so regressions are caught immediately.' Status from: 2026-04-14: fjahr: 'The functional tests already have -par=1 coverage and the fuzz test also use 0 worker threads in deterministic mode so I' |
| Eunovo (member) | suggestion, maintenance | document the trade-off that script cache entries are missed if a block fails validation | open | answered | yes | 2026-09-22: 'Some txs that should be cached here will be missed if the block fails for any reason... I wonder if we should document the trade-off here.' |
Support:
- theuni (member) (Concept ACK, 2025-05-20): Anything to clean this up :)
- sedited (member) (Concept ACK, 2025-05-20): (no reason given) [not substantive]
- stickies-v (member) (Concept ACK, 2025-05-21): (no reason given) [not substantive]
- sedited (member) (Approach ACK, 2026-02-13): This seems like a nice cleanup, will look at it in detail later.
- darosior (member) (Concept ACK, 2026-03-17): (no reason given) [not substantive]
- Eunovo (member) (Concept ACK, 2026-05-22) [read: ConnectBlock, txvalidationcache_tests.cpp]: The changes simplify the flow for script verification in ConnectBlock, and they have a nice side effect of simplifying txvalidationcache_tests.cpp.
- Eunovo (member) (ACK, 2026-09-22): (no reason given) [not substantive]
- fanquake (member) (Concept ACK, 2026-10-16): (no reason given) [not substantive]
- ismaelsadeeq (member) (Concept ACK, 2026-10-30): (no reason given) [not substantive]
Participants:
- sedited (member, support, 7 statements 2025-05-20 to 2026-09-22): Concept ACK and Approach ACK; suggested running checkqueue without worker threads
- theuni (member, support, 5 statements 2025-05-20 to 2025-06-17): Concept ACK; suggested unifying single-threaded and multithreaded script checking via checkqueue or ConnectBlock loop
- stickies-v (member, support, 1 statements 2025-05-21 to 2025-05-21): Concept ACK
- mzumsande (member, question, 2 statements 2025-05-22 to 2025-05-22): asked about avoiding duplication in CheckInputScripts and questioned script cache behavior in TestBlockValidity
- maflcko (member, neutral, 1 statements 2025-06-20 to 2025-06-20): noted typos and grammatical errors in comments
- l0rinc (member, objection, 18 statements 2026-03-05 to 2026-04-14): Approach NACK over mixing consensus behavior changes with refactoring; raised performance concerns
- darosior (member, objection, 2 statements 2026-03-17 to 2026-03-17): Concept ACK, but flagged dropped script execution caching in TestBlockValidity as a regression
- svanstaa (contributor, neutral, 2 statements 2026-03-22 to 2026-04-05): ran reindex-chainstate benchmarks confirming no slowdown with par=1
- Eunovo (member, support, 5 statements 2026-05-22 to 2026-09-22): Concept ACK and full ACK; suggested code and comment improvements
- fanquake (member, support, 2 statements 2026-10-16 to 2026-10-16): Concept ACK
- ismaelsadeeq (member, support, 1 statements 2026-10-30 to 2026-10-30): Concept ACK
Files
38 lines under test/bench/ci.
- src/validation.cpp +88/-57
- src/test/txvalidationcache_tests.cpp +16/-20
- src/validation.h +8/-3
- src/checkqueue.h +0/-2
- src/test/fuzz/connect_block.cpp +1/-1
Card
The short text the ranking pass compares across PRs.
ConnectBlock is updated to route all block script validation through CCheckQueue, eliminating a dedicated single-threaded verification branch since CCheckQueue executes checks synchronously on the calling thread when no worker threads exist. CheckInputScripts is stripped of its pvChecks output parameter and now only executes scripts inline, while ConnectBlock constructs script checks directly. The refactor also resolves a long-standing omission where block script validation results during TestBlockValidity were never added to the cache in multithreaded mode. The author notes this simplifies validation code in preparation for batch validation work.
Data
thread read through 2026-10-30 (81 statements), head 10276767db
labels: Refactoring
Categories
Validation
P3 · cleanup
Unifies single-threaded and multithreaded script check execution paths in ConnectBlock through CCheckQueue while resolving a long-standing omission in TestBlockValidity cache population. The change simplifies critical validation code and removes dual-mode behavior in CheckInputScripts, but remains an internal refactor without blocking major features.
Membership: Alters ConnectBlock script check dispatching in src/validation.cpp and fixes script execution cache insertion in TestBlockValidity.
Factors: security/stability 1, bug 1, performance 1, user value 0, leverage 0