diff --git a/src/main.cpp b/src/main.cpp index 8785a1e99..af1f3eae7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -58,8 +58,8 @@ CMedianFilter cPeerBlockCounts(8, 0); // Amount of blocks that other nodes map mapOrphanBlocks; multimap mapOrphanBlocksByPrev; -map mapOrphanTransactions; -map > mapOrphanTransactionsByPrev; +map mapOrphanTransactions; +map > mapOrphanTransactionsByPrev; // Constant stuff for coinbase transactions we create: CScript COINBASE_FLAGS; @@ -283,16 +283,12 @@ CBlockTreeDB *pblocktree = NULL; // mapOrphanTransactions // -bool AddOrphanTx(const CDataStream& vMsg) +bool AddOrphanTx(const CTransaction& tx) { - CTransaction tx; - CDataStream(vMsg) >> tx; uint256 hash = tx.GetHash(); if (mapOrphanTransactions.count(hash)) return false; - CDataStream* pvMsg = new CDataStream(vMsg); - // Ignore big transactions, to avoid a // send-big-orphans memory exhaustion attack. If a peer has a legitimate // large transaction with a missing parent then we assume @@ -300,16 +296,16 @@ bool AddOrphanTx(const CDataStream& vMsg) // have been mined or received. // 10,000 orphans, each of which is at most 5,000 bytes big is // at most 500 megabytes of orphans: - if (pvMsg->size() > 5000) + unsigned int sz = tx.GetSerializeSize(SER_NETWORK, CTransaction::CURRENT_VERSION); + if (sz > 5000) { - printf("ignoring large orphan tx (size: %"PRIszu", hash: %s)\n", pvMsg->size(), hash.ToString().c_str()); - delete pvMsg; + printf("ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString().c_str()); return false; } - mapOrphanTransactions[hash] = pvMsg; + mapOrphanTransactions[hash] = tx; BOOST_FOREACH(const CTxIn& txin, tx.vin) - mapOrphanTransactionsByPrev[txin.prevout.hash].insert(make_pair(hash, pvMsg)); + mapOrphanTransactionsByPrev[txin.prevout.hash].insert(hash); printf("stored orphan tx %s (mapsz %"PRIszu")\n", hash.ToString().c_str(), mapOrphanTransactions.size()); @@ -320,16 +316,13 @@ void static EraseOrphanTx(uint256 hash) { if (!mapOrphanTransactions.count(hash)) return; - const CDataStream* pvMsg = mapOrphanTransactions[hash]; - CTransaction tx; - CDataStream(*pvMsg) >> tx; + const CTransaction& tx = mapOrphanTransactions[hash]; BOOST_FOREACH(const CTxIn& txin, tx.vin) { mapOrphanTransactionsByPrev[txin.prevout.hash].erase(hash); if (mapOrphanTransactionsByPrev[txin.prevout.hash].empty()) mapOrphanTransactionsByPrev.erase(txin.prevout.hash); } - delete pvMsg; mapOrphanTransactions.erase(hash); } @@ -340,7 +333,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); @@ -832,7 +825,7 @@ bool CTransaction::AcceptToMemoryPool(CValidationState &state, bool fCheckInputs } } -bool CTxMemPool::addUnchecked(const uint256& hash, CTransaction &tx) +bool CTxMemPool::addUnchecked(const uint256& hash, const CTransaction &tx) { // Add to memory pool without checking anything. Don't call this directly, // call CTxMemPool::accept to properly check the transaction first. @@ -3525,7 +3518,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) CValidationState state; if (tx.AcceptToMemoryPool(state, true, true, &fMissingInputs)) { - RelayTransaction(tx, inv.hash, vMsg); + RelayTransaction(tx, inv.hash); mapAlreadyAskedFor.erase(inv); vWorkQueue.push_back(inv.hash); vEraseQueue.push_back(inv.hash); @@ -3534,31 +3527,31 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) for (unsigned int i = 0; i < vWorkQueue.size(); i++) { uint256 hashPrev = vWorkQueue[i]; - for (map::iterator mi = mapOrphanTransactionsByPrev[hashPrev].begin(); + for (set::iterator mi = mapOrphanTransactionsByPrev[hashPrev].begin(); mi != mapOrphanTransactionsByPrev[hashPrev].end(); ++mi) { - const CDataStream& vMsg = *((*mi).second); - CTransaction tx; - CDataStream(vMsg) >> tx; - CInv inv(MSG_TX, tx.GetHash()); + const uint256& orphanHash = *mi; + const CTransaction& orphanTx = mapOrphanTransactions[orphanHash]; 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) + // 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; if (tx.AcceptToMemoryPool(stateDummy, true, true, &fMissingInputs2)) { - printf(" accepted orphan tx %s\n", inv.hash.ToString().c_str()); - RelayTransaction(tx, inv.hash, vMsg); - mapAlreadyAskedFor.erase(inv); - vWorkQueue.push_back(inv.hash); - vEraseQueue.push_back(inv.hash); + printf(" accepted orphan tx %s\n", orphanHash.ToString().c_str()); + RelayTransaction(orphanTx, orphanHash); + 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(inv.hash); - printf(" removed orphan tx %s\n", inv.hash.ToString().c_str()); + vEraseQueue.push_back(orphanHash); + printf(" removed orphan tx %s\n", orphanHash.ToString().c_str()); } } } @@ -3568,7 +3561,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) } else if (fMissingInputs) { - AddOrphanTx(vMsg); + AddOrphanTx(tx); // DoS prevention: do not allow mapOrphanTransactions to grow unbounded unsigned int nEvicted = LimitOrphanTxSize(MAX_ORPHAN_TRANSACTIONS); @@ -4553,9 +4546,6 @@ public: mapOrphanBlocks.clear(); // orphan transactions - std::map::iterator it3 = mapOrphanTransactions.begin(); - for (; it3 != mapOrphanTransactions.end(); it3++) - delete (*it3).second; mapOrphanTransactions.clear(); } } instance_of_cmaincleanup; diff --git a/src/main.h b/src/main.h index 19f65da88..eee985892 100644 --- a/src/main.h +++ b/src/main.h @@ -2087,7 +2087,7 @@ public: std::map mapNextTx; bool accept(CValidationState &state, CTransaction &tx, bool fCheckInputs, bool fLimitFree, bool* pfMissingInputs); - bool addUnchecked(const uint256& hash, CTransaction &tx); + bool addUnchecked(const uint256& hash, const CTransaction &tx); bool remove(const CTransaction &tx, bool fRecursive = false); bool removeConflicts(const CTransaction &tx); void clear(); diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index b1e98f65e..2ee64755d 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -16,10 +16,10 @@ #include // Tests this internal-to-main.cpp method: -extern bool AddOrphanTx(const CDataStream& vMsg); +extern bool AddOrphanTx(const CTransaction& tx); extern unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans); -extern std::map mapOrphanTransactions; -extern std::map > mapOrphanTransactionsByPrev; +extern std::map mapOrphanTransactions; +extern std::map > mapOrphanTransactionsByPrev; CService ip(uint32_t i) { @@ -133,14 +133,11 @@ BOOST_AUTO_TEST_CASE(DoS_checknbits) CTransaction RandomOrphan() { - std::map::iterator it; + std::map::iterator it; it = mapOrphanTransactions.lower_bound(GetRandHash()); if (it == mapOrphanTransactions.end()) it = mapOrphanTransactions.begin(); - const CDataStream* pvMsg = it->second; - CTransaction tx; - CDataStream(*pvMsg) >> tx; - return tx; + return it->second; } BOOST_AUTO_TEST_CASE(DoS_mapOrphans) @@ -162,9 +159,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].nValue = 1*CENT; tx.vout[0].scriptPubKey.SetDestination(key.GetPubKey().GetID()); - CDataStream ds(SER_DISK, CLIENT_VERSION); - ds << tx; - AddOrphanTx(ds); + AddOrphanTx(tx); } // ... and 50 that depend on other orphans: @@ -181,9 +176,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].scriptPubKey.SetDestination(key.GetPubKey().GetID()); SignSignature(keystore, txPrev, tx, 0); - CDataStream ds(SER_DISK, CLIENT_VERSION); - ds << tx; - AddOrphanTx(ds); + AddOrphanTx(tx); } // This really-big orphan should be ignored: @@ -207,9 +200,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) for (unsigned int j = 1; j < tx.vin.size(); j++) tx.vin[j].scriptSig = tx.vin[0].scriptSig; - CDataStream ds(SER_DISK, CLIENT_VERSION); - ds << tx; - BOOST_CHECK(!AddOrphanTx(ds)); + BOOST_CHECK(!AddOrphanTx(tx)); } // Test LimitOrphanTxSize() function: @@ -246,9 +237,7 @@ BOOST_AUTO_TEST_CASE(DoS_checkSig) tx.vout[0].nValue = 1*CENT; tx.vout[0].scriptPubKey.SetDestination(key.GetPubKey().GetID()); - CDataStream ds(SER_DISK, CLIENT_VERSION); - ds << tx; - AddOrphanTx(ds); + AddOrphanTx(tx); } // Create a transaction that depends on orphans: