Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts

sipa commented at 5:49 pm on May 12, 2025: member

This introduces a per-txin cache for sighash midstate computation to the script interpreter for legacy (bare), P2SH, P2WSH, and (as collateral effect, but not actually useful) P2WPKH. This reduces the impact of certain types of quadratic hashing attacks that use standard transactions. It is not known to improve the situation for attacks involving non-standard transaction attacks.

The cache works by remembering for each of the 6 sighash modes a (scriptCode, midstate) tuple, which gives a midstate CSHA256 object right before the appending of the sighash type itself (to permit all 256, rather than just the 6 ones that match the modes). The midstate is only reused if the scriptCode matches. This works because - within a single input - only the sighash type and the scriptCode affect the actual sighash used.

The PR implements two different approaches:

The initial commits introduce the caching effect always, for both consensus and relay relation validation. Despite being primarily intended for improving the situation for standard transactions only, I chose this approach as the code paths are already largely common between the two, and this approach I believe involves fewer code changes than a more targetted approach, and furthermore, it should not hurt (it may even help common multisig cases slightly).
The final commit changes the behavior to only using the cache for non-consensus script validation. I’m open to feedback about whether adding this commit is worth it.

Functional tests are included that construct contrived cases with many sighash types (standard and non-standard ones) and OP_CODESEPARATORs in all script types (including P2TR, which isn’t modified by this PR).

DrahtBot commented at 5:49 pm on May 12, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32473.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, darosior, achow101
Concept ACK	l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32301 (test: cover invalid codesep positions for signature in taproot by instagibbs)
#32247 (BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only) by jamesob)

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.

in test/functional/feature_taproot.py:222 in 26fced465b outdated

221@@ -218,6 +222,20 @@ def default_controlblock(ctx):
222     """Default expression for "controlblock": combine leafversion, negflag, pubkey_internal, merklebranch."""
223     return bytes([get(ctx, "leafversion") + get(ctx, "negflag")]) + get(ctx, "pubkey_internal") + get(ctx, "merklebranch")
224 
225+def default_scriptcode_suffix(ctx):

darosior commented at 6:00 pm on May 12, 2025:

Should it be named default_scriptcode_wsh to be explicit that it implements the Segwit v0 (and not legacy) logic with regard to OP_CODESEPARATOR serialization?

sipa commented at 6:29 pm on May 12, 2025:

Hmm, this didn’t change between legacy and witv0, I think?

darosior commented at 7:03 pm on May 12, 2025:

It did, legacy skips OP_CODESEPARATORs in the suffix (i.e. “upcoming CODESEPs”) whereas Segwit v0 doesn’t.

sipa commented at 7:15 pm on May 12, 2025:

Ah, yes, but this all happens before the call to LegacySignatureMsg(), which filters out the OP_CODESEPARATORs that remain. In either case, this function default_scriptcode_suffix is exercised for both p2wsh and p2sh, so the fact that the test works should mean something…

bitcoin deleted a comment on May 12, 2025

laanwj added the label Validation on May 13, 2025

darosior commented at 2:20 pm on May 13, 2025: member

Neat. Concept ACK.

I’m open to discussing alternatives, however.

I think enabling it by consensus is unnecessarily risky. I would rather take some slightly more complicate logic, for instance by having CScriptCheck own the sighash cache:

  0diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
  1index ffac2acb948..fe24dae6c54 100644
  2--- a/src/script/interpreter.cpp
  3+++ b/src/script/interpreter.cpp
  4@@ -1700,7 +1700,7 @@ bool GenericTransactionSignatureChecker<T>::CheckECDSASignature(const std::vecto
  5     // Witness sighashes need the amount.
  6     if (sigversion == SigVersion::WITNESS_V0 && amount < 0) return HandleMissingData(m_mdb);
  7 
  8-    uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata, &m_sighash_cache);
  9+    uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata, m_sighash_cache);
 10 
 11     if (!VerifyECDSASignature(vchSig, pubkey, sighash))
 12         return false;
 13diff --git a/src/script/interpreter.h b/src/script/interpreter.h
 14index 2108e3992fc..6c84c6dd74c 100644
 15--- a/src/script/interpreter.h
 16+++ b/src/script/interpreter.h
 17@@ -308,15 +308,15 @@ private:
 18     unsigned int nIn;
 19     const CAmount amount;
 20     const PrecomputedTransactionData* txdata;
 21-    mutable SigHashCache m_sighash_cache;
 22+    mutable SigHashCache* m_sighash_cache;
 23 
 24 protected:
 25     virtual bool VerifyECDSASignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const;
 26     virtual bool VerifySchnorrSignature(std::span<const unsigned char> sig, const XOnlyPubKey& pubkey, const uint256& sighash) const;
 27 
 28 public:
 29-    GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, MissingDataBehavior mdb) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(nullptr) {}
 30-    GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn, MissingDataBehavior mdb) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {}
 31+    GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, MissingDataBehavior mdb, SigHashCache* sighash_cache = nullptr) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(nullptr), m_sighash_cache(sighash_cache) {}
 32+    GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn, MissingDataBehavior mdb, SigHashCache* sighash_cache = nullptr) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(&txdataIn), m_sighash_cache(sighash_cache) {}
 33     bool CheckECDSASignature(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override;
 34     bool CheckSchnorrSignature(std::span<const unsigned char> sig, std::span<const unsigned char> pubkey, SigVersion sigversion, ScriptExecutionData& execdata, ScriptError* serror = nullptr) const override;
 35     bool CheckLockTime(const CScriptNum& nLockTime) const override;
 36diff --git a/src/script/sigcache.h b/src/script/sigcache.h
 37index ca4b44910e6..89aa5e85ad8 100644
 38--- a/src/script/sigcache.h
 39+++ b/src/script/sigcache.h
 40@@ -67,7 +67,7 @@ private:
 41     SignatureCache& m_signature_cache;
 42 
 43 public:
 44-    CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, bool storeIn, SignatureCache& signature_cache, PrecomputedTransactionData& txdataIn) : TransactionSignatureChecker(txToIn, nInIn, amountIn, txdataIn, MissingDataBehavior::ASSERT_FAIL), store(storeIn), m_signature_cache(signature_cache)  {}
 45+    CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, bool storeIn, SignatureCache& signature_cache, PrecomputedTransactionData& txdataIn, SigHashCache* sighash_cache = nullptr) : TransactionSignatureChecker(txToIn, nInIn, amountIn, txdataIn, MissingDataBehavior::ASSERT_FAIL, sighash_cache), store(storeIn), m_signature_cache(signature_cache) {}
 46 
 47     bool VerifyECDSASignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const override;
 48     bool VerifySchnorrSignature(std::span<const unsigned char> sig, const XOnlyPubKey& pubkey, const uint256& sighash) const override;
 49diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
 50index af36a956931..d2f97f47461 100644
 51--- a/src/test/txvalidationcache_tests.cpp
 52+++ b/src/test/txvalidationcache_tests.cpp
 53@@ -24,7 +24,8 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
 54                        const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
 55                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
 56                        ValidationCache& validation_cache,
 57-                       std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 58+                       std::vector<CScriptCheck>* pvChecks,
 59+                       bool cache_sighash = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 60 
 61 BOOST_AUTO_TEST_SUITE(txvalidationcache_tests)
 62 
 63diff --git a/src/validation.cpp b/src/validation.cpp
 64index 5ad2ebdcd7e..58bdc8baaf3 100644
 65--- a/src/validation.cpp
 66+++ b/src/validation.cpp
 67@@ -142,7 +142,8 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
 68                        const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
 69                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
 70                        ValidationCache& validation_cache,
 71-                       std::vector<CScriptCheck>* pvChecks = nullptr)
 72+                       std::vector<CScriptCheck>* pvChecks = nullptr,
 73+                       bool cache_sighash = false)
 74                        EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 75 
 76 bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction& tx)
 77@@ -1237,13 +1238,13 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws)
 78 
 79     // Check input scripts and signatures.
 80     // This is done last to help prevent CPU exhaustion denial-of-service attacks.
 81-    if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache())) {
 82+    if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache(), /* pvChecks= */ nullptr, /* cache_sighash= */ true)) {
 83         // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
 84         // need to turn both off, and compare against just turning off CLEANSTACK
 85         // to see if the failure is specifically due to witness validation.
 86         TxValidationState state_dummy; // Want reported failures to be from first CheckInputScripts
 87-        if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata, GetValidationCache()) &&
 88-                !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata, GetValidationCache())) {
 89+        if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata, GetValidationCache(), /* pvChecks= */ nullptr, /* cache_sighash= */ true) &&
 90+                !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata, GetValidationCache(), /* pvChecks= */ nullptr, /* cache_sighash= */ true)) {
 91             // Only the witness is missing, so the transaction itself may be fine.
 92             state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED,
 93                     state.GetRejectReason(), state.GetDebugMessage());
 94@@ -2119,7 +2120,7 @@ std::optional<std::pair<ScriptError, std::string>> CScriptCheck::operator()() {
 95     const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
 96     const CScriptWitness *witness = &ptxTo->vin[nIn].scriptWitness;
 97     ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR};
 98-    if (VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error)) {
 99+    if (VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata, m_sighash_cache.get()), &error)) {
100         return std::nullopt;
101     } else {
102         auto debug_str = strprintf("input %i of %s (wtxid %s), spending %s:%i", nIn, ptxTo->GetHash().ToString(), ptxTo->GetWitnessHash().ToString(), ptxTo->vin[nIn].prevout.hash.ToString(), ptxTo->vin[nIn].prevout.n);
103@@ -2166,7 +2167,8 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
104                        const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
105                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
106                        ValidationCache& validation_cache,
107-                       std::vector<CScriptCheck>* pvChecks)
108+                       std::vector<CScriptCheck>* pvChecks,
109+                       bool cache_sighash)
110 {
111     if (tx.IsCoinBase()) return true;
112 
113diff --git a/src/validation.h b/src/validation.h
114index 7411ab400c2..842e3f0d28a 100644
115--- a/src/validation.h
116+++ b/src/validation.h
117@@ -338,10 +338,11 @@ private:
118     bool cacheStore;
119     PrecomputedTransactionData *txdata;
120     SignatureCache* m_signature_cache;
121+    std::unique_ptr<SigHashCache> m_sighash_cache;
122 
123 public:
124-    CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, SignatureCache& signature_cache, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :
125-        m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn), m_signature_cache(&signature_cache) { }
126+    CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, SignatureCache& signature_cache, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn, bool cache_sighash = false) :
127+        m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn), m_signature_cache(&signature_cache), m_sighash_cache(cache_sighash ? std::make_unique<SigHashCache>() : nullptr) { }
128 
129     CScriptCheck(const CScriptCheck&) = delete;
130     CScriptCheck& operator=(const CScriptCheck&) = delete;

in src/script/interpreter.cpp:1609 in cbf10305d2 outdated

1577+                return uint256::ONE;
1578+            }
1579+        }
1580+    }
1581+
1582+    HashWriter ss{};

theuni commented at 5:58 pm on May 13, 2025:

HashWriter ss{} in the if() branch below now shadows this one. Rename to avoid confusion (and warnings) ?

sipa commented at 6:29 pm on May 13, 2025:

Done.

instagibbs commented at 6:14 pm on May 13, 2025: member

@darosior just noting the tests are specifically written to not worry about standardness, so at least these would have to be augmented to cover the cases this PR is ostensibly supporting

sipa commented at 6:21 pm on May 13, 2025: member

@instagibbs Yes and no, depending on the meaning of standardness I guess. The worrisome transactions are non-standard (in the sense that they are not relayed/accepted), but are standard in the IsStandard() sense, and thus not immediately rejected before script execution even begins.

sipa force-pushed on May 13, 2025

in test/functional/feature_taproot.py:1241 in 902fdf31e7 outdated

1233@@ -1207,6 +1234,70 @@ def predict_sigops_ratio(n, dummy_size):
1234                 standard = hashtype in VALID_SIGHASHES_ECDSA and (p2sh or witv0)
1235                 add_spender(spenders, "compat/nocsa", hashtype=hashtype, p2sh=p2sh, witv0=witv0, standard=standard, script=CScript([OP_IF, OP_11, pubkey1, OP_CHECKSIGADD, OP_12, OP_EQUAL, OP_ELSE, pubkey1, OP_CHECKSIG, OP_ENDIF]), key=eckey1, sigops_weight=4-3*witv0, inputs=[getter("sign"), b''], failure={"inputs": [getter("sign"), b'\x01']}, **ERR_BAD_OPCODE)
1236 
1237+    # == sighash caching tests ==
1238+
1239+    # Sighash caching in legacy.
1240+    for p2sh in [False, True]:

instagibbs commented at 6:44 pm on May 13, 2025:

scriptsig-size failures are being hit for non-witness cases, which I think would stop caching?

sipa commented at 8:57 pm on July 9, 2025:

Fixed.

instagibbs commented at 6:16 pm on July 11, 2025:

looks right, 20 maximally sized sigs should fit under limits

theuni commented at 8:16 pm on May 13, 2025: member

Hmm, not relevant to what’s being addressed here, but couldn’t the per-tx cache->hashPrevouts and cache->hashSequence be replaced with a similar midstate cache trick?

Edit: Given their size I guess that’s not nearly as interesting.

sipa force-pushed on Jul 9, 2025

sipa commented at 9:02 pm on July 9, 2025: member

@darosior I did it slightly differently, but added a commit that makes the use of the sighash cache optional. I avoided using default values as they can result in unexpected/missed call sites.

sipa force-pushed on Jul 9, 2025

DrahtBot added the label CI failed on Jul 9, 2025

DrahtBot commented at 9:32 pm on July 9, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/45674313491 LLM reason (✨ experimental): Clang-tidy reported argument comment mismatches caused by warnings treated as errors, leading to build failure.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

sipa force-pushed on Jul 10, 2025

sipa force-pushed on Jul 11, 2025

DrahtBot removed the label CI failed on Jul 11, 2025

in test/functional/feature_taproot.py:181 in fb7e9decf3 outdated

178     """Return a callable that evaluates name in its passed context."""
179-    return lambda ctx: get(ctx, name)
180+    if len(kwargs) == 0:
181+        return lambda ctx: get(ctx, name)
182+    else:
183+        return lambda ctx: get({**ctx, **kwargs}, name)

darosior commented at 3:06 pm on July 21, 2025:

nit: no need to special-case when kwargs is empty since ctx == {**ctx, **{}}.

0    return lambda ctx: get({**ctx, **kwargs}, name)

sipa commented at 1:39 pm on August 6, 2025:

Good point. Gone.

in test/functional/feature_taproot.py:325 in fb7e9decf3 outdated

324+    mode = get(ctx, "mode")
325+    hashtype_actual = get(ctx, "hashtype_actual")
326+    if mode != "taproot" or hashtype_actual != 0:
327+        return bytes([hashtype_actual])
328+    else:
329+        return bytes()

darosior commented at 7:04 pm on July 21, 2025:

nit: why is that needed? Looks like it may have been necessary for an earlier version of the test that had a 0 sighash type in the ops list for the legacy signature caching test, but it is not necessary anymore.

sipa commented at 1:39 pm on August 6, 2025:

Maybe there are currently no tests that actually make use of this, but it’s still correct: it’s not illegal to have a signature with sighash type 0 in non-taproot, and it would need to be serialized as an actual 0 at the end.

in src/script/interpreter.cpp:1573 in fb7e9decf3 outdated

1568+{
1569+    // Note that we do not distinguish between BASE and WITNESS_V0 to determine the cache index,
1570+    // because no input can simultaneously use both.
1571+    return 3 * !!(hash_type & SIGHASH_ANYONECANPAY) +
1572+           2 * ((hash_type & 0x1f) == SIGHASH_SINGLE) +
1573+           1 * ((hash_type & 0x1f) == SIGHASH_NONE);

darosior commented at 7:56 pm on July 21, 2025:

Various modifications of this code does not make any test fail. For instance:

 0diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
 1index 528d8851d5f..81fbc4ab151 100644
 2--- a/src/script/interpreter.cpp
 3+++ b/src/script/interpreter.cpp
 4@@ -1569,8 +1569,7 @@ int SigHashCache::CacheIndex(int32_t hash_type) const noexcept
 5     // Note that we do not distinguish between BASE and WITNESS_V0 to determine the cache index,
 6     // because no input can simultaneously use both.
 7     return 3 * !!(hash_type & SIGHASH_ANYONECANPAY) +
 8-           2 * ((hash_type & 0x1f) == SIGHASH_SINGLE) +
 9-           1 * ((hash_type & 0x1f) == SIGHASH_NONE);
10+           2 * ((hash_type & 0x1f) == SIGHASH_SINGLE);
11 }
12 
13 bool SigHashCache::Load(int32_t hash_type, const CScript& script_code, HashWriter& writer) const noexcept

sipa commented at 1:38 pm on August 6, 2025:

Nice catch. Seems your fuzz test below (now included here) catches this.

darosior commented at 4:03 pm on July 22, 2025: member

I’ve been playing with this code for the past couple of days and this is looking good. I wrote a unit test which sanity checks the caching and notably exercises false positive cache hits, which appears to not currently be covered (see #32473 (review)): https://github.com/darosior/bitcoin/commit/f303c6d68b5b9b3b23af34c8ef9efa7822fbe38b. I ran IBD on mainnet and signet with caching for blocks enabled and it did not result in any historical block being rejected on either of these networks. I’ll do testnet3 and testnet4 next.

I’m now looking into adding some basic fuzzing coverage that computing the sighash with or without a cache is equivalent. I’ll also be running two mainnet nodes on the same machine, one with caching and one without. Since about 5% of all inputs are legacy or p2wsh multisigs, i’m curious to see if it makes any difference.

darosior commented at 5:16 pm on July 23, 2025: member

Here is a fuzz test which asserts the cached and not cached versions of the sighash function always return the same result: https://github.com/bitcoin/bitcoin/commit/3c056d766d6311e4120d43705b53059e3b1b0bb1. Running it for a while here did not reveal anything.

I ran IBD on mainnet and signet with caching for blocks enabled and it did not result in any historical block being rejected on either of these networks. I’ll do testnet3 and testnet4 next.

Done that and it didn’t cause any issue either. (Stopped at December 2024 for testnet3.)

DrahtBot added the label Needs rebase on Jul 25, 2025

darosior commented at 2:52 pm on July 28, 2025: member

I ran two identically compiled nodes on two identical VMs on the same host, one on top of fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c (this PR but modified to enable caching for block validation too) and one on top of 2cad7226c2d02ec67bbac7ec909030f8bae593f8 (the commit this PR is based on).

Over 4 days of blocks (from height 906875 (00000000000000000000264374e6d9b26be206793712e94266a62ddb0e9551e9) to height 907450 (00000000000000000001d85ba0ff861fd2499bd72fd436e2c37e11e0c28caf74)), the verification time of all inputs in the block (as recorded by the Verify XXXX txins: Y.YYms log line) took on average 62.45ms on the node running this PR with caching enabled for blocks, and took on average 64.46ms on the other node. (I had to first correct for stale block 00000000000000000001aad53cc3e15fcf36766a25b15ac8c6d84fe2c2055e82 which was validated by one node but not the other.)

It’s interesting to see that it’s faster on average, as it’s slightly slower for inexpensive blocks. As one would expect, it’s more than made up for by being substantially faster for more expensive blocks. As an illustration of this, the median time is 8.93ms for the node running this PR with caching enabled for blocks and 8.45ms for the other one.

darosior approved

darosior commented at 2:35 pm on July 29, 2025: member

ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c

l0rinc commented at 5:30 pm on July 31, 2025: contributor

I ran two identically compiled nodes on two identical VMs on the same host

I did something similar, measuring two reindex runs until 900k blocks for master vs current PR (rebased) vs enabling all use_sighash_cache values.

 0diff --git a/src/psbt.cpp b/src/psbt.cpp
 1index 7167d13af3..6d57c80de3 100644
 2--- a/src/psbt.cpp
 3+++ b/src/psbt.cpp
 4@@ -321,7 +321,7 @@ bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned
 5     }
 6 
 7     if (txdata) {
 8-        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/false});
 9+        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/true});
10     } else {
11         return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, MissingDataBehavior::FAIL});
12     }
13diff --git a/src/script/sign.cpp b/src/script/sign.cpp
14index 0310a5bf02..5c9b689e0d 100644
15--- a/src/script/sign.cpp
16+++ b/src/script/sign.cpp
17@@ -28,7 +28,7 @@ MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMu
18 
19 MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction& tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type)
20     : m_txto{tx}, nIn{input_idx}, nHashType{hash_type}, amount{amount},
21-      checker{txdata ? MutableTransactionSignatureChecker{&m_txto, nIn, amount, *txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/false} :
22+      checker{txdata ? MutableTransactionSignatureChecker{&m_txto, nIn, amount, *txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/true} :
23                        MutableTransactionSignatureChecker{&m_txto, nIn, amount, MissingDataBehavior::FAIL}},
24       m_txdata(txdata)
25 {
26@@ -813,7 +813,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
27         }
28 
29         ScriptError serror = SCRIPT_ERR_OK;
30-        if (!sigdata.complete && !VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/false), &serror)) {
31+        if (!sigdata.complete && !VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/true), &serror)) {
32             if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
33                 // Unable to sign input and verification failed (possible attempt to partially sign).
34                 input_errors[i] = Untranslated("Unable to sign input, invalid stack size (possibly missing key)");
35diff --git a/src/signet.cpp b/src/signet.cpp
36index d43daca51c..91031b2d09 100644
37--- a/src/signet.cpp
38+++ b/src/signet.cpp
39@@ -142,7 +142,7 @@ bool CheckSignetBlockSolution(const CBlock& block, const Consensus::Params& cons
40 
41     PrecomputedTransactionData txdata;
42     txdata.Init(signet_txs->m_to_sign, {signet_txs->m_to_spend.vout[0]});
43-    TransactionSignatureChecker sigcheck(&signet_txs->m_to_sign, /* nInIn= */ 0, /* amountIn= */ signet_txs->m_to_spend.vout[0].nValue, txdata, MissingDataBehavior::ASSERT_FAIL, /*use_sighash_cache=*/false);
44+    TransactionSignatureChecker sigcheck(&signet_txs->m_to_sign, /* nInIn= */ 0, /* amountIn= */ signet_txs->m_to_spend.vout[0].nValue, txdata, MissingDataBehavior::ASSERT_FAIL, /*use_sighash_cache=*/true);
45 
46     if (!VerifyScript(scriptSig, signet_txs->m_to_spend.vout[0].scriptPubKey, &witness, BLOCK_SCRIPT_VERIFY_FLAGS, sigcheck)) {
47         LogDebug(BCLog::VALIDATION, "CheckSignetBlockSolution: Errors in block (block solution invalid)\n");
48diff --git a/src/validation.cpp b/src/validation.cpp
49index 6b25ea3e3d..3143172d01 100644
50--- a/src/validation.cpp
51+++ b/src/validation.cpp
52@@ -2677,10 +2677,10 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
53             // they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning.
54             if (control) {
55                 std::vector<CScriptCheck> vChecks;
56-                tx_ok = CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, /*use_sighash_cache=*/false, txsdata[i], m_chainman.m_validation_cache, &vChecks);
57+                tx_ok = CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, /*use_sighash_cache=*/true, txsdata[i], m_chainman.m_validation_cache, &vChecks);
58                 if (tx_ok) control->Add(std::move(vChecks));
59             } else {
60-                tx_ok = CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, /*use_sighash_cache=*/false, txsdata[i], m_chainman.m_validation_cache);
61+                tx_ok = CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, /*use_sighash_cache=*/true, txsdata[i], m_chainman.m_validation_cache);
62             }
63             if (!tx_ok) {
64                 // Any transaction validation failure in ConnectBlock is a block consensus failure
65diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
66index 243a300cb0..e32ef5cb89 100644
67--- a/src/wallet/feebumper.cpp
68+++ b/src/wallet/feebumper.cpp
69@@ -222,7 +222,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const Txid& txid, const CCoinC
70             // In order to do this, we verify the script with a special SignatureChecker which
71             // will observe the signatures verified and record their sizes.
72             SignatureWeights weights;
73-            TransactionSignatureChecker tx_checker(wtx.tx.get(), i, coin.out.nValue, txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/false);
74+            TransactionSignatureChecker tx_checker(wtx.tx.get(), i, coin.out.nValue, txdata, MissingDataBehavior::FAIL, /*use_sighash_cache=*/true);
75             SignatureWeightChecker size_checker(weights, tx_checker);
76             VerifyScript(txin.scriptSig, coin.out.scriptPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, size_checker);
77             // Add the difference between max and current to input_weight so that it represents the largest the input could be

The results are similar, the current PR doesn’t cause any slowdown, while fully enabling caching (or at least doing the above) would speed it up by 1%.

 0COMMITS="e6018665f8ef58a8fcebe6f48bb6c5378d1ff3c2 2ac472ba7ab5b6eb93e6497ebfde3c1a9cd62cd1 6ed17b6ca9a61bba614eb8bb9fd3cfd6fdc90cf7"; \
 1STOP=900000; DBCACHE=10000; \
 2CC=gcc; CXX=g++; \
 3BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
 4(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
 5hyperfine \
 6  --sort command \
 7  --runs 2 \
 8  --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
 9  --parameter-list COMMIT ${COMMITS// /,} \
10  --prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
11    cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind && \
12    ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 10" \
13  --cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
14  "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0"
15
16e6018665f8 tests: add sighash caching tests to feature_taproot
172ac472ba7a script: (belt-and-suspenders) make sighash cache optional
186ed17b6ca9 /*use_sighash_cache=*/true
19
20Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6018665f8ef58a8fcebe6f48bb6c5378d1ff3c2)
21  Time (mean ± σ):     36170.929 s ± 14.801 s    [User: 366606.776 s, System: 1076.452 s]
22  Range (min … max):   36160.463 s … 36181.395 s    2 runs
23 
24Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 2ac472ba7ab5b6eb93e6497ebfde3c1a9cd62cd1)
25  Time (mean ± σ):     36192.806 s ± 43.410 s    [User: 366747.522 s, System: 1084.112 s]
26  Range (min … max):   36162.111 s … 36223.501 s    2 runs
27 
28Benchmark 3: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 6ed17b6ca9a61bba614eb8bb9fd3cfd6fdc90cf7)
29 ⠼ Current estimate: 35772.481 s 
30  Time (mean ± σ):     35773.556 s ±  1.521 s    [User: 362730.406 s, System: 1067.860 s]
31  Range (min … max):   35772.481 s … 35774.632 s    2 runs
32 
33Relative speed comparison
34        1.01 ±  0.00  COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6018665f8ef58a8fcebe6f48bb6c5378d1ff3c2)
35        1.01 ±  0.00  COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 2ac472ba7ab5b6eb93e6497ebfde3c1a9cd62cd1)
36        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -assumevalid=0 -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 6ed17b6ca9a61bba614eb8bb9fd3cfd6fdc90cf7)

Concept ACK

dergoegge approved

dergoegge commented at 10:38 am on August 5, 2025: member

Code review ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c

DrahtBot requested review from l0rinc on Aug 5, 2025

sipa force-pushed on Aug 5, 2025

DrahtBot removed the label Needs rebase on Aug 5, 2025

sipa commented at 6:01 pm on August 5, 2025: member

Rebased.

theuni commented at 6:22 pm on August 5, 2025: member

The final commit changes the behavior to only using the cache for non-consensus script validation. I’m open to feedback about whether adding this commit is worth it.

I’m not sure I understand the rationale for this. Consensus validation will still be affected by the new midstate caches by virtue of hitting a cached entry in the script execution cache which used them previously, no?

sipa commented at 6:36 pm on August 5, 2025: member

Consensus validation will still be affected by the new midstate caches by virtue of hitting a cached entry in the script execution cache which used them previously, no?

No, the cache only lives for the duration of one script execution. It does not survive beyond script execution, let alone across multiple validations of the same transaction. With the last commit, the cache object just won’t be present in consensus-critical validations.

I’m ambivalent about the last commit, though. It’s a worthwhile goal to avoid touching consensus code where possible, but in this case it still means touching the code in a mildly invasive manner, just one that (hopefully clearly) does not result in modified behavior. If the last commit doesn’t reach the “easy to verify the result means unmodified behavior” bar, it may be better to get rid of it, and have the cache always be active (but even then, it won’t result in cache being reused across multiple script/transaction validations).

darosior commented at 6:40 pm on August 5, 2025: member

Consensus validation will still be affected by the new midstate caches by virtue of hitting a cached entry in the script execution cache which used them previously, no?

No, the cache only lives for the duration of one script execution.

Worth noting (as i had to convince myself during review) that it also does not affect affect consensus validation by means of the signature cache. The signature cache is looked up by sighash (among other things), so if the sighash differs because there was a bug in the sighash caching then it will not use the corresponding entry inside block validation.

theuni commented at 7:09 pm on August 5, 2025: member

No, the cache only lives for the duration of one script execution. It does not survive beyond script execution, let alone across multiple validations of the same transaction. With the last commit, the cache object just won’t be present in consensus-critical validations.

Heh, there are so many levels of caching here.. my question still stands though…

Worth noting (as i had to convince myself during review) that it also does not affect affect consensus validation by means of the signature cache. The signature cache is looked up by sighash (among other things), so if the sighash differs because there was a bug in the sighash caching then it will not use the corresponding entry inside block validation.

Agreed that this protects the signature cache, as the sighash is calculated without the midstate caches for consensus validation.

But I’m asking specifically about the script execution cache. CheckInputsFromMempoolAndCache calls CheckInputScripts with cacheFullScriptStore=true and use_sighash_cache=true. ConnectBlock then calls it with use_sighash_cache=false, but it’ll be found in the execution cache, so the consensus checks are effectively using the new sigcache midstate caches. Or am I missing something?

I’m ambivalent about the last commit, though. It’s a worthwhile goal to avoid touching consensus code where possible, but in this case it still means touching the code in a mildly invasive manner, just one that (hopefully clearly) does not result in modified behavior. If the last commit doesn’t reach the “easy to verify the result means unmodified behavior” bar, it may be better to get rid of it, and have the cache always be active (but even then, it won’t result in cache being reused across multiple script/transaction validations).

Yeah, imo this would be easier to reason about (and concerns like mine above moot) with unified code paths.

darosior commented at 7:59 pm on August 5, 2025: member

But I’m asking specifically about the script execution cache.

Oh right! I think you are correct and use_sighash_cache should not be set for CheckInputsFromMempoolAndCache.

Yeah, imo this would be easier to reason about (and concerns like mine above moot) with unified code paths.

My preference is still to not touch consensus code when unnecessary, and it seems appropriately setting use_sighash_cache in CheckInputsFromMempoolAndCache would achieve that. But if both Pieter and you prefer it this way, that’s fine by me. I’ve significantly tested this code and i am confident it is correct. If we do (intentionally..) enable it for consensus, i think it would be nice to get the unit test and fuzz coverage i shared above.

achow101 commented at 9:24 pm on August 5, 2025: member

I would prefer having the cache always active as I find that much easier to reason about.

sipa force-pushed on Aug 6, 2025

tests: add sighash caching tests to feature_taproot 9014d4016a

script: (refactor) prepare for introducing sighash midstate cache 8f3ddb0bcc

script: (optimization) introduce sighash midstate caching 92af9f74d7

qa: simple differential fuzzing for sighash with/without caching b221aa80a0

sipa force-pushed on Aug 6, 2025

qa: unit test sighash caching 83950275ed

sipa commented at 1:45 pm on August 6, 2025: member

@theuni

so the consensus checks are effectively using the new sigcache midstate caches. Or am I missing something?

Good point. I think that further undermines the idea of disabling it for consensus-critical checks, as even though it’s possible to set the flags in such a way that any script-execution-cache-storing paths also disable sighash midstate caches, that makes it even less obvious to readers. @achow101

I would prefer having the cache always active as I find that much easier to reason about.

Done. @darosior

If we do (intentionally..) enable it for consensus, i think it would be nice to get the unit test and fuzz coverage i shared above.

Done.

achow101 added this to the milestone 30.0 on Aug 7, 2025

dergoegge approved

dergoegge commented at 9:36 am on August 8, 2025: member

Code review ACK 83950275eddacac56c58a7a3648ed435a5593328

DrahtBot requested review from darosior on Aug 8, 2025

darosior approved

darosior commented at 2:45 pm on August 8, 2025: member

re-ACK 83950275eddacac56c58a7a3648ed435a5593328

achow101 commented at 0:10 am on August 9, 2025: member

ACK 83950275eddacac56c58a7a3648ed435a5593328

fanquake merged this on Aug 11, 2025

fanquake closed this on Aug 11, 2025

Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts #32473

Code Coverage & Benchmarks

Reviews

Conflicts