From c44e4c467c30fe7790372bdea8941ba29fd3327e Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 10 Nov 2016 22:29:19 -0800 Subject: Make AcceptToMemoryPool take CTransactionRef --- src/bench/mempool_eviction.cpp | 2 +- src/net_processing.cpp | 8 +++++--- src/rpc/rawtransaction.cpp | 6 +++--- src/test/test_bitcoin.cpp | 2 +- src/test/txvalidationcache_tests.cpp | 2 +- src/txmempool.cpp | 10 +++++----- src/txmempool.h | 3 ++- src/validation.cpp | 16 +++++++++------- src/validation.h | 4 ++-- src/wallet/wallet.cpp | 4 ++-- 10 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp index 0ae69c75fc..98ab2a3555 100644 --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -18,7 +18,7 @@ static void AddTx(const CTransaction& tx, const CAmount& nFee, CTxMemPool& pool) unsigned int sigOpCost = 4; LockPoints lp; pool.addUnchecked(tx.GetHash(), CTxMemPoolEntry( - tx, nFee, nTime, dPriority, nHeight, pool.HasNoInputsOf(tx), + MakeTransactionRef(tx), nFee, nTime, dPriority, nHeight, pool.HasNoInputsOf(tx), tx.GetValueOut(), spendsCoinbase, sigOpCost, lp)); } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 87d367c2fb..4c693eea04 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1596,7 +1596,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, deque vWorkQueue; vector vEraseQueue; - CTransaction tx(deserialize, vRecv); + CTransactionRef ptx; + vRecv >> ptx; + const CTransaction& tx = *ptx; CInv inv(MSG_TX, tx.GetHash()); pfrom->AddInventoryKnown(inv); @@ -1609,7 +1611,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, pfrom->setAskFor.erase(inv.hash); mapAlreadyAskedFor.erase(inv.hash); - if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs)) { + if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs)) { mempool.check(pcoinsTip); RelayTransaction(tx, connman); for (unsigned int i = 0; i < tx.vout.size(); i++) { @@ -1646,7 +1648,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, if (setMisbehaving.count(fromPeer)) continue; - if (AcceptToMemoryPool(mempool, stateDummy, orphanTx, true, &fMissingInputs2)) { + if (AcceptToMemoryPool(mempool, stateDummy, MakeTransactionRef(orphanTx), true, &fMissingInputs2)) { LogPrint("mempool", " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanTx, connman); for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 21af449466..28f43fbec0 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -883,8 +883,8 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_str())) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); - CTransaction tx(std::move(mtx)); - uint256 hashTx = tx.GetHash(); + CTransactionRef tx(MakeTransactionRef(std::move(mtx))); + const uint256& hashTx = tx->GetHash(); bool fLimitFree = false; CAmount nMaxRawTxFee = maxTxFee; @@ -899,7 +899,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) // push to local node and sync with wallets CValidationState state; bool fMissingInputs; - if (!AcceptToMemoryPool(mempool, state, tx, fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) { + if (!AcceptToMemoryPool(mempool, state, std::move(tx), fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) { if (state.IsInvalid()) { throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason())); } else { diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 2a5a78de02..31551325b4 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -151,7 +151,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransaction &txn, CTxMemPo // Hack to assume either its completely dependent on other mempool txs or not at all CAmount inChainValue = hasNoDependencies ? txn.GetValueOut() : 0; - return CTxMemPoolEntry(txn, nFee, nTime, dPriority, nHeight, + return CTxMemPoolEntry(MakeTransactionRef(txn), nFee, nTime, dPriority, nHeight, hasNoDependencies, inChainValue, spendsCoinbase, sigOpCost, lp); } diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 9f58f004d5..ac78fb1fd8 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -23,7 +23,7 @@ ToMemPool(CMutableTransaction& tx) LOCK(cs_main); CValidationState state; - return AcceptToMemoryPool(mempool, state, tx, false, NULL, true, 0); + return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), false, NULL, true, 0); } BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index c3da69c3f0..90a8066114 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -20,22 +20,22 @@ using namespace std; -CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, +CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, bool poolHasNoInputsOf, CAmount _inChainInputValue, bool _spendsCoinbase, int64_t _sigOpsCost, LockPoints lp): - tx(MakeTransactionRef(_tx)), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), + tx(_tx), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), spendsCoinbase(_spendsCoinbase), sigOpCost(_sigOpsCost), lockPoints(lp) { - nTxWeight = GetTransactionWeight(_tx); - nModSize = _tx.CalculateModifiedSize(GetTxSize()); + nTxWeight = GetTransactionWeight(*tx); + nModSize = tx->CalculateModifiedSize(GetTxSize()); nUsageSize = RecursiveDynamicUsage(*tx) + memusage::DynamicUsage(tx); nCountWithDescendants = 1; nSizeWithDescendants = GetTxSize(); nModFeesWithDescendants = nFee; - CAmount nValueIn = _tx.GetValueOut()+nFee; + CAmount nValueIn = tx->GetValueOut()+nFee; assert(inChainInputValue <= nValueIn); feeDelta = 0; diff --git a/src/txmempool.h b/src/txmempool.h index 8a5787a886..1c0fbf6052 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -111,10 +111,11 @@ private: int64_t nSigOpCostWithAncestors; public: - CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, + CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, bool poolHasNoInputsOf, CAmount _inChainInputValue, bool spendsCoinbase, int64_t nSigOpsCost, LockPoints lp); + CTxMemPoolEntry(const CTxMemPoolEntry& other); const CTransaction& GetTx() const { return *this->tx; } diff --git a/src/validation.cpp b/src/validation.cpp index bd60bbfdcd..bc14c1d8c6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -525,10 +525,11 @@ std::string FormatStateMessage(const CValidationState &state) state.GetRejectCode()); } -bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const CTransaction& tx, bool fLimitFree, +bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx, bool fLimitFree, bool* pfMissingInputs, int64_t nAcceptTime, bool fOverrideMempoolLimit, const CAmount& nAbsurdFee, std::vector& vHashTxnToUncache) { + const CTransaction& tx = *ptx; const uint256 hash = tx.GetHash(); AssertLockHeld(cs_main); if (pfMissingInputs) @@ -691,7 +692,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C } } - CTxMemPoolEntry entry(tx, nFees, nAcceptTime, dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase, nSigOpsCost, lp); + CTxMemPoolEntry entry(ptx, nFees, nAcceptTime, dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase, nSigOpsCost, lp); unsigned int nSize = entry.GetTxSize(); // Check that the transaction doesn't have an excessive number of @@ -955,7 +956,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C return true; } -bool AcceptToMemoryPoolWithTime(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree, +bool AcceptToMemoryPoolWithTime(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree, bool* pfMissingInputs, int64_t nAcceptTime, bool fOverrideMempoolLimit, const CAmount nAbsurdFee) { std::vector vHashTxToUncache; @@ -970,7 +971,7 @@ bool AcceptToMemoryPoolWithTime(CTxMemPool& pool, CValidationState &state, const return res; } -bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree, +bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree, bool* pfMissingInputs, bool fOverrideMempoolLimit, const CAmount nAbsurdFee) { return AcceptToMemoryPoolWithTime(pool, state, tx, fLimitFree, pfMissingInputs, GetTime(), fOverrideMempoolLimit, nAbsurdFee); @@ -2116,7 +2117,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara const CTransaction& tx = *it; // ignore validation errors in resurrected transactions CValidationState stateDummy; - if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, true)) { + if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, it, false, NULL, true)) { mempool.removeRecursive(tx); } else if (mempool.exists(tx.GetHash())) { vHashUpdate.push_back(tx.GetHash()); @@ -4054,15 +4055,16 @@ bool LoadMempool(void) file >> num; double prioritydummy = 0; while (num--) { + CTransactionRef tx; int64_t nTime; int64_t nFeeDelta; - CTransaction tx(deserialize, file); + file >> tx; file >> nTime; file >> nFeeDelta; CAmount amountdelta = nFeeDelta; if (amountdelta) { - mempool.PrioritiseTransaction(tx.GetHash(), tx.GetHash().ToString(), prioritydummy, amountdelta); + mempool.PrioritiseTransaction(tx->GetHash(), tx->GetHash().ToString(), prioritydummy, amountdelta); } CValidationState state; if (nTime + nExpiryTimeout > nNow) { diff --git a/src/validation.h b/src/validation.h index f0f54786c7..ce3774f10f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -309,11 +309,11 @@ void FlushStateToDisk(); void PruneAndFlush(); /** (try to) add transaction to memory pool **/ -bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree, +bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree, bool* pfMissingInputs, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0); /** (try to) add transaction to memory pool with a specified acceptance time **/ -bool AcceptToMemoryPoolWithTime(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree, +bool AcceptToMemoryPoolWithTime(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree, bool* pfMissingInputs, int64_t nAcceptTime, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0); /** Convert CValidationState to a human-readable message for logging */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1261aad3f2..e918b6e156 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2563,7 +2563,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) { // Lastly, ensure this tx will pass the mempool's chain limits LockPoints lp; - CTxMemPoolEntry entry(txNew, 0, 0, 0, 0, false, 0, false, 0, lp); + CTxMemPoolEntry entry(wtxNew.tx, 0, 0, 0, 0, false, 0, false, 0, lp); CTxMemPool::setEntries setAncestors; size_t nLimitAncestors = GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); size_t nLimitAncestorSize = GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000; @@ -3782,5 +3782,5 @@ int CMerkleTx::GetBlocksToMaturity() const bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state) { - return ::AcceptToMemoryPool(mempool, state, *this, true, NULL, false, nAbsurdFee); + return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, false, nAbsurdFee); } -- cgit v1.2.3 From 62607d796cd93bfc5ce4ee5050c93cf0ca80ba91 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 5 Dec 2016 00:15:33 -0800 Subject: Convert COrphanTx to keep a CTransactionRef --- src/net_processing.cpp | 25 +++++++++++++------------ src/test/DoS_tests.cpp | 24 ++++++++++++------------ 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4c693eea04..10aaa77fff 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -51,7 +51,7 @@ struct IteratorComparator struct COrphanTx { // When modifying, adapt the copy of this definition in tests/DoS_tests. - CTransaction tx; + CTransactionRef tx; NodeId fromPeer; int64_t nTimeExpire; }; @@ -586,9 +586,9 @@ void UnregisterNodeSignals(CNodeSignals& nodeSignals) // mapOrphanTransactions // -bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { - uint256 hash = tx.GetHash(); + const uint256& hash = tx->GetHash(); if (mapOrphanTransactions.count(hash)) return false; @@ -599,7 +599,7 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(c // have been mined or received. // 100 orphans, each of which is at most 99,999 bytes big is // at most 10 megabytes of orphans and somewhat more byprev index (in the worst case): - unsigned int sz = GetTransactionWeight(tx); + unsigned int sz = GetTransactionWeight(*tx); if (sz >= MAX_STANDARD_TX_WEIGHT) { LogPrint("mempool", "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString()); @@ -608,7 +608,7 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(c auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME}); assert(ret.second); - BOOST_FOREACH(const CTxIn& txin, tx.vin) { + BOOST_FOREACH(const CTxIn& txin, tx->vin) { mapOrphanTransactionsByPrev[txin.prevout].insert(ret.first); } @@ -622,7 +622,7 @@ int static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) map::iterator it = mapOrphanTransactions.find(hash); if (it == mapOrphanTransactions.end()) return 0; - BOOST_FOREACH(const CTxIn& txin, it->second.tx.vin) + BOOST_FOREACH(const CTxIn& txin, it->second.tx->vin) { auto itPrev = mapOrphanTransactionsByPrev.find(txin.prevout); if (itPrev == mapOrphanTransactionsByPrev.end()) @@ -644,7 +644,7 @@ void EraseOrphansFor(NodeId peer) map::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid if (maybeErase->second.fromPeer == peer) { - nErased += EraseOrphanTx(maybeErase->second.tx.GetHash()); + nErased += EraseOrphanTx(maybeErase->second.tx->GetHash()); } } if (nErased > 0) LogPrint("mempool", "Erased %d orphan tx from peer %d\n", nErased, peer); @@ -665,7 +665,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRE { map::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += EraseOrphanTx(maybeErase->second.tx.GetHash()); + nErased += EraseOrphanTx(maybeErase->second.tx->GetHash()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -736,7 +736,7 @@ void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIn auto itByPrev = mapOrphanTransactionsByPrev.find(tx.vin[j].prevout); if (itByPrev == mapOrphanTransactionsByPrev.end()) continue; for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) { - const CTransaction& orphanTx = (*mi)->second.tx; + const CTransaction& orphanTx = *(*mi)->second.tx; const uint256& orphanHash = orphanTx.GetHash(); vOrphanErase.push_back(orphanHash); } @@ -1636,7 +1636,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, mi != itByPrev->second.end(); ++mi) { - const CTransaction& orphanTx = (*mi)->second.tx; + const CTransactionRef& porphanTx = (*mi)->second.tx; + const CTransaction& orphanTx = *porphanTx; const uint256& orphanHash = orphanTx.GetHash(); NodeId fromPeer = (*mi)->second.fromPeer; bool fMissingInputs2 = false; @@ -1648,7 +1649,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, if (setMisbehaving.count(fromPeer)) continue; - if (AcceptToMemoryPool(mempool, stateDummy, MakeTransactionRef(orphanTx), true, &fMissingInputs2)) { + if (AcceptToMemoryPool(mempool, stateDummy, porphanTx, true, &fMissingInputs2)) { LogPrint("mempool", " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanTx, connman); for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { @@ -1701,7 +1702,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, pfrom->AddInventoryKnown(_inv); if (!AlreadyHave(_inv)) pfrom->AskFor(_inv); } - AddOrphanTx(tx, pfrom->GetId()); + AddOrphanTx(ptx, pfrom->GetId()); // DoS prevention: do not allow mapOrphanTransactions to grow unbounded unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)); diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index 131d667e0b..493a568b49 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -23,11 +23,11 @@ #include // Tests this internal-to-main.cpp method: -extern bool AddOrphanTx(const CTransaction& tx, NodeId peer); +extern bool AddOrphanTx(const CTransactionRef& tx, NodeId peer); extern void EraseOrphansFor(NodeId peer); extern unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans); struct COrphanTx { - CTransaction tx; + CTransactionRef tx; NodeId fromPeer; int64_t nTimeExpire; }; @@ -115,7 +115,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) BOOST_CHECK(!connman->IsBanned(addr)); } -CTransaction RandomOrphan() +CTransactionRef RandomOrphan() { std::map::iterator it; it = mapOrphanTransactions.lower_bound(GetRandHash()); @@ -143,30 +143,30 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vout[0].nValue = 1*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(key.GetPubKey().GetID()); - AddOrphanTx(tx, i); + AddOrphanTx(MakeTransactionRef(tx), i); } // ... and 50 that depend on other orphans: for (int i = 0; i < 50; i++) { - CTransaction txPrev = RandomOrphan(); + CTransactionRef txPrev = RandomOrphan(); CMutableTransaction tx; tx.vin.resize(1); tx.vin[0].prevout.n = 0; - tx.vin[0].prevout.hash = txPrev.GetHash(); + tx.vin[0].prevout.hash = txPrev->GetHash(); tx.vout.resize(1); tx.vout[0].nValue = 1*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(key.GetPubKey().GetID()); - SignSignature(keystore, txPrev, tx, 0, SIGHASH_ALL); + SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL); - AddOrphanTx(tx, i); + AddOrphanTx(MakeTransactionRef(tx), i); } // This really-big orphan should be ignored: for (int i = 0; i < 10; i++) { - CTransaction txPrev = RandomOrphan(); + CTransactionRef txPrev = RandomOrphan(); CMutableTransaction tx; tx.vout.resize(1); @@ -176,15 +176,15 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) for (unsigned int j = 0; j < tx.vin.size(); j++) { tx.vin[j].prevout.n = j; - tx.vin[j].prevout.hash = txPrev.GetHash(); + tx.vin[j].prevout.hash = txPrev->GetHash(); } - SignSignature(keystore, txPrev, tx, 0, SIGHASH_ALL); + SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL); // Re-use same signature for other inputs // (they don't have to be valid for this test) for (unsigned int j = 1; j < tx.vin.size(); j++) tx.vin[j].scriptSig = tx.vin[0].scriptSig; - BOOST_CHECK(!AddOrphanTx(tx, i)); + BOOST_CHECK(!AddOrphanTx(MakeTransactionRef(tx), i)); } // Test EraseOrphansFor: -- cgit v1.2.3 From 6713f0f142d97b4608c95a3ea03b4b670fceab2b Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 11 Nov 2016 13:01:27 -0800 Subject: Make FillBlock consume txn_available to avoid shared_ptr copies --- src/blockencodings.cpp | 14 ++++++++---- src/blockencodings.h | 2 +- src/test/blockencodings_tests.cpp | 45 +++++++++++++++++++++++++-------------- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 914af0c670..72fe17bdc7 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -142,8 +142,9 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const { return txn_available[index] ? true : false; } -ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector& vtx_missing) const { +ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector& vtx_missing) { assert(!header.IsNull()); + uint256 hash = header.GetHash(); block = header; block.vtx.resize(txn_available.size()); @@ -154,8 +155,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< return READ_STATUS_INVALID; block.vtx[i] = vtx_missing[tx_missing_offset++]; } else - block.vtx[i] = txn_available[i]; + block.vtx[i] = std::move(txn_available[i]); } + + // Make sure we can't call FillBlock again. + header.SetNull(); + txn_available.clear(); + if (vtx_missing.size() != tx_missing_offset) return READ_STATUS_INVALID; @@ -170,10 +176,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< return READ_STATUS_CHECKBLOCK_FAILED; } - LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", header.GetHash().ToString(), prefilled_count, mempool_count, vtx_missing.size()); + LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, vtx_missing.size()); if (vtx_missing.size() < 5) { for (const auto& tx : vtx_missing) - LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", header.GetHash().ToString(), tx->GetHash().ToString()); + LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString()); } return READ_STATUS_OK; diff --git a/src/blockencodings.h b/src/blockencodings.h index 27baf1f8f8..809ccbf936 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -202,7 +202,7 @@ public: ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock); bool IsTxAvailable(size_t index) const; - ReadStatus FillBlock(CBlock& block, const std::vector& vtx_missing) const; + ReadStatus FillBlock(CBlock& block, const std::vector& vtx_missing); }; #endif diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 7478758f73..90f273e731 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -85,17 +85,23 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) BOOST_CHECK_EQUAL(pool.size(), poolSize - 1); CBlock block2; - std::vector vtx_missing; - BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions + { + PartiallyDownloadedBlock tmp = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions + partialBlock = tmp; + } - vtx_missing.push_back(block.vtx[2]); // Wrong transaction - partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that + // Wrong transaction + { + PartiallyDownloadedBlock tmp = partialBlock; + partialBlock.FillBlock(block2, {block.vtx[2]}); // Current implementation doesn't check txn here, but don't require that + partialBlock = tmp; + } bool mutated; BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated)); - vtx_missing[0] = block.vtx[1]; CBlock block3; - BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK); + BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}) == READ_STATUS_OK); BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString()); BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString()); BOOST_CHECK(!mutated); @@ -181,17 +187,24 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); CBlock block2; - std::vector vtx_missing; - BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions + { + PartiallyDownloadedBlock tmp = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions + partialBlock = tmp; + } - vtx_missing.push_back(block.vtx[1]); // Wrong transaction - partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that + // Wrong transaction + { + PartiallyDownloadedBlock tmp = partialBlock; + partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that + partialBlock = tmp; + } bool mutated; BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated)); - vtx_missing[0] = block.vtx[0]; CBlock block3; - BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK); + PartiallyDownloadedBlock partialBlockCopy = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}) == READ_STATUS_OK); BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString()); BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString()); BOOST_CHECK(!mutated); @@ -200,7 +213,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) block.vtx.clear(); block2.vtx.clear(); block3.vtx.clear(); - BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); + BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy. } BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); } @@ -240,8 +253,8 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); CBlock block2; - std::vector vtx_missing; - BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_OK); + PartiallyDownloadedBlock partialBlockCopy = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_OK); BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString()); bool mutated; BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString()); @@ -250,7 +263,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) txhash = block.vtx[1]->GetHash(); block.vtx.clear(); block2.vtx.clear(); - BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); + BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy. } BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); } -- cgit v1.2.3 From 91335ba389918966c94eeb7071b6c5a998bf1be8 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 5 Dec 2016 20:38:20 -0800 Subject: Remove unused MakeTransactionRef overloads --- src/primitives/transaction.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 4c4a238dd5..8e5b424408 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -453,8 +453,6 @@ struct CMutableTransaction typedef std::shared_ptr CTransactionRef; static inline CTransactionRef MakeTransactionRef() { return std::make_shared(); } template static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared(std::forward(txIn)); } -static inline CTransactionRef MakeTransactionRef(const CTransactionRef& txIn) { return txIn; } -static inline CTransactionRef MakeTransactionRef(CTransactionRef&& txIn) { return std::move(txIn); } /** Compute the weight of a transaction, as defined by BIP 141 */ int64_t GetTransactionWeight(const CTransaction &tx); -- cgit v1.2.3