refactor: separate deferred script check collection from CheckInputScripts #34875

1. 
   l0rinc commented at 5:54 am on March 20, 2026: contributor

   Problem

   CheckInputScripts() currently serves two different purposes:

   • In the single-threaded path it executes script checks inline and may cache successful results.
   • In the multithreaded block validation path it instead prepares deferred CScriptChecks for the checkqueue.

   That makes the function name misleading and the control flow harder to follow in a consensus-sensitive area. The main issue is that these two uses are intertwined in one function.

   Fix

   This PR separates those responsibilities without changing behavior, making it a pure refactor with test coverage of the affected functionality.

   It first cleans up the shared pieces around script checking by encapsulating script execution cache access in ValidationCache, extracting spent output collection, and extracting the common pre-check logic into PrepareScriptChecks().

   It then splits deferred script-check collection out of CheckInputScripts() into a separate CollectScriptChecks() helper. ConnectBlock() uses CollectScriptChecks() only in the existing multithreaded path when worker threads are available. The single-threaded path still calls CheckInputScripts() directly, as before.

   With that split in place, CheckInputScripts() no longer needs the pvChecks parameter and can go back to only doing inline script validation. The tests are updated to cover both thread modes directly and to test the cache pre-check logic through PrepareScriptChecks().

   Notes

   This is a narrower alternative to #32575, keeping the original author and content in each commit where possible. The original PR eventually went further that a refactor and unified the single-threaded and multithreaded cases by always going through the checkqueue. I found that approach too risky - especially since it seems to lead to a catastrophic slowdown with single-threaded validation.

   This PR keeps the current thread-mode behavior intact and only separates the two uses into different functions so the code is easier to read, review, and reason about. If there is still interest in unifying the paths later, that can be discussed separately.

2. test: cover script checking thread modes

   The upcoming refactor needs direct coverage of the single-threaded `TestBlockValidity()` path and the multithreaded `ConnectBlock()` path.
   The existing coverage only exercised pieces of that behavior indirectly.
   
   Add explicit thread-mode coverage in the affected tests.
   The unit tests now check whether `TestBlockValidity()` populates the script execution cache with zero worker threads but not with worker threads.
   The functional test exercises `generateblock` with `-par=1` and `-par=2`.
   
   A small test-harness override for `worker_threads_num` makes the unit setups deterministic.

   26715c2b88
3. refactor: encapsulate script execution cache access in `ValidationCache`

   Move `m_script_execution_cache` to private and expose `IsScriptValidated()` and `CacheScriptValidation()` methods.
   This ensures the `AssertLockHeld` check is consistently applied for both cache lookups and insertions, rather than relying on callers to assert the lock.
   
   Co-authored-by: l0rinc <pap.lorinc@gmail.com>

   1c96a0d19c
4. refactor: extract `GetSpentOutputs` from `CheckInputScripts`

   `CheckInputScripts()` needs the spent outputs only when `PrecomputedTransactionData` is not already initialized.
   Extract that calculation into `GetSpentOutputs()` and leave the readiness check and size assertion at the call site, so the helper only does the one piece of work that is actually being shared.

   56067a12d2
5. refactor: extract PrepareScriptChecks from CheckInputScripts

   Separate the pre-check logic into `PrepareScriptChecks()`.
   It returns `std::optional<uint256>`: `nullopt` if no script checks are needed, or the cache entry hash for use by the caller when storing results later.
   
   `CheckInputScripts()` now calls `PrepareScriptChecks()` internally.
   This keeps the single-threaded execution path unchanged while making the shared setup reusable for deferred script check collection in a later step.
   
   Co-authored-by: l0rinc <pap.lorinc@gmail.com>

   9d1bd6fbd4
6. test: use `PrepareScriptChecks` in `txvalidationcache_tests`

   Update cache-hit and cache-miss assertions to call `PrepareScriptChecks()` directly instead of inferring the same result from the `pvChecks` vector.
   This keeps the tests focused on the cache pre-check contract while leaving the thread-mode coverage intact.
   
   Co-authored-by: l0rinc <pap.lorinc@gmail.com>

   4edcb0369c
7. refactor: split deferred script check collection

   `CheckInputScripts()` still handled both inline execution and deferred `CScriptCheck` collection.
   That mixed the single-threaded path we want to preserve with the multithreaded path that only needs to gather work for the checkqueue.
   
   Introduce `CollectScriptChecks()` for the deferred path and use it from `ConnectBlock()` only when worker threads are available.
   The single-threaded branch still calls `CheckInputScripts()` exactly as before, and the multithreaded branch still only queues collected checks, so this stays a pure refactor.

   7d102c1075
8. refactor: remove `pvChecks` parameter from `CheckInputScripts`

   Since `ConnectBlock()` now gathers deferred script checks through `CollectScriptChecks()`, no caller needs `CheckInputScripts()` to serve both inline execution and collection.
   
   Remove the `pvChecks` parameter and the associated dead branches.
   The remaining callers all want inline script execution, so this keeps the single-threaded path explicit without changing behavior.
   
   Co-authored-by: l0rinc <pap.lorinc@gmail.com>

   b2c8181f52
9. DrahtBot added the label Refactoring on Mar 20, 2026
10. 
    DrahtBot commented at 5:54 am on March 20, 2026: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32575 (refactor: Remove special treatment for single threaded script checking by fjahr)
    • #29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    LLM Linter (✨ experimental)

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • CheckInputScripts(tx, state, active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache) in src/test/txvalidationcache_tests.cpp
    • CheckInputScripts(CTransaction(spend_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache) in src/test/txvalidationcache_tests.cpp
    • CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache) in src/test/txvalidationcache_tests.cpp
    • CheckInputScripts(CTransaction(invalid_with_csv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache) in src/test/txvalidationcache_tests.cpp
    • CheckInputScripts(CTransaction(tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache) in src/test/txvalidationcache_tests.cpp

    2026-03-20 05:54:45

Contributors
l0rinc DrahtBot

Labels
Refactoring