If I understand correctly, m_script_execution_cache_hasher is a pre-initialized hasher that we want to copy every time we use it, and we definitely don't want to modify in-place. I think having it as a public struct member is not very robust to enforce that, perhaps better to use a getter here? E.g. something like:
<details>
<summary>git diff on 63923c8da6</summary>
diff --git a/src/validation.cpp b/src/validation.cpp
index 39ab7b56ab..600c427c93 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2095,17 +2095,22 @@ bool CScriptCheck::operator()() {
return VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error);
}
-ValidationCache::ValidationCache(size_t script_execution_cache_bytes, size_t signature_cache_bytes)
- : m_signature_cache{signature_cache_bytes / 2}
+CSHA256 ValidationCache::CreateScriptExecutionCacheHasher()
{
// Setup the salted hasher
uint256 nonce = GetRandHash();
// We want the nonce to be 64 bytes long to force the hasher to process
// this chunk, which makes later hash computations more efficient. We
// just write our 32-byte entropy twice to fill the 64 bytes.
- m_script_execution_cache_hasher.Write(nonce.begin(), 32);
- m_script_execution_cache_hasher.Write(nonce.begin(), 32);
+ CSHA256 hasher;
+ hasher.Write(nonce.begin(), 32);
+ hasher.Write(nonce.begin(), 32);
+ return hasher;
+}
+ValidationCache::ValidationCache(size_t script_execution_cache_bytes, size_t signature_cache_bytes)
+ : m_signature_cache{signature_cache_bytes / 2}
+{
const auto [num_elems, approx_size_bytes] = m_script_execution_cache.setup_bytes(script_execution_cache_bytes);
LogPrintf("Using %zu MiB out of %zu MiB requested for script execution cache, able to store %zu elements\n",
approx_size_bytes >> 20, script_execution_cache_bytes >> 20, num_elems);
@@ -2148,7 +2153,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
// properly commits to the scriptPubKey in the inputs view of that
// transaction).
uint256 hashCacheEntry;
- CSHA256 hasher = validation_cache.m_script_execution_cache_hasher;
+ auto hasher{validation_cache.ScriptExecutionCacheHasher()};
hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
if (validation_cache.m_script_execution_cache.contains(hashCacheEntry, !cacheFullScriptStore)) {
diff --git a/src/validation.h b/src/validation.h
index c4b49ec3e8..1e48f6b4a4 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -369,11 +369,16 @@ static_assert(std::is_nothrow_destructible_v<CScriptCheck>);
* Convenience struct for initializing and passing the script execution cache
* and signature cache.
*/
-struct ValidationCache {
+class ValidationCache {
+public:
ScriptCache m_script_execution_cache;
- CSHA256 m_script_execution_cache_hasher;
+ CSHA256 ScriptExecutionCacheHasher() { return m_script_execution_cache_hasher; }
CSignatureCache m_signature_cache;
ValidationCache(size_t script_execution_cache_bytes, size_t signature_cache_bytes);
+private:
+ static CSHA256 CreateScriptExecutionCacheHasher();
+ //! Pre-initialized hasher to avoid having to recreate it for every hash calculation
+ const CSHA256 m_script_execution_cache_hasher{CreateScriptExecutionCacheHasher()};
};
/** Functions for validating blocks and updating the block tree */
</details>
It seems this is quite brittle on master too, so not a regression, but probably something to address here?
