From 137d0685a45d4a02f5773652130704ad135e63f7 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Fri, 20 Jan 2012 17:07:40 -0500 Subject: [PATCH] Simplify counting of P2SH sigops to match BIP 16 (thanks to Matt Corallo for prompting this). This also removes an un-needed sigops-per-byte check when accepting transactions to the memory pool (un-needed assuming only standard transactions are being accepted). And it only counts P2SH sigops after the switchover date. --- src/main.cpp | 78 +++++++++++++++------------------- src/main.h | 8 ++-- src/test/script_P2SH_tests.cpp | 17 ++++++-- src/test/transaction_tests.cpp | 2 - 4 files changed, 53 insertions(+), 52 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 891dbed98..18d3c2f0e 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)) 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. @@ -1013,7 +1010,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; @@ -1021,14 +1018,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& 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 @@ -1065,20 +1064,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. @@ -1189,6 +1174,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()); @@ -1197,6 +1188,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); @@ -1206,17 +1201,19 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex) if (!tx.FetchInputs(txdb, mapQueuedChanges, true, false, mapInputs)) 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; } @@ -1500,9 +1497,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) { @@ -3036,8 +3030,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey) map 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 @@ -3051,8 +3044,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 @@ -3070,8 +3063,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)) @@ -3083,8 +3076,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 be5f2f58a..6db9fc805 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) @@ -698,11 +697,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& 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 aed3e2331..c782e0c6c 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 33765ca96..592fe3f81 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); }