From f2f7f25be7d145a36e6a400df515b6fff9330c29 Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Mon, 15 Nov 2021 08:52:02 +0200 Subject: [PATCH 1/3] Rework mempool accounts view Signed-off-by: Anthony Fieroni --- src/txmempool.cpp | 110 +++++++++++++++----------------- src/txmempool.h | 2 + src/validation.cpp | 5 +- test/functional/rpc_mn_basic.py | 6 +- 4 files changed, 59 insertions(+), 64 deletions(-) mode change 100755 => 100644 test/functional/rpc_mn_basic.py diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e3686020c9..7bbbb40687 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -333,6 +333,7 @@ CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) // accepting transactions becomes O(N^2) where N is the number // of transactions in the pool nCheckFrequency = 0; + accountsViewDirty = false; } bool CTxMemPool::isSpent(const COutPoint& outpoint) const @@ -570,61 +571,27 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne { AssertLockHeld(cs); - setEntries staged; std::vector entries; for (const auto& tx : vtx) { auto it = mapTx.find(tx->GetHash()); if (it != mapTx.end()) { - staged.insert(it); entries.push_back(&*it); } } // Before the txs in the new block have been removed from the mempool, update policy estimates if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);} - for (auto& it : staged) { - auto& tx = it->GetTx(); - removeConflicts(tx); - ClearPrioritisation(tx.GetHash()); - } - - RemoveStaged(staged, true, MemPoolRemovalReason::BLOCK); - - if (pcustomcsview) { // can happen in tests - // check entire mempool - CAmount txfee = 0; - accountsView().Discard(); - CCustomCSView viewDuplicate(accountsView()); - CCoinsViewCache mempoolDuplicate(&::ChainstateActive().CoinsTip()); - - setEntries staged; - // Check custom TX consensus types are now not in conflict with account layer - auto& txsByEntryTime = mapTx.get(); - for (auto it = txsByEntryTime.begin(); it != txsByEntryTime.end(); ++it) { - CValidationState state; - const auto& tx = it->GetTx(); - if (!Consensus::CheckTxInputs(tx, state, mempoolDuplicate, &viewDuplicate, nBlockHeight, txfee, Params())) { - LogPrintf("%s: Remove conflicting TX: %s\n", __func__, tx.GetHash().GetHex()); - staged.insert(mapTx.project<0>(it)); - continue; - } - auto res = ApplyCustomTx(viewDuplicate, mempoolDuplicate, tx, Params().GetConsensus(), nBlockHeight); - if (!res.ok && (res.code & CustomTxErrCodes::Fatal)) { - LogPrintf("%s: Remove conflicting custom TX: %s\n", __func__, tx.GetHash().GetHex()); - staged.insert(mapTx.project<0>(it)); - } - } - - for (auto& it : staged) { - auto& tx = it->GetTx(); - removeConflicts(tx); - ClearPrioritisation(tx.GetHash()); + for (const auto& tx : vtx) { + auto it = mapTx.find(tx->GetHash()); + if (it != mapTx.end()) { + RemoveStaged({it}, true, MemPoolRemovalReason::BLOCK); } - - RemoveStaged(staged, true, MemPoolRemovalReason::BLOCK); - viewDuplicate.Flush(); + removeConflicts(*tx); + ClearPrioritisation(tx->GetHash()); } + rebuildAccountsView(nBlockHeight); + lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } @@ -975,27 +942,10 @@ size_t CTxMemPool::DynamicMemoryUsage() const { void CTxMemPool::RemoveStaged(const setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason) { AssertLockHeld(cs); UpdateForRemoveFromMempool(stage, updateDescendants); - std::set txids; for (txiter it : stage) { - txids.insert(it->GetTx().GetHash()); removeUnchecked(it, reason); } - if (pcustomcsview && !txids.empty()) { - auto& view = accountsView(); - std::map orderedTxs; - auto it = NewKVIterator(UndoKey{}, view.GetStorage().GetRaw()); - for (; it.Valid() && !txids.empty(); it.Next()) { - auto& key = it.Key(); - auto itTx = txids.find(key.txid); - if (itTx != txids.end()) { - orderedTxs.emplace(key.height, key.txid); - txids.erase(itTx); - } - } - for (auto it = orderedTxs.rbegin(); it != orderedTxs.rend(); ++it) { - view.OnUndoTx(it->second, it->first); - } - } + accountsViewDirty = accountsViewDirty || !stage.empty(); } int CTxMemPool::Expire(int64_t time) { @@ -1134,6 +1084,46 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends } } +void CTxMemPool::rebuildAccountsView(int height) +{ + if (!pcustomcsview || !accountsViewDirty) { + return; + } + + CAmount txfee = 0; + accountsView().Discard(); + CCustomCSView viewDuplicate(accountsView()); + CCoinsViewCache mempoolDuplicate(&::ChainstateActive().CoinsTip()); + + setEntries staged; + // Check custom TX consensus types are now not in conflict with account layer + auto& txsByEntryTime = mapTx.get(); + for (auto it = txsByEntryTime.begin(); it != txsByEntryTime.end(); ++it) { + CValidationState state; + const auto& tx = it->GetTx(); + if (!Consensus::CheckTxInputs(tx, state, mempoolDuplicate, &viewDuplicate, height, txfee, Params())) { + LogPrintf("%s: Remove conflicting TX: %s\n", __func__, tx.GetHash().GetHex()); + staged.insert(mapTx.project<0>(it)); + continue; + } + auto res = ApplyCustomTx(viewDuplicate, mempoolDuplicate, tx, Params().GetConsensus(), height); + if (!res && (res.code & CustomTxErrCodes::Fatal)) { + LogPrintf("%s: Remove conflicting custom TX: %s\n", __func__, tx.GetHash().GetHex()); + staged.insert(mapTx.project<0>(it)); + } + } + + for (const auto& it : staged) { + auto tx = it->GetSharedTx(); + RemoveStaged({it}, true, MemPoolRemovalReason::BLOCK); + removeConflicts(*tx); + ClearPrioritisation(tx->GetHash()); + } + + viewDuplicate.Flush(); + accountsViewDirty = false; +} + uint64_t CTxMemPool::CalculateDescendantMaximum(txiter entry) const { // find parent with highest descendant count std::vector candidates; diff --git a/src/txmempool.h b/src/txmempool.h index 264e5f0b41..8be6620370 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -548,6 +548,7 @@ class CTxMemPool std::vector GetSortedDepthAndScore() const EXCLUSIVE_LOCKS_REQUIRED(cs); + bool accountsViewDirty; std::unique_ptr acview; public: indirectmap mapNextTx GUARDED_BY(cs); @@ -704,6 +705,7 @@ class CTxMemPool boost::signals2::signal NotifyEntryRemoved; CCustomCSView& accountsView(); + void rebuildAccountsView(int height); private: /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update * the descendants for a single transaction that has been added to the diff --git a/src/validation.cpp b/src/validation.cpp index 2ff3ad4634..f9508ae7b9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -612,7 +612,8 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const auto height = GetSpendHeight(view); - // it does not need to check mempool anymore it has view there + // rebuild accounts view if dirty + pool.rebuildAccountsView(height); CAmount nFees = 0; if (!Consensus::CheckTxInputs(tx, state, view, &mnview, height, nFees, chainparams)) { @@ -910,6 +911,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Store transaction in memory pool.addUnchecked(entry, setAncestors, validForFeeEstimation); + mnview.Flush(); // trim mempool and check if tx was trimmed if (!bypass_limits) { @@ -917,7 +919,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool if (!pool.exists(hash)) return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool full"); } - mnview.Flush(); } GetMainSignals().TransactionAddedToMempool(ptx); diff --git a/test/functional/rpc_mn_basic.py b/test/functional/rpc_mn_basic.py old mode 100755 new mode 100644 index b5e86d726d..509f133580 --- a/test/functional/rpc_mn_basic.py +++ b/test/functional/rpc_mn_basic.py @@ -148,9 +148,11 @@ def run_test(self): self.nodes[2].generate(35) connect_nodes_bi(self.nodes, 0, 2) self.sync_blocks(self.nodes[0:3]) + assert_equal(len(self.nodes[0].listmasternodes()), 8) - # fundingTx is removed for a block - assert_equal(len(self.nodes[0].getrawmempool()), 1) # auto auth + mempool = self.nodes[0].getrawmempool() + assert(idnode0 in mempool and fundingTx in mempool) + assert_equal(len(mempool), 3) # + auto auth collateral0 = self.nodes[0].getnewaddress("", "legacy") self.nodes[0].createmasternode(collateral0) From e4dc103b1f6941ff3a7892052022a3d1ee42495c Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Tue, 16 Nov 2021 15:40:59 +0200 Subject: [PATCH 2/3] Do not reuse iterators in recreating mempool view Signed-off-by: Anthony Fieroni --- src/txmempool.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 7bbbb40687..2b797f848f 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1096,6 +1096,7 @@ void CTxMemPool::rebuildAccountsView(int height) CCoinsViewCache mempoolDuplicate(&::ChainstateActive().CoinsTip()); setEntries staged; + std::vector vtx; // Check custom TX consensus types are now not in conflict with account layer auto& txsByEntryTime = mapTx.get(); for (auto it = txsByEntryTime.begin(); it != txsByEntryTime.end(); ++it) { @@ -1104,18 +1105,20 @@ void CTxMemPool::rebuildAccountsView(int height) if (!Consensus::CheckTxInputs(tx, state, mempoolDuplicate, &viewDuplicate, height, txfee, Params())) { LogPrintf("%s: Remove conflicting TX: %s\n", __func__, tx.GetHash().GetHex()); staged.insert(mapTx.project<0>(it)); + vtx.push_back(it->GetSharedTx()); continue; } auto res = ApplyCustomTx(viewDuplicate, mempoolDuplicate, tx, Params().GetConsensus(), height); if (!res && (res.code & CustomTxErrCodes::Fatal)) { LogPrintf("%s: Remove conflicting custom TX: %s\n", __func__, tx.GetHash().GetHex()); staged.insert(mapTx.project<0>(it)); + vtx.push_back(it->GetSharedTx()); } } - for (const auto& it : staged) { - auto tx = it->GetSharedTx(); - RemoveStaged({it}, true, MemPoolRemovalReason::BLOCK); + RemoveStaged(staged, true, MemPoolRemovalReason::BLOCK); + + for (const auto& tx : vtx) { removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } From f4fe2cacbe7d9ca22e79de7a8c0b14be2fba1023 Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Wed, 22 Dec 2021 16:37:17 +0200 Subject: [PATCH 3/3] Use consistent coins cache in rebuild account view Signed-off-by: Anthony Fieroni --- src/txmempool.cpp | 11 ++++++----- src/txmempool.h | 2 +- src/validation.cpp | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 2b797f848f..94314c392c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -590,7 +590,9 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne ClearPrioritisation(tx->GetHash()); } - rebuildAccountsView(nBlockHeight); + if (pcustomcsview) { + rebuildAccountsView(nBlockHeight, &::ChainstateActive().CoinsTip()); + } lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; @@ -1084,7 +1086,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends } } -void CTxMemPool::rebuildAccountsView(int height) +void CTxMemPool::rebuildAccountsView(int height, const CCoinsViewCache& coinsCache) { if (!pcustomcsview || !accountsViewDirty) { return; @@ -1093,7 +1095,6 @@ void CTxMemPool::rebuildAccountsView(int height) CAmount txfee = 0; accountsView().Discard(); CCustomCSView viewDuplicate(accountsView()); - CCoinsViewCache mempoolDuplicate(&::ChainstateActive().CoinsTip()); setEntries staged; std::vector vtx; @@ -1102,13 +1103,13 @@ void CTxMemPool::rebuildAccountsView(int height) for (auto it = txsByEntryTime.begin(); it != txsByEntryTime.end(); ++it) { CValidationState state; const auto& tx = it->GetTx(); - if (!Consensus::CheckTxInputs(tx, state, mempoolDuplicate, &viewDuplicate, height, txfee, Params())) { + if (!Consensus::CheckTxInputs(tx, state, coinsCache, &viewDuplicate, height, txfee, Params())) { LogPrintf("%s: Remove conflicting TX: %s\n", __func__, tx.GetHash().GetHex()); staged.insert(mapTx.project<0>(it)); vtx.push_back(it->GetSharedTx()); continue; } - auto res = ApplyCustomTx(viewDuplicate, mempoolDuplicate, tx, Params().GetConsensus(), height); + auto res = ApplyCustomTx(viewDuplicate, coinsCache, tx, Params().GetConsensus(), height); if (!res && (res.code & CustomTxErrCodes::Fatal)) { LogPrintf("%s: Remove conflicting custom TX: %s\n", __func__, tx.GetHash().GetHex()); staged.insert(mapTx.project<0>(it)); diff --git a/src/txmempool.h b/src/txmempool.h index 8be6620370..edfbdc8f0d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -705,7 +705,7 @@ class CTxMemPool boost::signals2::signal NotifyEntryRemoved; CCustomCSView& accountsView(); - void rebuildAccountsView(int height); + void rebuildAccountsView(int height, const CCoinsViewCache& coinsCache); private: /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update * the descendants for a single transaction that has been added to the diff --git a/src/validation.cpp b/src/validation.cpp index 46db6e1f11..433a5065d1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -614,7 +614,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const auto height = GetSpendHeight(view); // rebuild accounts view if dirty - pool.rebuildAccountsView(height); + pool.rebuildAccountsView(height, view); CAmount nFees = 0; if (!Consensus::CheckTxInputs(tx, state, view, &mnview, height, nFees, chainparams)) {