From c74332c67806ed92e6e18de174671a7c30608780 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Thu, 28 Aug 2014 13:23:24 -0400 Subject: [PATCH] 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: