7726a6a validation, refactor: Remove single threaded special case:
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:
diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp (revision 87fcd645598d186bd1e4f68d5aca275dc7815fda)
+++ b/src/validation.cpp (date 1772729378036)
@@ -137,11 +137,17 @@
return m_chain.Genesis();
}
-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
- const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
- bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
- ValidationCache& validation_cache,
- std::vector<CScriptCheck>* pvChecks = nullptr)
+void CollectScriptChecks(const CTransaction& tx,
+ const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
+ ValidationCache& validation_cache,
+ std::vector<CScriptCheck>& pvChecks)
+ EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
+ const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
+ ValidationCache& validation_cache)
EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction& tx)
@@ -2060,8 +2066,8 @@
return spent_outputs;
}
-/** Perform pre-checks for CheckInputScripts: skip coinbase, check the script
- * execution cache, reserve space for script checks, and initialize txdata.
+/** Perform pre-checks for script validation: skip coinbase, check the script
+ * execution cache, and initialize txdata.
* Returns nullopt if the transaction is already validated (caller should
* return true), or the cache entry hash needed to store results later. */
static std::optional<uint256> LookupScriptValidation(
@@ -2090,16 +2096,28 @@
return hashCacheEntry;
}
+/** Collect script checks for all inputs without executing them.
+ * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp */
+void CollectScriptChecks(const CTransaction& tx,
+ const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
+ ValidationCache& validation_cache,
+ std::vector<CScriptCheck>& checks) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+{
+ if (LookupScriptValidation(tx, inputs, flags, cacheFullScriptStore, txdata, validation_cache)) {
+ checks.reserve(tx.vin.size());
+ for (unsigned int i = 0; i < tx.vin.size(); i++) {
+ checks.emplace_back(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata);
+ }
+ }
+}
+
/**
* Check whether all of this transaction's input scripts succeed.
*
* This involves ECDSA signature checks so can be computationally intensive. This function should
* only be called after the cheap sanity checks in CheckTxInputs passed.
*
- * If pvChecks is not nullptr, script checks are pushed onto it instead of being performed inline. Any
- * script checks which are not necessary (eg due to script execution cache hits) are, obviously,
- * not pushed onto pvChecks/run.
- *
* Setting cacheSigStore/cacheFullScriptStore to false will remove elements from the corresponding cache
* which are matched. This is useful for checking blocks where we will likely never need the cache
* entry again.
@@ -2112,8 +2130,7 @@
bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore,
bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
- ValidationCache& validation_cache,
- std::vector<CScriptCheck>* pvChecks)
+ ValidationCache& validation_cache)
{
const auto hash_cache_entry{LookupScriptValidation(tx, inputs, flags, cacheFullScriptStore, txdata, validation_cache)};
if (!hash_cache_entry) return true;
@@ -2128,10 +2145,7 @@
// Verify signature
CScriptCheck check(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata);
- if (pvChecks) {
- pvChecks->reserve(tx.vin.size());
- pvChecks->emplace_back(std::move(check));
- } else if (auto result = check(); result.has_value()) {
+ if (auto result = check()) {
// Tx failures never trigger disconnections/bans.
// This is so that network splits aren't triggered
// either due to non-consensus relay policies (such as
@@ -2146,7 +2160,7 @@
}
}
- if (cacheFullScriptStore && !pvChecks) {
+ if (cacheFullScriptStore) {
// We executed all of the provided scripts, and were told to
// cache the result. Do so now.
validation_cache.CacheScriptValidation(*hash_cache_entry);
@@ -2596,18 +2610,20 @@
if (!tx.IsCoinBase() && fScriptChecks)
{
bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
- TxValidationState tx_state;
if (control) {
// CheckInputScripts always returns true when pvChecks is provided
// (checks are collected for deferred execution, not executed inline).
std::vector<CScriptCheck> vChecks;
- Assume(CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks));
+ CollectScriptChecks(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, vChecks);
control->Add(std::move(vChecks));
- } else if (!CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache)) {
- // Any transaction validation failure in ConnectBlock is a block consensus failure
- state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
- tx_state.GetRejectReason(), tx_state.GetDebugMessage());
- break;
+ } else {
+ TxValidationState tx_state;
+ if (!CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache)) {
+ // Any transaction validation failure in ConnectBlock is a block consensus failure
+ state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
+ tx_state.GetRejectReason(), tx_state.GetDebugMessage());
+ break;
+ }
}
}
