We’re mixing refactoring (separation of the two intertwined use cases) with a behavior change in a consensus area. I have a really hard time trusting this completely.
It would help if we could separate “validation” from “refactor” here. We could split the collection use case from validation by duplicating CheckInputScripts into a CollectScriptChecks, which creates the vector, fills it, and returns the values, while making input script validation ignore the vector. The two algorithms would already be very different (probably already achieving the main goal of this change without introducing any potentially dangerous unification, which we can still do in a follow-up if needed). This is what that would look like:
0diff --git a/src/validation.cpp b/src/validation.cpp
1--- a/src/validation.cpp (revision 87fcd645598d186bd1e4f68d5aca275dc7815fda)
2+++ b/src/validation.cpp (date 1772729378036)
3@@ -137,11 +137,17 @@
4 return m_chain.Genesis();
5 }
6
7-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
8- const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
9- bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
10- ValidationCache& validation_cache,
11- std::vector<CScriptCheck>* pvChecks = nullptr)
12+void CollectScriptChecks(const CTransaction& tx,
13+ const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
14+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
15+ ValidationCache& validation_cache,
16+ std::vector<CScriptCheck>& pvChecks)
17+ EXCLUSIVE_LOCKS_REQUIRED(cs_main);
18+
19+bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
20+ const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
21+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
22+ ValidationCache& validation_cache)
23 EXCLUSIVE_LOCKS_REQUIRED(cs_main);
24
25 bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction& tx)
26@@ -2060,8 +2066,8 @@
27 return spent_outputs;
28 }
29
30-/** Perform pre-checks for CheckInputScripts: skip coinbase, check the script
31- * execution cache, reserve space for script checks, and initialize txdata.
32+/** Perform pre-checks for script validation: skip coinbase, check the script
33+ * execution cache, and initialize txdata.
34 * Returns nullopt if the transaction is already validated (caller should
35 * return true), or the cache entry hash needed to store results later. */
36 static std::optional<uint256> LookupScriptValidation(
37@@ -2090,16 +2096,28 @@
38 return hashCacheEntry;
39 }
40
41+/** Collect script checks for all inputs without executing them.
42+ * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp */
43+void CollectScriptChecks(const CTransaction& tx,
44+ const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
45+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
46+ ValidationCache& validation_cache,
47+ std::vector<CScriptCheck>& checks) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
48+{
49+ if (LookupScriptValidation(tx, inputs, flags, cacheFullScriptStore, txdata, validation_cache)) {
50+ checks.reserve(tx.vin.size());
51+ for (unsigned int i = 0; i < tx.vin.size(); i++) {
52+ checks.emplace_back(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata);
53+ }
54+ }
55+}
56+
57 /**
58 * Check whether all of this transaction's input scripts succeed.
59 *
60 * This involves ECDSA signature checks so can be computationally intensive. This function should
61 * only be called after the cheap sanity checks in CheckTxInputs passed.
62 *
63- * If pvChecks is not nullptr, script checks are pushed onto it instead of being performed inline. Any
64- * script checks which are not necessary (eg due to script execution cache hits) are, obviously,
65- * not pushed onto pvChecks/run.
66- *
67 * Setting cacheSigStore/cacheFullScriptStore to false will remove elements from the corresponding cache
68 * which are matched. This is useful for checking blocks where we will likely never need the cache
69 * entry again.
70@@ -2112,8 +2130,7 @@
71 bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
72 const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
73 bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
74- ValidationCache& validation_cache,
75- std::vector<CScriptCheck>* pvChecks)
76+ ValidationCache& validation_cache)
77 {
78 const auto hash_cache_entry{LookupScriptValidation(tx, inputs, flags, cacheFullScriptStore, txdata, validation_cache)};
79 if (!hash_cache_entry) return true;
80@@ -2128,10 +2145,7 @@
81
82 // Verify signature
83 CScriptCheck check(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata);
84- if (pvChecks) {
85- pvChecks->reserve(tx.vin.size());
86- pvChecks->emplace_back(std::move(check));
87- } else if (auto result = check(); result.has_value()) {
88+ if (auto result = check()) {
89 // Tx failures never trigger disconnections/bans.
90 // This is so that network splits aren't triggered
91 // either due to non-consensus relay policies (such as
92@@ -2146,7 +2160,7 @@
93 }
94 }
95
96- if (cacheFullScriptStore && !pvChecks) {
97+ if (cacheFullScriptStore) {
98 // We executed all of the provided scripts, and were told to
99 // cache the result. Do so now.
100 validation_cache.CacheScriptValidation(*hash_cache_entry);
101@@ -2596,18 +2610,20 @@
102 if (!tx.IsCoinBase() && fScriptChecks)
103 {
104 bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
105- TxValidationState tx_state;
106 if (control) {
107 // CheckInputScripts always returns true when pvChecks is provided
108 // (checks are collected for deferred execution, not executed inline).
109 std::vector<CScriptCheck> vChecks;
110- Assume(CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks));
111+ CollectScriptChecks(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, vChecks);
112 control->Add(std::move(vChecks));
113- } else if (!CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache)) {
114- // Any transaction validation failure in ConnectBlock is a block consensus failure
115- state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
116- tx_state.GetRejectReason(), tx_state.GetDebugMessage());
117- break;
118+ } else {
119+ TxValidationState tx_state;
120+ if (!CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache)) {
121+ // Any transaction validation failure in ConnectBlock is a block consensus failure
122+ state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
123+ tx_state.GetRejectReason(), tx_state.GetDebugMessage());
124+ break;
125+ }
126 }
127 }
