#32575 consensus: Remove special treatment for single threaded script checking

full analysis

https://github.com/bitcoin/bitcoin/pull/32575 · fjahr · +107/-85 in 4 files, 6 commits · labels: Refactoring

Goal

  • Unify script verification dispatch so single-threaded and multithreaded execution share one path
  • Eliminate dual-purpose behavior to make block validation easier to reason about and maintain
  • Help unblock future consensus verification optimizations like batch verification

This PR refactors script verification in ConnectBlock to always pass script checks through CCheckQueueControl, removing the separate single-threaded execution path. It extracts script execution cache checking into GetScriptCacheEntry and ValidationCache methods, ensures TestBlockValidity populates the script cache consistently, and drops the pvChecks parameter from CheckInputScripts.

Problem: CheckInputScripts previously served two different functions: running script verification inline when single-threaded, or collecting checks into a vector when multithreading. This dual behavior made the function confusing to reason about and complicated future consensus verification optimizations like batch verification.

Category: Validation (#33 of 48)

P3 · cleanup

  • P3 because it is an internal cleanup of block validation logic that does not fix an active bug
  • Has neutral performance while removing hard-to-follow dual paths in consensus script checks
  • Provides modest leverage by unblocking future consensus optimizations like batch verification

P3 because it refactors and unifies single-threaded and multithreaded script check dispatching in ConnectBlock, removing dual-purpose parameters from CheckInputScripts. As fjahr stated, this dual use makes the function hard to grasp, and unifying it aids future batch verification work (#29491). Extensive benchmarks confirmed the change has neutral performance, and it does not fix an active bug or consensus vulnerability.

Membership: Directly modifies block connection and script checking dispatch in src/validation.cpp and src/checkqueue.h.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1

Reviewability: Stale: Author silent

  • Author has been silent for over three months though all pending review feedback was addressed
  • Review is worthwhile to help resolve the outstanding approach disagreement

The author has had no GitHub activity on the PR for 111 days, exceeding the project stale threshold, though the last push addressed all pending review comments.

Author status: silent since 2026-05-28

Open concerns:

  • l0rinc maintains an Approach NACK arguing that combining consensus behavior changes with structural refactoring is too risky and that the PR should be split, opening alternative PR #34875

Resolved concerns:

  • darosior pointed out that TestBlockValidity script execution caching was unintentionally dropped; fjahr restored caching for valid blocks in TestBlockValidity
  • l0rinc expressed concern about potential single-threaded performance regressions; benchmarks by svanstaa and l0rinc confirmed no measurable performance slowdown
  • Eunovo suggested renaming PrepareScriptChecks to GetScriptCacheEntry and clarifying caching comments; fjahr addressed both in the final push

Agreement: Disputed

  • Broad concept support for simplifying block script verification flow (theuni, sedited, darosior, Eunovo)
  • Confirmed no measurable performance regression across single-threaded benchmarks (svanstaa, l0rinc)
  • Approach objection that combining consensus changes with refactoring is too risky (l0rinc)
  • Author declined to split the changes into separate PRs

Disputed: l0rinc holds an Approach NACK against mixing consensus behavior changes with refactoring; author declined to split.

Several maintainers and contributors support the concept and approach (theuni, sedited, stickies-v, darosior, Eunovo). However, l0rinc maintains an unresolved Approach NACK, arguing that changes to single-threaded consensus execution and caching are too sensitive to combine with refactoring and should be split into separate PRs. The author engaged but declined to split.

  • theuni: Concept ACK. Anything to clean this up
  • sedited: Approach ACK. This seems like a nice cleanup
  • darosior: Concept ACK and corrected script execution cache behavior in TestBlockValidity
  • Eunovo: Concept ACK. The changes simplify the flow for script verification in ConnectBlock
  • l0rinc: Approach NACK. The rest of the commits still combine a consensus-sensitive behavior change with a structural refactor, so the risk seems significant
  • fjahr: I don't agree that splitting a PR is safer by default... Keeping separate paths around longer is not obviously safer

Review verdicts (DrahtBot): 0 -1

Dependencies

Enables:

Files

39 lines under test/bench/ci.

  • src/validation.cpp +84/-60
  • src/test/txvalidationcache_tests.cpp +17/-22
  • src/validation.h +6/-1
  • src/checkqueue.h +0/-2

Uncertainties

  • Whether the PR should strictly be marked Stale due to author silence when the author addressed all feedback in the latest push and is waiting for reviewers.

Card

PR 32575 unifies script verification in ConnectBlock so single-threaded execution routes through CCheckQueueControl rather than having a dedicated inline verification branch, allowing CheckInputScripts to drop its pvChecks output parameter. It also refactors script cache checks and encapsulates validation cache lookups. The change simplifies validation internals and aids ongoing batch verification work (#29491) with neutral benchmark performance. While several reviewers support the approach, l0rinc maintains an Approach NACK arguing that behavioral changes in consensus code should not be combined with structural refactoring.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T17:11, confidence high, input hash 8e9fb9af7bd37f9f