Accurate sigop/sighash accounting and limits

Adds a ValidationCostTracker class that is passed to CheckInputs() / CScriptCheck() to keep track of the exact number of signature operations required to validate a block, and the exact number of bytes hashed to compute signature hashes. Also extends CHashWriter to keep track of number of bytes hashed. Signature operations per block are limited to MAX_BLOCK_SIGOPS (unchanged at 20,000) Bytes hashed to compute signatures is limited to MAX_BLOCK_SIGHASH (1.3 GB in this commit). Conflicts: src/main.cpp src/miner.cpp src/script/interpreter.h
2026-05-27 06:14:42 +02:00 · 2016-01-22 11:03:50 -05:00
parent 76e123a3b3
commit 3aadc515e0
8 changed files with 138 additions and 27 deletions
--- a/src/consensus/consensus.h
+++ b/src/consensus/consensus.h
@@ -10,8 +10,10 @@
 static const unsigned int MAX_BLOCK_SIZE = 2000000;
 /** The old block size limit */
 static const unsigned int OLD_MAX_BLOCK_SIZE = 1000000;
-/** pre-2MB-fork limit on signature operations in a block */
+/** limit on signature operations in a block */
 static const unsigned int MAX_BLOCK_SIGOPS = OLD_MAX_BLOCK_SIZE/50;
+/** limit on number of bytes hashed to compute signatures in a block */
+static const unsigned int MAX_BLOCK_SIGHASH = 1300 * 1000 * 1000; // 1.3 gigabytes
 /** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */
 static const int COINBASE_MATURITY = 100;

--- a/src/hash.h
+++ b/src/hash.h
@@ -131,15 +131,17 @@ class CHashWriter
 {
 private:
    CHash256 ctx;
+    size_t nBytesHashed;

 public:
    int nType;
    int nVersion;

-    CHashWriter(int nTypeIn, int nVersionIn) : nType(nTypeIn), nVersion(nVersionIn) {}
+    CHashWriter(int nTypeIn, int nVersionIn) : nBytesHashed(0), nType(nTypeIn), nVersion(nVersionIn) {}

    CHashWriter& write(const char *pch, size_t size) {
        ctx.Write((const unsigned char*)pch, size);
+        nBytesHashed += size;
        return (*this);
    }

@@ -149,6 +151,9 @@ public:
        ctx.Finalize((unsigned char*)&result);
        return result;
    }
+    size_t GetNumBytesHashed() const {
+        return nBytesHashed;
+    }

    template<typename T>
    CHashWriter& operator<<(const T& obj) {
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -1183,7 +1183,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C

        // Check against previous transactions
        // This is done last to help prevent CPU exhaustion denial-of-service attacks.
-        if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true))
+        if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true, NULL))
            return false;

        // Check again against just the consensus-critical mandatory script
@@ -1195,7 +1195,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
        // There is a similar check in CreateNewBlock() to prevent creating
        // invalid blocks, however allowing such transactions into the mempool
        // can be exploited as a DoS attack.
-        if (!CheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true))
+        if (!CheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, NULL))
        {
            return error("%s: BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags %s, %s",
                __func__, hash.ToString(), FormatStateMessage(state));
@@ -1573,10 +1573,19 @@ void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCach
 }

 bool CScriptCheck::operator()() {
+    if (costTracker && !costTracker->IsWithinLimits())
+        return false; // Don't do any more checks if already past limits
+
    const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
-    if (!VerifyScript(scriptSig, scriptPubKey, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, cacheStore), &error)) {
+    CachingTransactionSignatureChecker checker(ptxTo, nIn, cacheStore);
+    if (!VerifyScript(scriptSig, scriptPubKey, nFlags, checker, &error)) {
        return false;
    }
+    if (costTracker) {
+        if (!costTracker->Update(ptxTo->GetHash(), checker.GetNumSigops(), checker.GetBytesHashed()))
+           return ::error("CScriptCheck(): %s:%d sigop and/or sighash byte limit exceeded",
+                           ptxTo->GetHash().ToString(), nIn);
+    }
    return true;
 }

@@ -1633,7 +1642,7 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins
 }
 }// namespace Consensus

-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheStore, std::vector<CScriptCheck> *pvChecks)
+bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheStore, ValidationCostTracker* costTracker, std::vector<CScriptCheck> *pvChecks)
 {
    if (!tx.IsCoinBase())
    {
@@ -1657,7 +1666,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
                assert(coins);

                // Verify signature
-                CScriptCheck check(*coins, tx, i, flags, cacheStore);
+                CScriptCheck check(costTracker, *coins, tx, i, flags, cacheStore);
                if (pvChecks) {
                    pvChecks->push_back(CScriptCheck());
                    check.swap(pvChecks->back());
@@ -1669,7 +1678,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
                        // arguments; if so, don't trigger DoS protection to
                        // avoid splitting the network between upgraded and
                        // non-upgraded nodes.
-                        CScriptCheck check2(*coins, tx, i,
+                        CScriptCheck check2(NULL, *coins, tx, i,
                                flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheStore);
                        if (check2())
                            return state.Invalid(false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
@@ -2077,11 +2086,16 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin

    CBlockUndo blockundo;

+    // Pre-fork, legacy sigop counting is used, unlimited resource tracker
+    // Post-fork, accurately counted sigop/sighash limits are used
+    ValidationCostTracker costTracker(MaxBlockSigops(block.nTime), MaxBlockSighash(block.nTime));
+
    CCheckQueueControl<CScriptCheck> control(fScriptChecks && nScriptCheckThreads ? &scriptcheckqueue : NULL);

    CAmount nFees = 0;
    int nInputs = 0;
-    unsigned int nSigOps = 0;
+    uint32_t nSigOps = 0;
+    uint32_t nMaxLegacySigops = MaxLegacySigops(block.nTime);
    CDiskTxPos pos(pindex->GetBlockPos(), GetSizeOfCompactSize(block.vtx.size()));
    std::vector<std::pair<uint256, CDiskTxPos> > vPos;
    vPos.reserve(block.vtx.size());
@@ -2092,7 +2106,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin

        nInputs += tx.vin.size();
        nSigOps += GetLegacySigOpCount(tx);
-        if (nSigOps > MAX_BLOCK_SIGOPS)
+        if (nSigOps > nMaxLegacySigops)
            return state.DoS(100, error("ConnectBlock(): too many sigops"),
                             REJECT_INVALID, "bad-blk-sigops");

@@ -2108,7 +2122,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
                // this is to prevent a "rogue miner" from creating
                // an incredibly-expensive-to-validate block.
                nSigOps += GetP2SHSigOpCount(tx, view);
-                if (nSigOps > MAX_BLOCK_SIGOPS)
+                if (nSigOps > nMaxLegacySigops)
                    return state.DoS(100, error("ConnectBlock(): too many sigops"),
                                     REJECT_INVALID, "bad-blk-sigops");
            }
@@ -2117,7 +2131,8 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin

            std::vector<CScriptCheck> vChecks;
            bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, nScriptCheckThreads ? &vChecks : NULL))
+            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults,
+                        &costTracker, nScriptCheckThreads ? &vChecks : NULL))
                return error("ConnectBlock(): CheckInputs on %s failed with %s",
                    tx.GetHash().ToString(), FormatStateMessage(state));
            control.Add(vChecks);
@@ -3027,7 +3042,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
    {
        nSigOps += GetLegacySigOpCount(tx);
    }
-    if (nSigOps > MAX_BLOCK_SIGOPS)
+    if (nSigOps > MaxLegacySigops(block.nTime))
        return state.DoS(100, error("CheckBlock(): out-of-bounds SigOpCount"),
                         REJECT_INVALID, "bad-blk-sigops");

@@ -5726,6 +5741,29 @@ unsigned int MaxBlockSize(uint32_t nBlockTime)
    return MAX_BLOCK_SIZE;
 }

+/** Maximum size of a block */
+unsigned int MaxBlockSigops(uint32_t nBlockTime)
+{
+    if (nBlockTime < sizeForkTime.load())
+        return std::numeric_limits<uint32_t>::max(); // Use old way of counting
+    return MAX_BLOCK_SIGOPS;
+}
+/** Maximum size of a block */
+unsigned int MaxBlockSighash(uint32_t nBlockTime)
+{
+    if (nBlockTime < sizeForkTime.load())
+        return std::numeric_limits<uint32_t>::max(); // no limit before
+    return MAX_BLOCK_SIGHASH;
+}
+
+/** Maximum legacy (miscounted) sigops in a block */
+uint32_t MaxLegacySigops(uint32_t nBlockTime)
+{
+    if (nBlockTime < sizeForkTime.load())
+        return MAX_BLOCK_SIGOPS;
+    return std::numeric_limits<uint32_t>::max(); // Use accurately-counted limit
+}
+


 class CMainCleanup
--- a/src/main.h
+++ b/src/main.h
@@ -26,8 +26,10 @@
 #include <utility>
 #include <vector>

+#include <boost/atomic.hpp>
 #include <boost/unordered_map.hpp>

+class ValidationCostTracker;
 class CBlockIndex;
 class CBlockTreeDB;
 class CBloomFilter;
@@ -330,7 +332,8 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& ma
 * instead of being performed inline.
 */
 bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &view, bool fScriptChecks,
-                 unsigned int flags, bool cacheStore, std::vector<CScriptCheck> *pvChecks = NULL);
+                 unsigned int flags, bool cacheStore, ValidationCostTracker* costTracker,
+                 std::vector<CScriptCheck> *pvChecks = NULL);

 /** Apply the effects of this transaction on the UTXO set represented by view */
 void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCache &inputs, int nHeight);
@@ -353,6 +356,44 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
 */
 bool CheckFinalTx(const CTransaction &tx, int flags = -1);

+/**
+ * Class that keeps track of number of signature operations
+ * and bytes hashed to compute signature hashes.
+ */
+class ValidationCostTracker
+{
+private:
+    mutable CCriticalSection cs;
+    uint32_t nSigops;
+    const uint32_t nMaxSigops;
+    uint32_t nSighashBytes;
+    const uint32_t nMaxSighashBytes;
+
+public:
+    ValidationCostTracker(uint32_t nMaxSigopsIn, uint32_t nMaxSighashBytesIn) :
+                                  nSigops(0), nMaxSigops(nMaxSigopsIn),
+                                  nSighashBytes(0), nMaxSighashBytes(nMaxSighashBytesIn) { }
+
+    bool IsWithinLimits() const {
+        LOCK(cs);
+        return (nSigops <= nMaxSigops && nSighashBytes <= nMaxSighashBytes);
+    }
+    bool Update(const uint256& txid, uint32_t nSigopsIn, uint32_t nSighashBytesIn) {
+        LOCK(cs);
+        nSigops += nSigopsIn;
+        nSighashBytes += nSighashBytesIn;
+        return (nSigops <= nMaxSigops && nSighashBytes <= nMaxSighashBytes);
+    }
+    uint32_t GetSigOps() const {
+        LOCK(cs);
+        return nSigops;
+    }
+    uint32_t GetSighashBytes() const {
+        LOCK(cs);
+        return nSighashBytes;
+    }
+};
+
 /** 
 * Closure representing one script verification
 * Note that this stores references to the spending transaction 
@@ -360,6 +401,7 @@ bool CheckFinalTx(const CTransaction &tx, int flags = -1);
 class CScriptCheck
 {
 private:
+    ValidationCostTracker* costTracker;
    CScript scriptPubKey;
    const CTransaction *ptxTo;
    unsigned int nIn;
@@ -368,14 +410,15 @@ private:
    ScriptError error;

 public:
-    CScriptCheck(): ptxTo(0), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {}
-    CScriptCheck(const CCoins& txFromIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn) :
-        scriptPubKey(txFromIn.vout[txToIn.vin[nInIn].prevout.n].scriptPubKey),
+    CScriptCheck(): costTracker(NULL), ptxTo(0), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {}
+    CScriptCheck(ValidationCostTracker* costTrackerIn, const CCoins& txFromIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn) :
+        costTracker(costTrackerIn), scriptPubKey(txFromIn.vout[txToIn.vin[nInIn].prevout.n].scriptPubKey),
        ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR) { }

    bool operator()();

    void swap(CScriptCheck &check) {
+        std::swap(costTracker, check.costTracker);
        scriptPubKey.swap(check.scriptPubKey);
        std::swap(ptxTo, check.ptxTo);
        std::swap(nIn, check.nIn);
@@ -517,4 +560,13 @@ static const unsigned int REJECT_CONFLICT = 0x102;
 /** Maximum size of a block */
 unsigned int MaxBlockSize(uint32_t nBlockTime);

+/** Max accurately-counted sigops in a block */
+uint32_t MaxBlockSigops(uint32_t nBlockTime);
+
+/** Max accurately-counted bytes hashed to compute signatures, per block */
+uint32_t MaxBlockSighash(uint32_t nBlockTime);
+
+/** Maximum number of legacy sigops in a block */
+uint32_t MaxLegacySigops(uint32_t nBlockTime);
+
 #endif // BITCOIN_MAIN_H
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -79,6 +79,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
    if(!pblocktemplate.get())
        return NULL;
    CBlock *pblock = &pblocktemplate->block; // pointer for convenience
+    ValidationCostTracker resourceTracker(std::numeric_limits<size_t>::max(), std::numeric_limits<size_t>::max());

    // Create coinbase tx
    CMutableTransaction txNew;
@@ -169,6 +170,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s

        CTxMemPool::indexed_transaction_set::nth_index<3>::type::iterator mi = mempool.mapTx.get<3>().begin();
        CTxMemPool::txiter iter;
+        uint32_t nMaxLegacySigops = MaxLegacySigops(pblock->nTime);

        while (mi != mempool.mapTx.get<3>().end() || !clearedTxs.empty())
        {
@@ -236,8 +238,8 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
                continue;

            unsigned int nTxSigOps = iter->GetSigOpCount();
-            if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) {
-                if (nBlockSigOps > MAX_BLOCK_SIGOPS - 2) {
+            if (nBlockSigOps + nTxSigOps >= nMaxLegacySigops) {
+                if (nBlockSigOps > nMaxLegacySigops - 2) {
                    break;
                }
                continue;
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1069,7 +1069,7 @@ public:

 } // anon namespace

-uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType)
+uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, size_t* nHashedOut)
 {
    static const uint256 one(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));
    if (nIn >= txTo.vin.size()) {
@@ -1091,6 +1091,8 @@ uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsig
    // Serialize and hash
    CHashWriter ss(SER_GETHASH, 0);
    ss << txTmp << nHashType;
+    if (nHashedOut != NULL)
+        *nHashedOut = ss.GetNumBytesHashed();
    return ss.GetHash();
 }

@@ -1099,7 +1101,8 @@ bool TransactionSignatureChecker::VerifySignature(const std::vector<unsigned cha
    return pubkey.Verify(sighash, vchSig);
 }

-bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn, const vector<unsigned char>& vchPubKey, const CScript& scriptCode) const
+bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn, const vector<unsigned char>& vchPubKey,
+                                           const CScript& scriptCode) const
 {
    CPubKey pubkey(vchPubKey);
    if (!pubkey.IsValid())
@@ -1112,7 +1115,10 @@ bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn
    int nHashType = vchSig.back();
    vchSig.pop_back();

-    uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType);
+    size_t nHashed = 0;
+    uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, &nHashed);
+    nBytesHashed += nHashed;
+    ++nSigops;

    if (!VerifySignature(vchSig, pubkey, sighash))
        return false;
--- a/src/script/interpreter.h
+++ b/src/script/interpreter.h
@@ -85,7 +85,7 @@ enum

 bool CheckSignatureEncoding(const std::vector<unsigned char> &vchSig, unsigned int flags, ScriptError* serror);

-uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType);
+uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, size_t* nHashedOut=NULL);

 class BaseSignatureChecker
 {
@@ -108,14 +108,18 @@ class TransactionSignatureChecker : public BaseSignatureChecker
 private:
    const CTransaction* txTo;
    unsigned int nIn;
+    mutable size_t nBytesHashed;
+    mutable size_t nSigops;

 protected:
    virtual bool VerifySignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const;

 public:
-    TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn) : txTo(txToIn), nIn(nInIn) {}
+    TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn) : txTo(txToIn), nIn(nInIn), nBytesHashed(0), nSigops(0) {}
    bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode) const;
    bool CheckLockTime(const CScriptNum& nLockTime) const;
+    size_t GetBytesHashed() const { return nBytesHashed; }
+    size_t GetNumSigops() const { return nSigops; }
 };

 class MutableTransactionSignatureChecker : public TransactionSignatureChecker
@@ -127,7 +131,9 @@ public:
    MutableTransactionSignatureChecker(const CMutableTransaction* txToIn, unsigned int nInIn) : TransactionSignatureChecker(&txTo, nInIn), txTo(*txToIn) {}
 };

-bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* error = NULL);
-bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* error = NULL);
+bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags,
+                const BaseSignatureChecker& checker, ScriptError* error = NULL);
+bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags,
+                  const BaseSignatureChecker& checker, ScriptError* error = NULL);

 #endif // BITCOIN_SCRIPT_INTERPRETER_H
--- a/src/test/script_P2SH_tests.cpp
+++ b/src/test/script_P2SH_tests.cpp
@@ -116,7 +116,7 @@ BOOST_AUTO_TEST_CASE(sign)
        {
            CScript sigSave = txTo[i].vin[0].scriptSig;
            txTo[i].vin[0].scriptSig = txTo[j].vin[0].scriptSig;
-            bool sigOK = CScriptCheck(CCoins(txFrom, 0), txTo[i], 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false)();
+            bool sigOK = CScriptCheck(NULL, CCoins(txFrom, 0), txTo[i], 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false)();
            if (i == j)
                BOOST_CHECK_MESSAGE(sigOK, strprintf("VerifySignature %d %d", i, j));
            else