From c74332c67806ed92e6e18de174671a7c30608780 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Thu, 28 Aug 2014 13:23:24 -0400 Subject: [PATCH 1/2] Stricter handling of orphan transactions Prevent denial-of-service attacks by banning peers that send us invalid orphan transactions and only storing orphan transactions given to us by a peer while the peer is connected. --- src/main.cpp | 65 +++++++++++++++++++++++++++++++++--------- src/test/DoS_tests.cpp | 17 ++++++++--- 2 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 27100d62c..6a2e6ac65 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -63,8 +63,13 @@ struct COrphanBlock { map mapOrphanBlocks; multimap mapOrphanBlocksByPrev; -map mapOrphanTransactions; +struct COrphanTx { + CTransaction tx; + NodeId fromPeer; +}; +map mapOrphanTransactions; map > mapOrphanTransactionsByPrev; +void EraseOrphansFor(NodeId peer); // Constant stuff for coinbase transactions we create: CScript COINBASE_FLAGS; @@ -264,6 +269,7 @@ void FinalizeNode(NodeId nodeid) { mapBlocksInFlight.erase(entry.hash); BOOST_FOREACH(const uint256& hash, state->vBlocksToDownload) mapBlocksToDownload.erase(hash); + EraseOrphansFor(nodeid); mapNodeState.erase(nodeid); } @@ -461,7 +467,7 @@ CBlockTreeDB *pblocktree = NULL; // mapOrphanTransactions // -bool AddOrphanTx(const CTransaction& tx) +bool AddOrphanTx(const CTransaction& tx, NodeId peer) { uint256 hash = tx.GetHash(); if (mapOrphanTransactions.count(hash)) @@ -481,21 +487,22 @@ bool AddOrphanTx(const CTransaction& tx) return false; } - mapOrphanTransactions[hash] = tx; + mapOrphanTransactions[hash].tx = tx; + mapOrphanTransactions[hash].fromPeer = peer; BOOST_FOREACH(const CTxIn& txin, tx.vin) mapOrphanTransactionsByPrev[txin.prevout.hash].insert(hash); - LogPrint("mempool", "stored orphan tx %s (mapsz %u)\n", hash.ToString(), - mapOrphanTransactions.size()); + LogPrint("mempool", "stored orphan tx %s (mapsz %u prevsz %u)\n", hash.ToString(), + mapOrphanTransactions.size(), mapOrphanTransactionsByPrev.size()); return true; } void static EraseOrphanTx(uint256 hash) { - map::iterator it = mapOrphanTransactions.find(hash); + map::iterator it = mapOrphanTransactions.find(hash); if (it == mapOrphanTransactions.end()) return; - BOOST_FOREACH(const CTxIn& txin, it->second.vin) + BOOST_FOREACH(const CTxIn& txin, it->second.tx.vin) { map >::iterator itPrev = mapOrphanTransactionsByPrev.find(txin.prevout.hash); if (itPrev == mapOrphanTransactionsByPrev.end()) @@ -507,6 +514,23 @@ void static EraseOrphanTx(uint256 hash) mapOrphanTransactions.erase(it); } +void EraseOrphansFor(NodeId peer) +{ + int nErased = 0; + map::iterator iter = mapOrphanTransactions.begin(); + while (iter != mapOrphanTransactions.end()) + { + map::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid + if (maybeErase->second.fromPeer == peer) + { + EraseOrphanTx(maybeErase->second.tx.GetHash()); + ++nErased; + } + } + if (nErased > 0) LogPrint("mempool", "Erased %d orphan tx from peer %d\n", nErased, peer); +} + + unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) { unsigned int nEvicted = 0; @@ -514,7 +538,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) { // Evict a random orphan: uint256 randomhash = GetRandHash(); - map::iterator it = mapOrphanTransactions.lower_bound(randomhash); + map::iterator it = mapOrphanTransactions.lower_bound(randomhash); if (it == mapOrphanTransactions.end()) it = mapOrphanTransactions.begin(); EraseOrphanTx(it->first); @@ -3777,6 +3801,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, mempool.mapTx.size()); // Recursively process any orphan transactions that depended on this one + set setMisbehaving; for (unsigned int i = 0; i < vWorkQueue.size(); i++) { map >::iterator itByPrev = mapOrphanTransactionsByPrev.find(vWorkQueue[i]); @@ -3787,25 +3812,36 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, ++mi) { const uint256& orphanHash = *mi; - const CTransaction& orphanTx = mapOrphanTransactions[orphanHash]; + const CTransaction& orphanTx = mapOrphanTransactions[orphanHash].tx; + NodeId fromPeer = mapOrphanTransactions[orphanHash].fromPeer; bool fMissingInputs2 = false; // Use a dummy CValidationState so someone can't setup nodes to counter-DoS based on orphan // resolution (that is, feeding people an invalid transaction based on LegitTxX in order to get // anyone relaying LegitTxX banned) CValidationState stateDummy; + vEraseQueue.push_back(orphanHash); + + if (setMisbehaving.count(fromPeer)) + continue; if (AcceptToMemoryPool(mempool, stateDummy, orphanTx, true, &fMissingInputs2)) { LogPrint("mempool", " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanTx); mapAlreadyAskedFor.erase(CInv(MSG_TX, orphanHash)); vWorkQueue.push_back(orphanHash); - vEraseQueue.push_back(orphanHash); } else if (!fMissingInputs2) { - // invalid or too-little-fee orphan - vEraseQueue.push_back(orphanHash); + int nDos = 0; + if (stateDummy.IsInvalid(nDos) && nDos > 0) + { + // Punish peer that gave us an invalid orphan tx + Misbehaving(fromPeer, nDos); + setMisbehaving.insert(fromPeer); + LogPrint("mempool", " invalid orphan tx %s\n", orphanHash.ToString()); + } + // too-little-fee orphan LogPrint("mempool", " removed orphan tx %s\n", orphanHash.ToString()); } mempool.check(pcoinsTip); @@ -3817,7 +3853,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } else if (fMissingInputs) { - AddOrphanTx(tx); + AddOrphanTx(tx, pfrom->GetId()); // DoS prevention: do not allow mapOrphanTransactions to grow unbounded unsigned int nEvicted = LimitOrphanTxSize(MAX_ORPHAN_TRANSACTIONS); @@ -4324,7 +4360,9 @@ bool SendMessages(CNode* pto, bool fSendTrickle) if (pto->addr.IsLocal()) LogPrintf("Warning: not banning local peer %s!\n", pto->addr.ToString()); else + { CNode::Ban(pto->addr); + } } state.fShouldBan = false; } @@ -4538,5 +4576,6 @@ public: // orphan transactions mapOrphanTransactions.clear(); + mapOrphanTransactionsByPrev.clear(); } } instance_of_cmaincleanup; diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index fa4edff63..e01967481 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -24,7 +24,8 @@ #include // Tests this internal-to-main.cpp method: -extern bool AddOrphanTx(const CTransaction& tx); +extern bool AddOrphanTx(const CTransaction& tx, NodeId peer); +extern void EraseOrphansFor(NodeId peer); extern unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans); extern std::map mapOrphanTransactions; extern std::map > mapOrphanTransactionsByPrev; @@ -174,7 +175,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].nValue = 1*CENT; tx.vout[0].scriptPubKey.SetDestination(key.GetPubKey().GetID()); - AddOrphanTx(tx); + AddOrphanTx(tx, i); } // ... and 50 that depend on other orphans: @@ -191,7 +192,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].scriptPubKey.SetDestination(key.GetPubKey().GetID()); SignSignature(keystore, txPrev, tx, 0); - AddOrphanTx(tx); + AddOrphanTx(tx, i); } // This really-big orphan should be ignored: @@ -215,7 +216,15 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) for (unsigned int j = 1; j < tx.vin.size(); j++) tx.vin[j].scriptSig = tx.vin[0].scriptSig; - BOOST_CHECK(!AddOrphanTx(tx)); + BOOST_CHECK(!AddOrphanTx(tx, i)); + } + + // Test EraseOrphansFor: + for (NodeId i = 0; i < 3; i++) + { + size_t sizeBefore = mapOrphanTransactions.size(); + EraseOrphansFor(i); + BOOST_CHECK(mapOrphanTransactions.size() < sizeBefore); } // Test LimitOrphanTxSize() function: From aa3c697e90c02d5797a59a7bfb1ecac6fbd918cf Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Wed, 10 Sep 2014 14:08:03 -0400 Subject: [PATCH 2/2] Store fewer orphan tx by default, add -maxorphantx option There is no reason to store thousands of orphan transactions; normally an orphan's parents will either be broadcast or mined reasonably quickly. This pull drops the maximum number of orphans from 10,000 down to 100, and adds a command-line option (-maxorphantx) that is just like -maxorphanblocks to override the default. --- src/init.cpp | 1 + src/main.cpp | 3 ++- src/main.h | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 31f64878f..8f336c31f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -226,6 +226,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += " -dbcache= " + strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache) + "\n"; strUsage += " -loadblock= " + _("Imports blocks from external blk000??.dat file") + " " + _("on startup") + "\n"; strUsage += " -maxorphanblocks= " + strprintf(_("Keep at most unconnectable blocks in memory (default: %u)"), DEFAULT_MAX_ORPHAN_BLOCKS) + "\n"; + strUsage += " -maxorphantx= " + strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS) + "\n"; strUsage += " -par= " + strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"), -(int)boost::thread::hardware_concurrency(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS) + "\n"; strUsage += " -pid= " + _("Specify pid file (default: bitcoind.pid)") + "\n"; strUsage += " -reindex " + _("Rebuild block chain index from current blk000??.dat files") + " " + _("on startup") + "\n"; diff --git a/src/main.cpp b/src/main.cpp index 6a2e6ac65..f97825496 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3856,7 +3856,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, AddOrphanTx(tx, pfrom->GetId()); // DoS prevention: do not allow mapOrphanTransactions to grow unbounded - unsigned int nEvicted = LimitOrphanTxSize(MAX_ORPHAN_TRANSACTIONS); + unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)); + unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx); if (nEvicted > 0) LogPrint("mempool", "mapOrphan overflow, removed %u tx\n", nEvicted); } else if (pfrom->fWhitelisted) { diff --git a/src/main.h b/src/main.h index 30cccab2f..d340fd0b6 100644 --- a/src/main.h +++ b/src/main.h @@ -51,8 +51,8 @@ static const unsigned int MAX_BLOCK_SIGOPS = MAX_BLOCK_SIZE/50; static const unsigned int MAX_P2SH_SIGOPS = 15; /** The maximum number of sigops we're willing to relay/mine in a single tx */ static const unsigned int MAX_TX_SIGOPS = MAX_BLOCK_SIGOPS/5; -/** The maximum number of orphan transactions kept in memory */ -static const unsigned int MAX_ORPHAN_TRANSACTIONS = MAX_BLOCK_SIZE/100; +/** Default for -maxorphantx, maximum number of orphan transactions kept in memory */ +static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100; /** Default for -maxorphanblocks, maximum number of orphan blocks kept in memory */ static const unsigned int DEFAULT_MAX_ORPHAN_BLOCKS = 750; /** The maximum size of a blk?????.dat file (since 0.8) */