Repository: bitcoin/bitcoin
File: src/validation.h (C/C++)

Code before:
public:
    CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, CSignatureCache& signature_cache, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :
        m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn), m_signature_cache(&signature_cache) { }

    CScriptCheck(const CScriptCheck&) = delete;
    CScriptCheck& operator=(const CScriptCheck&) = delete;
    CScriptCheck(CScriptCheck&&) = default;
    CScriptCheck& operator=(CScriptCheck&&) = default;

    bool operator()();

    ScriptError GetScriptError() const { return error; }
};

// CScriptCheck is used a lot in std::vector, make sure that's efficient
static_assert(std::is_nothrow_move_assignable_v<CScriptCheck>);
static_assert(std::is_nothrow_move_constructible_v<CScriptCheck>);
static_assert(std::is_nothrow_destructible_v<CScriptCheck>);

/**
 * Convenience struct for initializing and passing the script execution cache
 * and signature cache.
 */
struct ValidationCache {
    ScriptCache m_script_execution_cache;
    CSHA256 m_script_execution_cache_hasher;
    CSignatureCache m_signature_cache;
    ValidationCache(size_t script_execution_cache_bytes, size_t signature_cache_bytes);
};

/** Functions for validating blocks and updating the block tree */

/** Context-independent validity checks */
bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);

/** Check a block is completely valid from start to finish (only works on top of our current best block) */
bool TestBlockValidity(BlockValidationState& state,
                       const CChainParams& chainparams,
                       Chainstate& chainstate,
                       const CBlock& block,
                       CBlockIndex* pindexPrev,
                       bool fCheckPOW = true,
                       bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/** Check with the proof of work on each blockheader matches the value in nBits */
bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);

/** Check if a block has been mutated (with respect to its merkle root and witness commitments). */
bool IsBlockMutated(const CBlock& block, bool check_witness_root);

/** Return the sum of the claimed work on a given set of headers. No verification of PoW is done. */

Review comment (performance):
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
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?

Code after:
        m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn), m_signature_cache(&signature_cache) { }

    CScriptCheck(const CScriptCheck&) = delete;
    CScriptCheck& operator=(const CScriptCheck&) = delete;
    CScriptCheck(CScriptCheck&&) = default;
    CScriptCheck& operator=(CScriptCheck&&) = default;

    bool operator()();

    ScriptError GetScriptError() const { return error; }
};

// CScriptCheck is used a lot in std::vector, make sure that's efficient
static_assert(std::is_nothrow_move_assignable_v<CScriptCheck>);
static_assert(std::is_nothrow_move_constructible_v<CScriptCheck>);
static_assert(std::is_nothrow_destructible_v<CScriptCheck>);

/**
 * Convenience class for initializing and passing the script execution cache
 * and signature cache.
 */
class ValidationCache
{
private:
    //! Pre-initialized hasher to avoid having to recreate it for every hash calculation.
    CSHA256 m_script_execution_cache_hasher;

public:
    CuckooCache::cache<uint256, SignatureCacheHasher> m_script_execution_cache;
    SignatureCache m_signature_cache;

    ValidationCache(size_t script_execution_cache_bytes, size_t signature_cache_bytes);

    ValidationCache(const ValidationCache&) = delete;
    ValidationCache& operator=(const ValidationCache&) = delete;

    //! Return a copy of the pre-initialized hasher.
    CSHA256 ScriptExecutionCacheHasher() const { return m_script_execution_cache_hasher; }
};

/** Functions for validating blocks and updating the block tree */

/** Context-independent validity checks */
bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);

/** Check a block is completely valid from start to finish (only works on top of our current best block) */
bool TestBlockValidity(BlockValidationState& state,
                       const CChainParams& chainparams,
                       Chainstate& chainstate,
                       const CBlock& block,
                       CBlockIndex* pindexPrev,