Merge pull request #773 from gavinandresen/p2shSigOpCount

Simplify counting of P2SH sigops to match BIP 16

4 files changed, 53 insertions, 52 deletions

diff --git a/src/main.cpp b/src/main.cpp
index e242fa640d..1dbd50f4e6 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -503,20 +503,17 @@ bool CTransaction::AcceptToMemoryPool(CTxDB& txdb, bool fCheckInputs, bool* pfMi
         if (!AreInputsStandard(mapInputs) && !fTestNet)
             return error("AcceptToMemoryPool() : nonstandard transaction input");
 
+        // Note: if you modify this code to accept non-standard transactions, then
+        // you should add code here to check that the transaction does a
+        // reasonable number of ECDSA signature verifications.
+
         int64 nFees = GetValueIn(mapInputs)-GetValueOut();
-        int nSigOps = GetSigOpCount(mapInputs);
         unsigned int nSize = ::GetSerializeSize(*this, SER_NETWORK);
 
         // Don't accept it if it can't get into a block
         if (nFees < GetMinFee(1000, true, GMF_RELAY))
             return error("AcceptToMemoryPool() : not enough fees");
 
-        // Checking ECDSA signatures is a CPU bottleneck, so to avoid denial-of-service
-        // attacks disallow transactions with more than one SigOp per 65 bytes.
-        // 65 bytes because that is the minimum size of an ECDSA signature
-        if (nSigOps > nSize / 65 || nSize < 100)
-            return error("AcceptToMemoryPool() : transaction with out-of-bounds SigOpCount");
-
         // Continuously rate-limit free transactions
         // This mitigates 'penny-flooding' -- sending thousands of free transactions just to
         // be annoying or make other's transactions take longer to confirm.
@@ -1022,7 +1019,7 @@ int64 CTransaction::GetValueIn(const MapPrevTx& inputs) const
 
 }
 
-int CTransaction::GetSigOpCount(const MapPrevTx& inputs) const
+int CTransaction::GetP2SHSigOpCount(const MapPrevTx& inputs) const
 {
     if (IsCoinBase())
         return 0;
@@ -1030,14 +1027,16 @@ int CTransaction::GetSigOpCount(const MapPrevTx& inputs) const
     int nSigOps = 0;
     for (int i = 0; i < vin.size(); i++)
     {
-        nSigOps += GetOutputFor(vin[i], inputs).scriptPubKey.GetSigOpCount(vin[i].scriptSig);
+        const CTxOut& prevout = GetOutputFor(vin[i], inputs);
+        if (prevout.scriptPubKey.IsPayToScriptHash())
+            nSigOps += prevout.scriptPubKey.GetSigOpCount(vin[i].scriptSig);
     }
     return nSigOps;
 }
 
 bool CTransaction::ConnectInputs(MapPrevTx inputs,
                                  map<uint256, CTxIndex>& mapTestPool, const CDiskTxPos& posThisTx,
-                                 const CBlockIndex* pindexBlock, bool fBlock, bool fMiner)
+                                 const CBlockIndex* pindexBlock, bool fBlock, bool fMiner, bool fStrictPayToScriptHash)
 {
     // Take over previous transactions' spent pointers
     // fBlock is true when this is called from AcceptBlock when a new best-block is added to the blockchain
@@ -1074,20 +1073,6 @@ bool CTransaction::ConnectInputs(MapPrevTx inputs,
             if (!MoneyRange(txPrev.vout[prevout.n].nValue) || !MoneyRange(nValueIn))
                 return DoS(100, error("ConnectInputs() : txin values out of range"));
 
-            bool fStrictPayToScriptHash = true;
-            if (fBlock)
-            {
-                // To avoid being on the short end of a block-chain split,
-                // don't do secondary validation of pay-to-script-hash transactions
-                // until blocks with timestamps after paytoscripthashtime:
-                int64 nEvalSwitchTime = GetArg("paytoscripthashtime", 1329264000); // Feb 15, 2012
-                fStrictPayToScriptHash = (pindexBlock->nTime >= nEvalSwitchTime);
-            }
-            // if !fBlock, then always be strict-- don't accept
-            // invalid-under-new-rules pay-to-script-hash transactions into
-            // our memory pool (don't relay them, don't include them
-            // in blocks we mine).
-
             // Skip ECDSA signature verification when connecting blocks (fBlock=true)
             // before the last blockchain checkpoint. This is safe because block merkle hashes are
             // still computed and checked, and any change will be caught at the next checkpoint.
@@ -1198,6 +1183,12 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex)
     if (!CheckBlock())
         return false;
 
+    // To avoid being on the short end of a block-chain split,
+    // don't do secondary validation of pay-to-script-hash transactions
+    // until blocks with timestamps after paytoscripthashtime:
+    int64 nEvalSwitchTime = GetArg("-paytoscripthashtime", 1329264000); // Feb 15, 2012
+    bool fStrictPayToScriptHash = (pindex->nTime >= nEvalSwitchTime);
+
     //// issue here: it doesn't know the version
     unsigned int nTxPos = pindex->nBlockPos + ::GetSerializeSize(CBlock(), SER_DISK) - 1 + GetSizeOfCompactSize(vtx.size());
 
@@ -1206,6 +1197,10 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex)
     int nSigOps = 0;
     BOOST_FOREACH(CTransaction& tx, vtx)
     {
+        nSigOps += tx.GetLegacySigOpCount();
+        if (nSigOps > MAX_BLOCK_SIGOPS)
+            return DoS(100, error("ConnectBlock() : too many sigops"));
+
         CDiskTxPos posThisTx(pindex->nFile, pindex->nBlockPos, nTxPos);
         nTxPos += ::GetSerializeSize(tx, SER_DISK);
 
@@ -1216,17 +1211,19 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex)
             if (!tx.FetchInputs(txdb, mapQueuedChanges, true, false, mapInputs, fInvalid))
                 return false;
 
-            int nTxOps = tx.GetSigOpCount(mapInputs);
-            nSigOps += nTxOps;
-            if (nSigOps > MAX_BLOCK_SIGOPS)
-                return DoS(100, error("ConnectBlock() : too many sigops"));
-            // There is a different MAX_BLOCK_SIGOPS check in AcceptBlock();
-            // a block must satisfy both to make it into the best-chain
-            // (AcceptBlock() is always called before ConnectBlock())
+            if (fStrictPayToScriptHash)
+            {
+                // Add in sigops done by pay-to-script-hash inputs;
+                // this is to prevent a "rogue miner" from creating
+                // an incredibly-expensive-to-validate block.
+                nSigOps += tx.GetP2SHSigOpCount(mapInputs);
+                if (nSigOps > MAX_BLOCK_SIGOPS)
+                    return DoS(100, error("ConnectBlock() : too many sigops"));
+            }
 
             nFees += tx.GetValueIn(mapInputs)-tx.GetValueOut();
 
-            if (!tx.ConnectInputs(mapInputs, mapQueuedChanges, posThisTx, pindex, true, false))
+            if (!tx.ConnectInputs(mapInputs, mapQueuedChanges, posThisTx, pindex, true, false, fStrictPayToScriptHash))
                 return false;
         }
 
@@ -1510,9 +1507,6 @@ bool CBlock::CheckBlock() const
         if (!tx.CheckTransaction())
             return DoS(tx.nDoS, error("CheckBlock() : CheckTransaction failed"));
 
-    // Pre-pay-to-script-hash (before version 0.6), this is how sigops
-    // were counted; there is another check in ConnectBlock when
-    // transaction inputs are fetched to count pay-to-script-hash sigops:
     int nSigOps = 0;
     BOOST_FOREACH(const CTransaction& tx, vtx)
     {
@@ -3046,8 +3040,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
         map<uint256, CTxIndex> mapTestPool;
         uint64 nBlockSize = 1000;
         uint64 nBlockTx = 0;
-        int nBlockSigOps1 = 100; // pre-0.6 count of sigOps
-        int nBlockSigOps2 = 100; // post-0.6 count of sigOps
+        int nBlockSigOps = 100;
         while (!mapPriority.empty())
         {
             // Take highest priority transaction off priority queue
@@ -3061,8 +3054,8 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
                 continue;
 
             // Legacy limits on sigOps:
-            int nTxSigOps1 = tx.GetLegacySigOpCount();
-            if (nBlockSigOps1 + nTxSigOps1 >= MAX_BLOCK_SIGOPS)
+            int nTxSigOps = tx.GetLegacySigOpCount();
+            if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS)
                 continue;
 
             // Transaction fee required depends on block size
@@ -3081,8 +3074,8 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
             if (nFees < nMinFee)
                 continue;
 
-            int nTxSigOps2 = tx.GetSigOpCount(mapInputs);
-            if (nBlockSigOps2 + nTxSigOps2 >= MAX_BLOCK_SIGOPS)
+            nTxSigOps += tx.GetP2SHSigOpCount(mapInputs);
+            if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS)
                 continue;
 
             if (!tx.ConnectInputs(mapInputs, mapTestPoolTmp, CDiskTxPos(1,1,1), pindexPrev, false, true))
@@ -3094,8 +3087,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
             pblock->vtx.push_back(tx);
             nBlockSize += nTxSize;
             ++nBlockTx;
-            nBlockSigOps1 += nTxSigOps1;
-            nBlockSigOps2 += nTxSigOps2;
+            nBlockSigOps += nTxSigOps;
 
             // Add transactions that depend on this one to the priority queue
             uint256 hash = tx.GetHash();
diff --git a/src/main.h b/src/main.h
index ec5623d17d..124b228ec9 100644
--- a/src/main.h
+++ b/src/main.h
@@ -528,14 +528,13 @@ public:
     */
     int GetLegacySigOpCount() const;
 
-    /** Count ECDSA signature operations the new (0.6-and-later) way
-        This is a better measure of how expensive it is to process this transaction.
+    /** Count ECDSA signature operations in pay-to-script-hash inputs.
 
         @param[in] mapInputs	Map of previous transactions that have outputs we're spending
         @return maximum number of sigops required to validate this transaction's inputs
         @see CTransaction::FetchInputs
      */
-    int GetSigOpCount(const MapPrevTx& mapInputs) const;
+    int GetP2SHSigOpCount(const MapPrevTx& mapInputs) const;
 
     /** Amount of bitcoins spent by this transaction.
         @return sum of all outputs (note: does not include fees)
@@ -699,11 +698,12 @@ public:
         @param[in] pindexBlock
         @param[in] fBlock	true if called from ConnectBlock
         @param[in] fMiner	true if called from CreateNewBlock
+        @param[in] fStrictPayToScriptHash	true if fully validating p2sh transactions
         @return Returns true if all checks succeed
      */
     bool ConnectInputs(MapPrevTx inputs,
                        std::map<uint256, CTxIndex>& mapTestPool, const CDiskTxPos& posThisTx,
-                       const CBlockIndex* pindexBlock, bool fBlock, bool fMiner);
+                       const CBlockIndex* pindexBlock, bool fBlock, bool fMiner, bool fStrictPayToScriptHash=true);
     bool ClientConnectInputs();
     bool CheckTransaction() const;
     bool AcceptToMemoryPool(CTxDB& txdb, bool fCheckInputs=true, bool* pfMissingInputs=NULL);
diff --git a/src/test/script_P2SH_tests.cpp b/src/test/script_P2SH_tests.cpp
index aed3e23319..c782e0c6c1 100644
--- a/src/test/script_P2SH_tests.cpp
+++ b/src/test/script_P2SH_tests.cpp
@@ -255,7 +255,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
     }
 
     CTransaction txFrom;
-    txFrom.vout.resize(5);
+    txFrom.vout.resize(6);
 
     // First three are standard:
     CScript pay1; pay1.SetBitcoinAddress(key[0].GetPubKey());
@@ -267,12 +267,18 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
     txFrom.vout[1].scriptPubKey = pay1;
     txFrom.vout[2].scriptPubKey = pay1of3;
 
-    // Last two non-standard:
+    // Last three non-standard:
     CScript empty;
     keystore.AddCScript(empty);
     txFrom.vout[3].scriptPubKey = empty;
     // Can't use SetPayToScriptHash, it checks for the empty Script. So:
     txFrom.vout[4].scriptPubKey << OP_HASH160 << Hash160(empty) << OP_EQUAL;
+    CScript oneOfEleven;
+    oneOfEleven << OP_1;
+    for (int i = 0; i < 11; i++)
+        oneOfEleven << key[0].GetPubKey();
+    oneOfEleven << OP_11 << OP_CHECKMULTISIG;
+    txFrom.vout[5].scriptPubKey.SetPayToScriptHash(oneOfEleven);
 
     mapInputs[txFrom.GetHash()] = make_pair(CTxIndex(), txFrom);
 
@@ -292,16 +298,21 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
     BOOST_CHECK(SignSignature(keystore, txFrom, txTo, 2));
 
     BOOST_CHECK(txTo.AreInputsStandard(mapInputs));
+    BOOST_CHECK_EQUAL(txTo.GetP2SHSigOpCount(mapInputs), 1);
 
     CTransaction txToNonStd;
     txToNonStd.vout.resize(1);
     txToNonStd.vout[0].scriptPubKey.SetBitcoinAddress(key[1].GetPubKey());
-    txToNonStd.vin.resize(1);
+    txToNonStd.vin.resize(2);
     txToNonStd.vin[0].prevout.n = 4;
     txToNonStd.vin[0].prevout.hash = txFrom.GetHash();
     txToNonStd.vin[0].scriptSig << Serialize(empty);
+    txToNonStd.vin[1].prevout.n = 5;
+    txToNonStd.vin[1].prevout.hash = txFrom.GetHash();
+    txToNonStd.vin[1].scriptSig << OP_0 << Serialize(oneOfEleven);
 
     BOOST_CHECK(!txToNonStd.AreInputsStandard(mapInputs));
+    BOOST_CHECK_EQUAL(txToNonStd.GetP2SHSigOpCount(mapInputs), 11);
 
     txToNonStd.vin[0].scriptSig.clear();
     BOOST_CHECK(!txToNonStd.AreInputsStandard(mapInputs));
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index 33765ca966..592fe3f81a 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -78,7 +78,6 @@ BOOST_AUTO_TEST_CASE(test_Get)
     t1.vout[0].scriptPubKey << OP_1;
 
     BOOST_CHECK(t1.AreInputsStandard(dummyInputs));
-    BOOST_CHECK_EQUAL(t1.GetSigOpCount(dummyInputs), 3);
     BOOST_CHECK_EQUAL(t1.GetValueIn(dummyInputs), (50+21+22)*CENT);
 }
 
@@ -103,7 +102,6 @@ BOOST_AUTO_TEST_CASE(test_GetThrow)
     t1.vout[0].scriptPubKey << OP_1;
 
     BOOST_CHECK_THROW(t1.AreInputsStandard(missingInputs), runtime_error);
-    BOOST_CHECK_THROW(t1.GetSigOpCount(missingInputs), runtime_error);
     BOOST_CHECK_THROW(t1.GetValueIn(missingInputs), runtime_error);
 }