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.

Despite being primarily intended for improving the situation for standard transactions only, the logic as implemented here is active for all script execution, including non-standard transactions, and including for consensus validation (for transactions seen in blocks). 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). I’m open to discussing alternatives, however.

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
Concept ACK	darosior

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:225 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.

tests: add sighash caching tests to feature_taproot 902fdf31e7

script: (refactor) prepare for introducing sighash midstate cache cb0329ed6a

script: (optimization) introduce sighash midstate caching dac139b32f

sipa force-pushed on May 13, 2025

in test/functional/feature_taproot.py:1240 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?

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts