From bdd5221cf67e241cad9934e3c9d4330ecd816aa9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 30 May 2016 16:50:14 +0200 Subject: [PATCH 1/9] Add support for unique_ptr and shared_ptr to memusage --- src/memusage.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/memusage.h b/src/memusage.h index 6ebdf0ae9c028..364cedbf33ccc 100644 --- a/src/memusage.h +++ b/src/memusage.h @@ -80,6 +80,15 @@ struct stl_tree_node X x; }; +struct stl_shared_counter +{ + /* Various platforms use different sized counters here. + * Conservatively assume that they won't be larger than size_t. */ + void* class_type; + size_t use_count; + size_t weak_count; +}; + template static inline size_t DynamicUsage(const std::vector& v) { @@ -158,6 +167,21 @@ static inline size_t RecursiveDynamicUsage(const std::pair& v) return RecursiveDynamicUsage(v.first) + RecursiveDynamicUsage(v.second); } +template +static inline size_t DynamicUsage(const std::unique_ptr& p) +{ + return p ? MallocUsage(sizeof(X)) : 0; +} + +template +static inline size_t DynamicUsage(const std::shared_ptr& p) +{ + // A shared_ptr can either use a single continuous memory block for both + // the counter and the storage (when using std::make_shared), or separate. + // We can't observe the difference, however, so assume the worst. + return p ? MallocUsage(sizeof(X)) + MallocUsage(sizeof(stl_shared_counter)) : 0; +} + // Boost data structures template From 2a89063e06711e9720dd38af6110574fece227ab Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 12 Oct 2020 16:22:12 -0300 Subject: [PATCH 2/9] Initial mempool migration to using CTransationRef instead of a CTransation copy. --- src/test/mempool_tests.cpp | 6 +++--- src/test/policyestimator_tests.cpp | 2 +- src/txmempool.cpp | 33 +++++++++++++++--------------- src/txmempool.h | 12 ++++++----- src/validation.cpp | 14 ++++++------- 5 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index a4bc80a81a77e..436a9ec4069b7 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) CTxMemPool testPool(CFeeRate(0)); - std::list removed; + std::list removed; // Nothing in pool, remove should do nothing: testPool.remove(txParent, removed, true); @@ -278,7 +278,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) BOOST_CHECK_EQUAL(pool.size(), 10); // Now try removing tx10 and verify the sort order returns to normal - std::list removed; + std::list removed; pool.remove(pool.mapTx.find(tx10.GetHash())->GetTx(), removed, true); CheckSort<1>(pool, snapshotOrder); @@ -442,7 +442,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) pool.addUnchecked(tx7.GetHash(), entry.Fee(9000LL).FromTx(tx7, &pool)); std::vector vtx; - std::list conflicts; + std::list conflicts; SetMockTime(42); SetMockTime(42 + CTxMemPool::ROLLING_FEE_HALFLIFE); BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + 1000); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index aaacb62f41293..1ce118cf3696e 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -37,7 +37,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (unsigned int i = 0; i < 128; i++) garbage.push_back('X'); CMutableTransaction tx; - std::list dummyConflicted; + std::list dummyConflicted; tx.vin.resize(1); tx.vin[0].scriptSig = garbage; tx.vout.resize(1); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e589b56d5dd6d..7f9d6f6f605f3 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -10,7 +10,6 @@ #include "policy/fees.h" #include "streams.h" #include "timedata.h" -#include "consensus/tx_verify.h" #include "util.h" #include "utilmoneystr.h" #include "utiltime.h" @@ -24,18 +23,18 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, bool poolHasNoInputsOf, CAmount _inChainInputValue, bool _spendsCoinbaseOrCoinstake, unsigned int _sigOps) : - tx(_tx), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), spendsCoinbaseOrCoinstake(_spendsCoinbaseOrCoinstake), sigOpCount(_sigOps) + tx(MakeTransactionRef(_tx)), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), spendsCoinbaseOrCoinstake(_spendsCoinbaseOrCoinstake), sigOpCount(_sigOps) { - nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); - nModSize = tx.CalculateModifiedSize(nTxSize); - nUsageSize = tx.DynamicMemoryUsage(); - hasZerocoins = tx.ContainsZerocoins(); - m_isShielded = tx.hasSaplingData(); + nTxSize = ::GetSerializeSize(_tx, SER_NETWORK, PROTOCOL_VERSION); + nModSize = _tx.CalculateModifiedSize(nTxSize); + nUsageSize = _tx.DynamicMemoryUsage(); + hasZerocoins = _tx.ContainsZerocoins(); + m_isShielded = tx->hasSaplingData(); nCountWithDescendants = 1; nSizeWithDescendants = nTxSize; nFeesWithDescendants = nFee; - CAmount nValueIn = tx.GetValueOut()+nFee; + CAmount nValueIn = _tx.GetValueOut()+nFee; assert(inChainInputValue <= nValueIn); feeDelta = 0; @@ -177,14 +176,14 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) { setEntries parentHashes; - const CTransaction &tx = entry.GetTx(); + const auto &tx = entry.GetSharedTx(); if (fSearchForParents) { // Get parents of this transaction that are in the mempool // GetMemPoolParents() is only valid for entries in the mempool, so we // iterate mapTx to find parents. - for (unsigned int i = 0; i < tx.vin.size(); i++) { - txiter piter = mapTx.find(tx.vin[i].prevout.hash); + for (unsigned int i = 0; i < tx->vin.size(); i++) { + txiter piter = mapTx.find(tx->vin[i].prevout.hash); if (piter != mapTx.end()) { parentHashes.insert(piter); if (parentHashes.size() + 1 > limitAncestorCount) { @@ -456,7 +455,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants } } -void CTxMemPool::remove(const CTransaction& origTx, std::list& removed, bool fRecursive) +void CTxMemPool::remove(const CTransaction& origTx, std::list& removed, bool fRecursive) { // Remove transaction from memory pool { @@ -488,7 +487,7 @@ void CTxMemPool::remove(const CTransaction& origTx, std::list& rem setAllRemoves.swap(txToRemove); } for (const txiter& it : setAllRemoves) { - removed.push_back(it->GetTx()); + removed.emplace_back(it->GetSharedTx()); } RemoveStaged(setAllRemoves); } @@ -518,12 +517,12 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem } } for (const CTransaction& tx : transactionsToRemove) { - std::list removed; + std::list removed; remove(tx, removed, true); } } -void CTxMemPool::removeConflicts(const CTransaction& tx, std::list& removed) +void CTxMemPool::removeConflicts(const CTransaction& tx, std::list& removed) { // Remove transactions which depend on inputs of tx, recursively std::list result; @@ -542,7 +541,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::list /** * Called when a block is connected. Removes from mempool and updates the miner fee estimator. */ -void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, std::list& conflicts, bool fCurrentEstimate) +void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, std::list& conflicts, bool fCurrentEstimate) { LOCK(cs); std::vector entries; @@ -553,7 +552,7 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne entries.push_back(*i); } for (const auto& tx : vtx) { - std::list dummy; + std::list dummy; remove(*tx, dummy, false); removeConflicts(*tx, conflicts); ClearPrioritisation(tx->GetHash()); diff --git a/src/txmempool.h b/src/txmempool.h index 9a1f7f4b7def1..6f7f7cfdaabd3 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -8,6 +8,7 @@ #define BITCOIN_TXMEMPOOL_H #include +#include #include #include "amount.h" @@ -59,7 +60,7 @@ class CTxMemPool; class CTxMemPoolEntry { private: - CTransaction tx; + CTransactionRef tx; CAmount nFee; //! Cached to avoid expensive parent-transaction lookups size_t nTxSize; //! ... and avoid recomputing tx size size_t nModSize; //! ... and modified size for priority @@ -92,7 +93,8 @@ class CTxMemPoolEntry unsigned int nSigOps); CTxMemPoolEntry(const CTxMemPoolEntry& other); - const CTransaction& GetTx() const { return this->tx; } + const CTransaction& GetTx() const { return *this->tx; } + std::shared_ptr GetSharedTx() const { return this->tx; } /** * Fast calculation of lower bound of current priority as update * from entry priority. Only inputs that were originally in-chain will age. @@ -463,10 +465,10 @@ class CTxMemPool // then invoke the second version. bool addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry, bool fCurrentEstimate = true); bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); - void remove(const CTransaction& tx, std::list& removed, bool fRecursive = false); + void remove(const CTransaction& tx, std::list& removed, bool fRecursive = false); void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags); - void removeConflicts(const CTransaction& tx, std::list& removed); - void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, std::list& conflicts, bool fCurrentEstimate = true); + void removeConflicts(const CTransaction& tx, std::list& removed); + void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, std::list& conflicts, bool fCurrentEstimate = true); void clear(); void _clear(); // lock-free void queryHashes(std::vector& vtxid); diff --git a/src/validation.cpp b/src/validation.cpp index 2d1c4905f79ba..541db0b899b75 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1892,7 +1892,7 @@ bool static DisconnectTip(CValidationState& state) for (const auto& txIn : block.vtx) { const CTransaction& tx = *txIn; // ignore validation errors in resurrected transactions - std::list removed; + std::list removed; CValidationState stateDummy; if (tx.IsCoinBase() || tx.IsCoinStake() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, nullptr, true)) { mempool.remove(tx, removed, true); @@ -1926,7 +1926,7 @@ static int64_t nTimePostConnect = 0; * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. */ -bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CBlock* pblock, bool fAlreadyChecked, std::list &txConflicted, std::vector> &txChanged) +bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CBlock* pblock, bool fAlreadyChecked, std::list &txConflicted, std::vector> &txChanged) { assert(pindexNew->pprev == chainActive.Tip()); @@ -2111,7 +2111,7 @@ static void PruneBlockIndexCandidates() * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const CBlock* pblock, bool fAlreadyChecked, std::list& txConflicted, std::vector>& txChanged) +static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const CBlock* pblock, bool fAlreadyChecked, std::list& txConflicted, std::vector>& txChanged) { AssertLockHeld(cs_main); if (pblock == NULL) @@ -2210,7 +2210,7 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre boost::this_thread::interruption_point(); const CBlockIndex *pindexFork; - std::list txConflicted; + std::list txConflicted; bool fInitialDownload; while (true) { TRY_LOCK(cs_main, lockMain); @@ -2234,11 +2234,11 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre fInitialDownload = IsInitialBlockDownload(); // throw all transactions though the signal-interface - for (const CTransaction &tx : txConflicted) { - GetMainSignals().SyncTransaction(tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + for (const auto &tx : txConflicted) { + GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); } // ... and about transactions that got confirmed: - for(unsigned int i = 0; i < txChanged.size(); i++) { + for (unsigned int i = 0; i < txChanged.size(); i++) { GetMainSignals().SyncTransaction(std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i])); } From 80a94df643716f2bc0bfeefaba3aac4490a96178 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 12 Oct 2020 17:20:51 -0300 Subject: [PATCH 3/9] Migrating to CTransationRef second round: 1) mempool: CTxMemPoolEntry migrated to receive CTransationRef as constructor argument. 2) validation: ATMP and ATMPW migrated to receive CTransationRef. 3) net_processing: ProcessMessage migrated TX message processing to use CTransactionRef. 4) swiftx: ProcessMessage migrated IX message processing to use CTransactionRef. 5) migrated mapOrphanTransactions to use CTransactionRef. --- src/net_processing.cpp | 33 ++++++++++++++++---------------- src/rpc/rawtransaction.cpp | 2 +- src/swifttx.cpp | 11 ++++++----- src/swifttx.h | 4 ++-- src/test/DoS_tests.cpp | 24 +++++++++++------------ src/test/test_pivx.cpp | 4 ++-- src/txmempool.cpp | 14 +++++++------- src/txmempool.h | 2 +- src/validation.cpp | 18 ++++++++--------- src/validation.h | 2 +- src/wallet/test/wallet_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 4 ++-- 12 files changed, 62 insertions(+), 60 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fd29c706ba20e..e0d337b5882ba 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -24,7 +24,7 @@ int64_t nTimeBestReceived = 0; // Used only to inform the wallet of when we las static const uint64_t RANDOMIZER_ID_ADDRESS_RELAY = 0x3cac0035b5866b90ULL; // SHA256("main address relay")[0:8] struct COrphanTx { - CTransaction tx; + CTransactionRef tx; NodeId fromPeer; }; @@ -511,9 +511,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; @@ -524,15 +524,15 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(c // 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: - unsigned int sz = GetSerializeSize(tx, SER_NETWORK, CTransaction::CURRENT_VERSION); + unsigned int sz = GetSerializeSize(*tx, SER_NETWORK, CTransaction::CURRENT_VERSION); if (sz > 5000) { LogPrint(BCLog::MEMPOOL, "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString()); return false; } - mapOrphanTransactions[hash].tx = tx; - mapOrphanTransactions[hash].fromPeer = peer; - for (const CTxIn& txin : tx.vin) + auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer}); + assert(ret.second); + for (const CTxIn& txin : tx->vin) mapOrphanTransactionsByPrev[txin.prevout.hash].insert(hash); LogPrint(BCLog::MEMPOOL, "stored orphan tx %s (mapsz %u prevsz %u)\n", hash.ToString(), @@ -545,7 +545,7 @@ void static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) std::map::iterator it = mapOrphanTransactions.find(hash); if (it == mapOrphanTransactions.end()) return; - for (const CTxIn& txin : it->second.tx.vin) { + for (const CTxIn& txin : it->second.tx->vin) { std::map >::iterator itPrev = mapOrphanTransactionsByPrev.find(txin.prevout.hash); if (itPrev == mapOrphanTransactionsByPrev.end()) continue; @@ -563,7 +563,7 @@ void EraseOrphansFor(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) while (iter != mapOrphanTransactions.end()) { std::map::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid if (maybeErase->second.fromPeer == peer) { - EraseOrphanTx(maybeErase->second.tx.GetHash()); + EraseOrphanTx(maybeErase->second.tx->GetHash()); ++nErased; } } @@ -1401,8 +1401,9 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR else if (strCommand == NetMsgType::TX) { std::vector vWorkQueue; std::vector vEraseQueue; - CTransaction tx; - vRecv >> tx; + CTransactionRef ptx; + vRecv >> ptx; + const CTransaction& tx = *ptx; CInv inv(MSG_TX, tx.GetHash()); pfrom->AddInventoryKnown(inv); @@ -1416,7 +1417,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR mapAlreadyAskedFor.erase(inv); - if (!tx.HasZerocoinSpendInputs() && AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs, false, ignoreFees)) { + if (!tx.HasZerocoinSpendInputs() && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs, false, ignoreFees)) { mempool.check(pcoinsTip); RelayTransaction(tx, connman); vWorkQueue.push_back(inv.hash); @@ -1435,7 +1436,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR mi != itByPrev->second.end(); ++mi) { const uint256 &orphanHash = *mi; - const CTransaction &orphanTx = mapOrphanTransactions[orphanHash].tx; + const auto &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 @@ -1448,7 +1449,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR continue; if(AcceptToMemoryPool(mempool, stateDummy, orphanTx, true, &fMissingInputs2)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); - RelayTransaction(orphanTx, connman); + RelayTransaction(*orphanTx, connman); vWorkQueue.push_back(orphanHash); vEraseQueue.push_back(orphanHash); } else if(!fMissingInputs2) { @@ -1472,7 +1473,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR for (uint256& hash : vEraseQueue) EraseOrphanTx(hash); - } else if (tx.HasZerocoinSpendInputs() && AcceptToMemoryPool(mempool, state, tx, true, &fMissingZerocoinInputs, false, false, ignoreFees)) { + } else if (tx.HasZerocoinSpendInputs() && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingZerocoinInputs, false, false, ignoreFees)) { //Presstab: ZCoin has a bunch of code commented out here. Is this something that should have more going on? //Also there is nothing that handles fMissingZerocoinInputs. Does there need to be? RelayTransaction(tx, connman); @@ -1481,7 +1482,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR tx.GetHash().ToString(), mempool.mapTx.size()); } else if (fMissingInputs) { - 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/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 23d72283a55aa..5fae8c834bdd5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -863,7 +863,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) } CValidationState state; bool fMissingInputs; - if (!AcceptToMemoryPool(mempool, state, tx, false, &fMissingInputs, false, !fOverrideFees)) { + if (!AcceptToMemoryPool(mempool, state, MakeTransactionRef(std::move(tx)), false, &fMissingInputs, false, !fOverrideFees)) { if (state.IsInvalid()) { throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason())); } else { diff --git a/src/swifttx.cpp b/src/swifttx.cpp index e32524108de18..b6561967fed3d 100644 --- a/src/swifttx.cpp +++ b/src/swifttx.cpp @@ -44,8 +44,9 @@ void ProcessMessageSwiftTX(CNode* pfrom, std::string& strCommand, CDataStream& v if (strCommand == NetMsgType::IX) { //LogPrintf("ProcessMessageSwiftTX::ix\n"); CDataStream vMsg(vRecv); - CTransaction tx; - vRecv >> tx; + CTransactionRef ptx; + vRecv >> ptx; + const CTransaction& tx = *ptx; CInv inv(MSG_TXLOCK_REQUEST, tx.GetHash()); pfrom->AddInventoryKnown(inv); @@ -75,7 +76,7 @@ void ProcessMessageSwiftTX(CNode* pfrom, std::string& strCommand, CDataStream& v bool fAccepted = false; { LOCK(cs_main); - fAccepted = AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs); + fAccepted = AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs); } if (fAccepted) { g_connman->RelayInv(inv); @@ -260,7 +261,7 @@ int64_t CreateNewLock(CTransaction tx) } // check if we need to vote on this transaction -void DoConsensusVote(CTransaction& tx, int64_t nBlockHeight) +void DoConsensusVote(const CTransaction& tx, int64_t nBlockHeight) { if (!fMasterNode) return; @@ -400,7 +401,7 @@ bool ProcessConsensusVote(CNode* pnode, CConsensusVote& ctx) return false; } -bool CheckForConflictingLocks(CTransaction& tx) +bool CheckForConflictingLocks(const CTransaction& tx) { /* It's possible (very unlikely though) to get 2 conflicting transaction locks approved by the network. diff --git a/src/swifttx.h b/src/swifttx.h index ffaabe9f9dfa3..d9ff34cec5670 100644 --- a/src/swifttx.h +++ b/src/swifttx.h @@ -45,12 +45,12 @@ int64_t CreateNewLock(CTransaction tx); bool IsIXTXValid(const CTransaction& txCollateral); // if two conflicting locks are approved by the network, they will cancel out -bool CheckForConflictingLocks(CTransaction& tx); +bool CheckForConflictingLocks(const CTransaction& tx); void ProcessMessageSwiftTX(CNode* pfrom, std::string& strCommand, CDataStream& vRecv); //check if we need to vote on this transaction -void DoConsensusVote(CTransaction& tx, int64_t nBlockHeight); +void DoConsensusVote(const CTransaction& tx, int64_t nBlockHeight); //process consensus vote message bool ProcessConsensusVote(CNode* pnode, CConsensusVote& ctx); diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index 3f5e933cd949e..9863008d7a9ce 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -25,11 +25,11 @@ #include // Tests this internal-to-validation.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; }; extern std::map mapOrphanTransactions; @@ -132,7 +132,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) BOOST_CHECK(!connman->IsBanned(addr)); } -CTransaction RandomOrphan() +CTransactionRef RandomOrphan() { std::map::iterator it; it = mapOrphanTransactions.lower_bound(InsecureRand256()); @@ -160,30 +160,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); @@ -193,15 +193,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: diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index af46f6a1e16f5..e0a7a4b3aa6f2 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -78,10 +78,10 @@ TestingSetup::~TestingSetup() } CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(CMutableTransaction &tx, CTxMemPool *pool) { - CTransaction txn(tx); + const CTransactionRef txn = MakeTransactionRef(tx); // todo: move to the caller side.. bool hasNoDependencies = pool ? pool->HasNoInputsOf(tx) : hadNoDependencies; // Hack to assume either its completely dependent on other mempool txs or not at all - CAmount inChainValue = hasNoDependencies ? txn.GetValueOut() : 0; + CAmount inChainValue = hasNoDependencies ? txn->GetValueOut() : 0; return CTxMemPoolEntry(txn, nFee, nTime, dPriority, nHeight, hasNoDependencies, inChainValue, spendsCoinbaseOrCoinstake, sigOpCount); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 7f9d6f6f605f3..0cda0fbd22dad 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -19,22 +19,22 @@ #include -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 _spendsCoinbaseOrCoinstake, unsigned int _sigOps) : tx(MakeTransactionRef(_tx)), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), spendsCoinbaseOrCoinstake(_spendsCoinbaseOrCoinstake), sigOpCount(_sigOps) { - nTxSize = ::GetSerializeSize(_tx, SER_NETWORK, PROTOCOL_VERSION); - nModSize = _tx.CalculateModifiedSize(nTxSize); - nUsageSize = _tx.DynamicMemoryUsage(); - hasZerocoins = _tx.ContainsZerocoins(); - m_isShielded = tx->hasSaplingData(); + nTxSize = ::GetSerializeSize(*_tx, SER_NETWORK, PROTOCOL_VERSION); + nModSize = _tx->CalculateModifiedSize(nTxSize); + nUsageSize = _tx->DynamicMemoryUsage(); + hasZerocoins = _tx->ContainsZerocoins(); + m_isShielded = _tx->hasSaplingData(); nCountWithDescendants = 1; nSizeWithDescendants = nTxSize; nFeesWithDescendants = 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 6f7f7cfdaabd3..79f4b4d389e2c 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -87,7 +87,7 @@ class CTxMemPoolEntry CAmount nFeesWithDescendants; //! ... and total fees (all including us) 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 _spendsCoinbaseOrCoinstake, unsigned int nSigOps); diff --git a/src/validation.cpp b/src/validation.cpp index 541db0b899b75..c3ea1f3db5826 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -303,11 +303,12 @@ 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& _tx, bool fLimitFree, bool* pfMissingInputs, bool fOverrideMempoolLimit, bool fRejectAbsurdFee, bool ignoreFees, std::vector& coins_to_uncache) { AssertLockHeld(cs_main); + const CTransaction& tx = *_tx; if (pfMissingInputs) *pfMissingInputs = false; @@ -469,7 +470,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C } } - CTxMemPoolEntry entry(tx, nFees, GetTime(), dPriority, chainHeight, pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbaseOrCoinstake, nSigOps); + CTxMemPoolEntry entry(_tx, nFees, GetTime(), dPriority, chainHeight, pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbaseOrCoinstake, nSigOps); unsigned int nSize = entry.GetTxSize(); // Don't accept it if it can't get into a block @@ -581,7 +582,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C return true; } -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, bool fRejectAbsurdFee, bool fIgnoreFees) { std::vector coins_to_uncache; @@ -1889,15 +1890,14 @@ bool static DisconnectTip(CValidationState& state) return false; // Resurrect mempool transactions from the disconnected block. std::vector vHashUpdate; - for (const auto& txIn : block.vtx) { - const CTransaction& tx = *txIn; + for (const auto& tx : block.vtx) { // ignore validation errors in resurrected transactions std::list removed; CValidationState stateDummy; - if (tx.IsCoinBase() || tx.IsCoinStake() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, nullptr, true)) { - mempool.remove(tx, removed, true); - } else if (mempool.exists(tx.GetHash())) { - vHashUpdate.push_back(tx.GetHash()); + if (tx->IsCoinBase() || tx->IsCoinStake() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, nullptr, true)) { + mempool.remove(*tx, removed, true); + } else if (mempool.exists(tx->GetHash())) { + vHashUpdate.push_back(tx->GetHash()); } } // AcceptToMemoryPool/addUnchecked all assume that new mempool entries have diff --git a/src/validation.h b/src/validation.h index f055aff2df0c2..49efa892d1973 100644 --- a/src/validation.h +++ b/src/validation.h @@ -217,7 +217,7 @@ void FlushStateToDisk(); /** (try to) add transaction to memory pool **/ -bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState& state, const CTransaction& tx, bool fLimitFree, bool* pfMissingInputs, bool fOverrideMempoolLimit = false, bool fRejectInsaneFee = false, bool ignoreFees = false); +bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState& state, const CTransactionRef& tx, bool fLimitFree, bool* pfMissingInputs, bool fOverrideMempoolLimit = false, bool fRejectInsaneFee = false, bool ignoreFees = false); int GetIXConfirmations(uint256 nTXHash); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index fd0fde8878ee6..34838a18dc86d 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -337,7 +337,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CBlockIndex* pprev = nullptr) return fakeIndex; } -void fakeMempoolInsertion(const CWalletTx& wtxCredit) +void fakeMempoolInsertion(const CTransactionRef& wtxCredit) { CTxMemPoolEntry entry(wtxCredit, 0, 0, 0, 0, false, 0, false, 0); LOCK(mempool.cs); @@ -435,7 +435,7 @@ BOOST_AUTO_TEST_CASE(cached_balances_tests) ); // GetUnconfirmedBalance requires tx in mempool. - fakeMempoolInsertion(wtxCredit); + fakeMempoolInsertion(MakeTransactionRef(wtxCredit)); BOOST_CHECK_EQUAL(wallet.GetUnconfirmedBalance(), nCredit); // 2) Confirm tx and verify diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a3900cbc0586e..fa23c24532e04 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2870,7 +2870,7 @@ CWallet::CommitResult CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& // Try ATMP. This must not fail. The transaction has already been signed and recorded. CValidationState state; - if (!AcceptToMemoryPool(mempool, state, wtxNew, false, nullptr, false, true, false)) { + if (!AcceptToMemoryPool(mempool, state, MakeTransactionRef(std::move(wtxNew)), false, nullptr, false, true, false)) { res.state = state; // Abandon the transaction if (AbandonTransaction(res.hashTx)) { @@ -4035,7 +4035,7 @@ bool CMerkleTx::IsInMainChainImmature() const bool CMerkleTx::AcceptToMemoryPool(bool fLimitFree, bool fRejectInsaneFee, bool ignoreFees) { CValidationState state; - bool fAccepted = ::AcceptToMemoryPool(mempool, state, *this, fLimitFree, nullptr, false, fRejectInsaneFee, ignoreFees); + bool fAccepted = ::AcceptToMemoryPool(mempool, state, MakeTransactionRef(*this), fLimitFree, nullptr, false, fRejectInsaneFee, ignoreFees); if (!fAccepted) LogPrintf("%s : %s\n", __func__, state.GetRejectReason()); return fAccepted; From 9a809a2a9c3db781d7c120227f817742911371e6 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 12 Oct 2020 17:33:59 -0300 Subject: [PATCH 4/9] Add struct to track block-connect-time-generated info for callbacks. Coming from btc@6fdd43b968f984ef92ca4576872dc65462ba7312 --- src/validation.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c3ea1f3db5826..c77eb4520b584 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1922,11 +1922,20 @@ static int64_t nTimeFlush = 0; static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; +/** + * Used to track conflicted transactions removed from mempool and transactions + * applied to the UTXO state as a part of a single ActivateBestChainStep call. + */ +struct ConnectTrace { + std::list txConflicted; + std::vector> txChanged; +}; + /** * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. */ -bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CBlock* pblock, bool fAlreadyChecked, std::list &txConflicted, std::vector> &txChanged) +bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CBlock* pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) { assert(pindexNew->pprev == chainActive.Tip()); @@ -1975,12 +1984,12 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CB LogPrint(BCLog::BENCH, " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool. - mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload()); + mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, connectTrace.txConflicted, !IsInitialBlockDownload()); // Update chainActive & related variables. UpdateTip(pindexNew); for(unsigned int i=0; i < pblock->vtx.size(); i++) { - txChanged.emplace_back(*pblock->vtx[i], pindexNew, i); + connectTrace.txChanged.emplace_back(pblock->vtx[i], pindexNew, i); } int64_t nTime6 = GetTimeMicros(); @@ -2111,7 +2120,7 @@ static void PruneBlockIndexCandidates() * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const CBlock* pblock, bool fAlreadyChecked, std::list& txConflicted, std::vector>& txChanged) +static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const CBlock* pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); if (pblock == NULL) @@ -2147,7 +2156,7 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo // Connect new blocks. BOOST_REVERSE_FOREACH (CBlockIndex* pindexConnect, vpindexToConnect) { - if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, fAlreadyChecked, txConflicted, txChanged)) { + if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, fAlreadyChecked, connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) @@ -2202,15 +2211,11 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre CBlockIndex* pindexNewTip = nullptr; CBlockIndex* pindexMostWork = nullptr; - std::vector> txChanged; - if (pblock) - txChanged.reserve(pblock->vtx.size()); do { - txChanged.clear(); boost::this_thread::interruption_point(); const CBlockIndex *pindexFork; - std::list txConflicted; + ConnectTrace connectTrace; bool fInitialDownload; while (true) { TRY_LOCK(cs_main, lockMain); @@ -2226,7 +2231,7 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre if (pindexMostWork == NULL || pindexMostWork == chainActive.Tip()) return true; - if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fAlreadyChecked, txConflicted, txChanged)) + if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fAlreadyChecked, connectTrace)) return false; pindexNewTip = chainActive.Tip(); @@ -2234,12 +2239,12 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre fInitialDownload = IsInitialBlockDownload(); // throw all transactions though the signal-interface - for (const auto &tx : txConflicted) { + for (const auto &tx : connectTrace.txConflicted) { GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); } // ... and about transactions that got confirmed: - for (unsigned int i = 0; i < txChanged.size(); i++) { - GetMainSignals().SyncTransaction(std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i])); + for (unsigned int i = 0; i < connectTrace.txChanged.size(); i++) { + GetMainSignals().SyncTransaction(*std::get<0>(connectTrace.txChanged[i]), std::get<1>(connectTrace.txChanged[i]), std::get<2>(connectTrace.txChanged[i])); } break; From 140446c111f7504b473b80509ffb074a486be5e1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Oct 2016 12:12:50 -0400 Subject: [PATCH 5/9] Keep blocks as shared_ptrs, instead of copying txn in ConnectTip --- src/validation.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c77eb4520b584..1a3f3acf2fe32 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1928,7 +1928,7 @@ static int64_t nTimePostConnect = 0; */ struct ConnectTrace { std::list txConflicted; - std::vector> txChanged; + std::vector > > blocksConnected; }; /** @@ -1944,11 +1944,15 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CB // Read block from disk. int64_t nTime1 = GetTimeMicros(); - CBlock block; if (!pblock) { - if (!ReadBlockFromDisk(block, pindexNew)) + std::shared_ptr pblockNew = std::make_shared(); + connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew); + if (!ReadBlockFromDisk(*pblockNew, pindexNew)) return AbortNode(state, "Failed to read block"); - pblock = █ + pblock = pblockNew.get(); + } else { + //TODO: This copy is a major performance regression, but ProcessNewBlock callers need updated to fix this + connectTrace.blocksConnected.emplace_back(pindexNew, std::make_shared(*pblock)); } // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); @@ -1988,10 +1992,6 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CB // Update chainActive & related variables. UpdateTip(pindexNew); - for(unsigned int i=0; i < pblock->vtx.size(); i++) { - connectTrace.txChanged.emplace_back(pblock->vtx[i], pindexNew, i); - } - int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; @@ -2164,6 +2164,8 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo state = CValidationState(); fInvalidFound = true; fContinue = false; + // If we didn't actually connect the block, don't notify listeners about it + connectTrace.blocksConnected.pop_back(); break; } else { // A system error occurred (disk space, database error, ...). @@ -2243,8 +2245,12 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); } // ... and about transactions that got confirmed: - for (unsigned int i = 0; i < connectTrace.txChanged.size(); i++) { - GetMainSignals().SyncTransaction(*std::get<0>(connectTrace.txChanged[i]), std::get<1>(connectTrace.txChanged[i]), std::get<2>(connectTrace.txChanged[i])); + for (const auto& pair : connectTrace.blocksConnected) { + assert(pair.second); + const CBlock& block = *(pair.second); + for (unsigned int i = 0; i < block.vtx.size(); i++) { + GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i); + } } break; From 4cf65506ea139fd4b28f463e3371cb34d0d0297e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 25 Oct 2016 14:22:41 -0400 Subject: [PATCH 6/9] Create a shared_ptr for the block we're connecting in ActivateBCS --- src/validation.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1a3f3acf2fe32..821d7e27b119e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1935,7 +1935,7 @@ struct ConnectTrace { * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. */ -bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CBlock* pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) +bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) { assert(pindexNew->pprev == chainActive.Tip()); @@ -1949,11 +1949,10 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CB connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew); if (!ReadBlockFromDisk(*pblockNew, pindexNew)) return AbortNode(state, "Failed to read block"); - pblock = pblockNew.get(); } else { - //TODO: This copy is a major performance regression, but ProcessNewBlock callers need updated to fix this - connectTrace.blocksConnected.emplace_back(pindexNew, std::make_shared(*pblock)); + connectTrace.blocksConnected.emplace_back(pindexNew, pblock); } + const CBlock& blockConnecting = *connectTrace.blocksConnected.back().second; // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1; @@ -1961,8 +1960,8 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CB LogPrint(BCLog::BENCH, " - Load block from disk: %.2fms [%.2fs]\n", (nTime2 - nTime1) * 0.001, nTimeReadFromDisk * 0.000001); { CCoinsViewCache view(pcoinsTip); - bool rv = ConnectBlock(*pblock, state, pindexNew, view, false, fAlreadyChecked); - GetMainSignals().BlockChecked(*pblock, state); + bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, false, fAlreadyChecked); + GetMainSignals().BlockChecked(blockConnecting, state); if (!rv) { if (state.IsInvalid()) InvalidBlockFound(pindexNew, state); @@ -1988,7 +1987,7 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const CB LogPrint(BCLog::BENCH, " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool. - mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, connectTrace.txConflicted, !IsInitialBlockDownload()); + mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, connectTrace.txConflicted, !IsInitialBlockDownload()); // Update chainActive & related variables. UpdateTip(pindexNew); @@ -2156,7 +2155,8 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo // Connect new blocks. BOOST_REVERSE_FOREACH (CBlockIndex* pindexConnect, vpindexToConnect) { - if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, fAlreadyChecked, connectTrace)) { + //TODO: The pblock copy is a major performance regression, but callers need updated to fix this + if (!ConnectTip(state, pindexConnect, (pblock && pindexConnect == pindexMostWork) ? std::make_shared(*pblock) : std::shared_ptr(), fAlreadyChecked, connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) From 7b38f646c449788fb2b30049a8167e0eb95e56c8 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 12 Oct 2020 18:11:29 -0300 Subject: [PATCH 7/9] Make the optional pblock in ActivateBestChain a shared_ptr Coming from btc@2736c44c8edea5ce6a502a04269926fecda27301 --- src/rpc/blockchain.cpp | 4 ++-- src/validation.cpp | 17 +++++++++++------ src/validation.h | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index d3a659b50e335..027f4418d4928 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1184,7 +1184,7 @@ UniValue invalidateblock(const JSONRPCRequest& request) } if (state.IsValid()) { - ActivateBestChain(state, nullptr, false); + ActivateBestChain(state); budget.SetBestHeight(WITH_LOCK(cs_main, return chainActive.Height(); )); } @@ -1223,7 +1223,7 @@ UniValue reconsiderblock(const JSONRPCRequest& request) } if (state.IsValid()) { - ActivateBestChain(state, nullptr, false); + ActivateBestChain(state); budget.SetBestHeight(WITH_LOCK(cs_main, return chainActive.Height(); )); } diff --git a/src/validation.cpp b/src/validation.cpp index 821d7e27b119e..bfd243ba125e0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2119,7 +2119,7 @@ static void PruneBlockIndexCandidates() * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const CBlock* pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) +static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); if (pblock == NULL) @@ -2155,8 +2155,7 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo // Connect new blocks. BOOST_REVERSE_FOREACH (CBlockIndex* pindexConnect, vpindexToConnect) { - //TODO: The pblock copy is a major performance regression, but callers need updated to fix this - if (!ConnectTip(state, pindexConnect, (pblock && pindexConnect == pindexMostWork) ? std::make_shared(*pblock) : std::shared_ptr(), fAlreadyChecked, connectTrace)) { + if (!ConnectTip(state, pindexConnect, (pindexConnect == pindexMostWork) ? pblock : std::shared_ptr(), fAlreadyChecked, connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) @@ -2203,7 +2202,7 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo * that is already loaded (to avoid loading it again from disk). */ -bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlreadyChecked) +bool ActivateBestChain(CValidationState& state, std::shared_ptr pblock, bool fAlreadyChecked) { // Note that while we're often called here from ProcessNewBlock, this is // far from a guarantee. Things in the P2P/RPC will often end up calling @@ -2233,7 +2232,8 @@ bool ActivateBestChain(CValidationState& state, const CBlock* pblock, bool fAlre if (pindexMostWork == NULL || pindexMostWork == chainActive.Tip()) return true; - if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fAlreadyChecked, connectTrace)) + std::shared_ptr nullBlockPtr; + if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, connectTrace)) return false; pindexNewTip = chainActive.Tip(); @@ -3335,7 +3335,12 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const CBlock* pblock } } - if (!ActivateBestChain(state, pblock, checked)) + //TODO: This copy is a major performance regression, but callers need updated to fix this + std::shared_ptr block_ptr; + if (pblock) + block_ptr.reset(new CBlock(*pblock)); + + if (!ActivateBestChain(state, block_ptr, checked)) return error("%s : ActivateBestChain failed", __func__); if (!fLiteMode) { diff --git a/src/validation.h b/src/validation.h index 49efa892d1973..cefaa619ae5ce 100644 --- a/src/validation.h +++ b/src/validation.h @@ -207,7 +207,7 @@ double ConvertBitsToDouble(unsigned int nBits); int64_t GetMasternodePayment(); /** Find the best known block, and make it the tip of the block chain */ -bool ActivateBestChain(CValidationState& state, const CBlock* pblock = NULL, bool fAlreadyChecked = false); +bool ActivateBestChain(CValidationState& state, std::shared_ptr pblock = std::shared_ptr(), bool fAlreadyChecked = false); CAmount GetBlockValue(int nHeight); /** Create a new block index entry for a given block hash */ From 908ffd9a3047f628f6e801bce2742a83869beeb9 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 12 Oct 2020 18:36:38 -0300 Subject: [PATCH 8/9] Switch pblock in ProcessNewBlock to a shared_ptr Coming from btc@2d6e5619afa2d43a37a0a38daf33f58965ddfa80 --- src/miner.cpp | 8 ++++---- src/miner.h | 2 +- src/net_processing.cpp | 16 ++++++++-------- src/rpc/mining.cpp | 7 ++++--- src/validation.cpp | 15 ++++++--------- src/validation.h | 2 +- 6 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index f82a0a1a05302..743478112745e 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -470,7 +470,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, CWallet* pwallet, return pblocktemplate.release(); } -void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int& nExtraNonce) +void IncrementExtraNonce(std::shared_ptr& pblock, CBlockIndex* pindexPrev, unsigned int& nExtraNonce) { // Update nExtraNonce static uint256 hashPrevBlock; @@ -517,7 +517,7 @@ CBlockTemplate* CreateNewBlockWithKey(CReserveKey& reservekey, CWallet* pwallet) return CreateNewBlock(scriptPubKey, pwallet, false); } -bool ProcessBlockFound(CBlock* pblock, CWallet& wallet, Optional& reservekey) +bool ProcessBlockFound(const std::shared_ptr& pblock, CWallet& wallet, Optional& reservekey) { LogPrintf("%s\n", pblock->ToString()); LogPrintf("generated %s\n", FormatMoney(pblock->vtx[0]->vout[0].nValue)); @@ -628,7 +628,7 @@ void BitcoinMiner(CWallet* pwallet, bool fProofOfStake) CreateNewBlock(CScript(), pwallet, true, &availableCoins) : CreateNewBlockWithKey(*opReservekey, pwallet))); if (!pblocktemplate.get()) continue; - CBlock* pblock = &pblocktemplate->block; + std::shared_ptr pblock = std::make_shared(pblocktemplate->block); // POS - block found: process it if (fProofOfStake) { @@ -713,7 +713,7 @@ void BitcoinMiner(CWallet* pwallet, bool fProofOfStake) ) break; // Update nTime every few seconds - UpdateTime(pblock, pindexPrev); + UpdateTime(pblock.get(), pindexPrev); if (Params().GetConsensus().fPowAllowMinDifficultyBlocks) { // Changing pblock->nTime can change work required on testnet: hashTarget.SetCompact(pblock->nBits); diff --git a/src/miner.h b/src/miner.h index 8b9116a7ed7bf..9f77fd3e867b4 100644 --- a/src/miner.h +++ b/src/miner.h @@ -26,7 +26,7 @@ struct CBlockTemplate; /** Generate a new block, without valid proof-of-work */ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, CWallet* pwallet, bool fProofOfStake, std::vector* availableCoins = nullptr); /** Modify the extranonce in a block */ -void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int& nExtraNonce); +void IncrementExtraNonce(std::shared_ptr& pblock, CBlockIndex* pindexPrev, unsigned int& nExtraNonce); /** Check mined block */ void UpdateTime(CBlockHeader* block, const CBlockIndex* pindexPrev); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e0d337b5882ba..427f75a85266d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1581,18 +1581,18 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR else if (strCommand == NetMsgType::BLOCK && !fImporting && !fReindex) // Ignore blocks received while importing { - CBlock block; - vRecv >> block; - const uint256& hashBlock = block.GetHash(); + std::shared_ptr pblock = std::make_shared(); + vRecv >> *pblock; + const uint256& hashBlock = pblock->GetHash(); CInv inv(MSG_BLOCK, hashBlock); LogPrint(BCLog::NET, "received block %s peer=%d\n", inv.hash.ToString(), pfrom->id); //sometimes we will be sent their most recent block and its not the one we want, in that case tell where we are - if (!mapBlockIndex.count(block.hashPrevBlock)) { + if (!mapBlockIndex.count(pblock->hashPrevBlock)) { if (find(pfrom->vBlockRequested.begin(), pfrom->vBlockRequested.end(), hashBlock) != pfrom->vBlockRequested.end()) { //we already asked for this block, so lets work backwards and ask for the previous block - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), block.hashPrevBlock)); - pfrom->vBlockRequested.push_back(block.hashPrevBlock); + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), pblock->hashPrevBlock)); + pfrom->vBlockRequested.push_back(pblock->hashPrevBlock); } else { //ask to sync to this block connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), hashBlock)); @@ -1604,7 +1604,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR if (!mapBlockIndex.count(hashBlock)) { WITH_LOCK(cs_main, MarkBlockAsReceived(hashBlock); ); bool fAccepted = true; - ProcessNewBlock(state, pfrom, &block, nullptr, &fAccepted); + ProcessNewBlock(state, pfrom, pblock, nullptr, &fAccepted); if (!fAccepted) { CheckBlockSpam(state, pfrom, hashBlock); } @@ -1622,7 +1622,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR //disconnect this node if its old protocol version pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol(), strCommand); } else { - LogPrint(BCLog::NET, "%s : Already processed block %s, skipping ProcessNewBlock()\n", __func__, block.GetHash().GetHex()); + LogPrint(BCLog::NET, "%s : Already processed block %s, skipping ProcessNewBlock()\n", __func__, pblock->GetHash().GetHex()); } } } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 8637e043fb270..6f364d2913091 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -169,7 +169,7 @@ UniValue generate(const JSONRPCRequest& request) CreateNewBlock(CScript(), pwalletMain, true, &availableCoins) : CreateNewBlockWithKey(reservekey, pwalletMain)); if (!pblocktemplate.get()) break; - CBlock *pblock = &pblocktemplate->block; + std::shared_ptr pblock = std::make_shared(pblocktemplate->block); if(!fPoS) { { @@ -679,7 +679,8 @@ UniValue submitblock(const JSONRPCRequest& request) "\nExamples:\n" + HelpExampleCli("submitblock", "\"mydata\"") + HelpExampleRpc("submitblock", "\"mydata\"")); - CBlock block; + std::shared_ptr blockptr = std::make_shared(); + CBlock& block = *blockptr; if (!DecodeHexBlk(block, request.params[0].get_str())) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed"); @@ -706,7 +707,7 @@ UniValue submitblock(const JSONRPCRequest& request) CValidationState state; submitblock_StateCatcher sc(block.GetHash()); RegisterValidationInterface(&sc); - bool fAccepted = ProcessNewBlock(state, nullptr, &block, nullptr); + bool fAccepted = ProcessNewBlock(state, nullptr, blockptr, nullptr); UnregisterValidationInterface(&sc); if (fBlockPresent) { if (fAccepted && !sc.found) diff --git a/src/validation.cpp b/src/validation.cpp index bfd243ba125e0..fcd3ee47560c6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3291,7 +3291,7 @@ void CBlockIndex::BuildSkip() pskip = pprev->GetAncestor(GetSkipHeight(nHeight)); } -bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const CBlock* pblock, CDiskBlockPos* dbp, bool* fAccepted) +bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_ptr pblock, CDiskBlockPos* dbp, bool* fAccepted) { AssertLockNotHeld(cs_main); @@ -3335,12 +3335,7 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const CBlock* pblock } } - //TODO: This copy is a major performance regression, but callers need updated to fix this - std::shared_ptr block_ptr; - if (pblock) - block_ptr.reset(new CBlock(*pblock)); - - if (!ActivateBestChain(state, block_ptr, checked)) + if (!ActivateBestChain(state, pblock, checked)) return error("%s : ActivateBestChain failed", __func__); if (!fLiteMode) { @@ -3801,7 +3796,8 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos* dbp) // process in case the block isn't known yet if (mapBlockIndex.count(hash) == 0 || (mapBlockIndex[hash]->nStatus & BLOCK_HAVE_DATA) == 0) { CValidationState state; - if (ProcessNewBlock(state, nullptr, &block, dbp)) + std::shared_ptr block_ptr = std::make_shared(block); + if (ProcessNewBlock(state, nullptr, block_ptr, dbp)) nLoaded++; if (state.IsError()) break; @@ -3822,7 +3818,8 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos* dbp) LogPrintf("%s: Processing out of order child %s of %s\n", __func__, block.GetHash().ToString(), head.ToString()); CValidationState dummy; - if (ProcessNewBlock(dummy, nullptr, &block, &it->second)) { + std::shared_ptr block_ptr = std::make_shared(block); + if (ProcessNewBlock(dummy, nullptr, block_ptr, &it->second)) { nLoaded++; queue.push_back(block.GetHash()); } diff --git a/src/validation.h b/src/validation.h index cefaa619ae5ce..2220ca181d3dc 100644 --- a/src/validation.h +++ b/src/validation.h @@ -172,7 +172,7 @@ static const uint64_t nMinDiskSpace = 52428800; * @param[out] fAccepted Whether the block is accepted or not * @return True if state.IsValid() */ -bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const CBlock* pblock, CDiskBlockPos* dbp, bool* fAccepted = nullptr); +bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_ptr pblock, CDiskBlockPos* dbp, bool* fAccepted = nullptr); /** Check whether enough disk space is available for an incoming block */ bool CheckDiskSpace(uint64_t nAdditionalBytes = 0); /** Open a block file (blk?????.dat) */ From 0e59badd9558b281bdd1333fa6e204f502611c5a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 29 Nov 2016 21:25:39 -0800 Subject: [PATCH 9/9] Document ConnectBlock connectTrace postconditions --- src/validation.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index fcd3ee47560c6..b9958eaaa7971 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1934,6 +1934,10 @@ struct ConnectTrace { /** * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. + * + * The block is always added to connectTrace (either after loading from disk or by copying + * pblock) - if that is not intended, care must be taken to remove the last entry in + * blocksConnected in case of failure. */ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) {