Make removed and conflicted arguments optional to remove

PIVX-Project · Jan 18, 2021 · 4f672c2 · 4f672c2
1 parent e51c4b8
commit 4f672c2
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 34 deletions.
diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp
@@ -56,12 +56,12 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
     std::list<CTransactionRef> removed;
 
     // Nothing in pool, remove should do nothing:
-    testPool.removeRecursive(txParent, removed);
+    testPool.removeRecursive(txParent, &removed);
     BOOST_CHECK_EQUAL(removed.size(), 0);
 
     // Just the parent:
     testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent));
-    testPool.removeRecursive(txParent, removed);
+    testPool.removeRecursive(txParent, &removed);
     BOOST_CHECK_EQUAL(removed.size(), 1);
     removed.clear();
 
@@ -73,16 +73,16 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
         testPool.addUnchecked(txGrandChild[i].GetHash(), entry.FromTx(txGrandChild[i]));
     }
     // Remove Child[0], GrandChild[0] should be removed:
-    testPool.removeRecursive(txChild[0], removed);
+    testPool.removeRecursive(txChild[0], &removed);
     BOOST_CHECK_EQUAL(removed.size(), 2);
     removed.clear();
     // ... make sure grandchild and child are gone:
-    testPool.removeRecursive(txGrandChild[0], removed);
+    testPool.removeRecursive(txGrandChild[0], &removed);
     BOOST_CHECK_EQUAL(removed.size(), 0);
-    testPool.removeRecursive(txChild[0], removed);
+    testPool.removeRecursive(txChild[0], &removed);
     BOOST_CHECK_EQUAL(removed.size(), 0);
     // Remove parent, all children/grandchildren should go:
-    testPool.removeRecursive(txParent, removed);
+    testPool.removeRecursive(txParent, &removed);
     BOOST_CHECK_EQUAL(removed.size(), 5);
     BOOST_CHECK_EQUAL(testPool.size(), 0);
     removed.clear();
@@ -95,7 +95,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
     }
     // Now remove the parent, as might happen if a block-re-org occurs but the parent cannot be
     // put into the mempool (maybe because it is non-standard):
-    testPool.removeRecursive(txParent, removed);
+    testPool.removeRecursive(txParent, &removed);
     BOOST_CHECK_EQUAL(removed.size(), 6);
     BOOST_CHECK_EQUAL(testPool.size(), 0);
     removed.clear();
@@ -279,12 +279,11 @@ 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<CTransactionRef> removed;
-    pool.removeRecursive(pool.mapTx.find(tx10.GetHash())->GetTx(), removed);
+    pool.removeRecursive(pool.mapTx.find(tx10.GetHash())->GetTx());
     CheckSort<descendant_score>(pool, snapshotOrder);
 
-    pool.removeRecursive(pool.mapTx.find(tx9.GetHash())->GetTx(), removed);
-    pool.removeRecursive(pool.mapTx.find(tx8.GetHash())->GetTx(), removed);
+    pool.removeRecursive(pool.mapTx.find(tx9.GetHash())->GetTx());
+    pool.removeRecursive(pool.mapTx.find(tx8.GetHash())->GetTx());
     /* Now check the sort on the mining score index.
      * Final order should be:
      *
@@ -416,8 +415,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
     /* after tx6 is mined, tx7 should move up in the sort */
     std::vector<CTransactionRef> vtx;
     vtx.emplace_back(MakeTransactionRef(tx6));
-    std::list<CTransactionRef> dummy;
-    pool.removeForBlock(vtx, 1, dummy, false);
+    pool.removeForBlock(vtx, 1, nullptr, false);
 
     sortedOrder.erase(sortedOrder.begin()+1);
     // Ties are broken by hash
@@ -556,12 +554,11 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
     pool.addUnchecked(tx7.GetHash(), entry.Fee(9000LL).FromTx(tx7, &pool));
 
     std::vector<CTransactionRef> vtx;
-    std::list<CTransactionRef> conflicts;
     SetMockTime(42);
     SetMockTime(42 + CTxMemPool::ROLLING_FEE_HALFLIFE);
     BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + 1000);
     // ... we should keep the same min fee until we get a block
-    pool.removeForBlock(vtx, 1, conflicts);
+    pool.removeForBlock(vtx, 1);
     SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE);
     BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/2);
     // ... then feerate should drop 1/2 each halflife

diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp
@@ -38,7 +38,6 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
     for (unsigned int i = 0; i < 128; i++)
         garbage.push_back('X');
     CMutableTransaction tx;
-    std::list<CTransactionRef> dummyConflicted;
     tx.vin.resize(1);
     tx.vin[0].scriptSig = garbage;
     tx.vout.resize(1);
@@ -73,7 +72,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
                 txHashes[9-h].pop_back();
             }
         }
-        mpool.removeForBlock(block, ++blocknum, dummyConflicted);
+        mpool.removeForBlock(block, ++blocknum);
         block.clear();
         if (blocknum == 30) {
             // At this point we should need to combine 5 buckets to get enough data points
@@ -112,7 +111,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
     // Mine 50 more blocks with no transactions happening, estimates shouldn't change
     // We haven't decayed the moving average enough so we still have enough data points in every bucket
     while (blocknum < 250)
-        mpool.removeForBlock(block, ++blocknum, dummyConflicted);
+        mpool.removeForBlock(block, ++blocknum);
 
     for (int i = 1; i < 10;i++) {
         BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() < origFeeEst[i-1] + deltaFee);
@@ -131,7 +130,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
                 txHashes[j].push_back(hash);
             }
         }
-        mpool.removeForBlock(block, ++blocknum, dummyConflicted);
+        mpool.removeForBlock(block, ++blocknum);
     }
 
     int answerFound;
@@ -150,7 +149,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
             txHashes[j].pop_back();
         }
     }
-    mpool.removeForBlock(block, 265, dummyConflicted);
+    mpool.removeForBlock(block, 265);
     block.clear();
     for (int i = 1; i < 10;i++) {
         BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee);
@@ -169,7 +168,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
                     block.emplace_back(ptx);
             }
         }
-        mpool.removeForBlock(block, ++blocknum, dummyConflicted);
+        mpool.removeForBlock(block, ++blocknum);
         block.clear();
     }
     for (int i = 1; i < 10; i++) {

diff --git a/src/txmempool.cpp b/src/txmempool.cpp
@@ -489,7 +489,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants
     }
 }
 
-void CTxMemPool::removeRecursive(const CTransaction& origTx, std::list<CTransactionRef>& removed)
+void CTxMemPool::removeRecursive(const CTransaction& origTx, std::list<CTransactionRef>* removed)
 {
     // Remove transaction from memory pool
     {
@@ -516,8 +516,10 @@ void CTxMemPool::removeRecursive(const CTransaction& origTx, std::list<CTransact
         for (const txiter& it : txToRemove) {
             CalculateDescendants(it, setAllRemoves);
         }
-        for (const txiter& it : setAllRemoves) {
-            removed.emplace_back(it->GetSharedTx());
+        if (removed) {
+            for (const txiter& it : setAllRemoves) {
+                removed->emplace_back(it->GetSharedTx());
+            }
         }
         RemoveStaged(setAllRemoves, false);
     }
@@ -574,12 +576,11 @@ void CTxMemPool::removeWithAnchor(const uint256& invalidRoot)
         }
     }
     for (const CTransaction& tx : transactionsToRemove) {
-        std::list<CTransactionRef> removed;
-        removeRecursive(tx, removed);
+        removeRecursive(tx);
     }
 }
 
-void CTxMemPool::removeConflicts(const CTransaction& tx, std::list<CTransactionRef>& removed)
+void CTxMemPool::removeConflicts(const CTransaction& tx, std::list<CTransactionRef>* removed)
 {
     // Remove transactions which depend on inputs of tx, recursively
     std::list<CTransaction> result;
@@ -610,7 +611,8 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::list<CTransactionR
 /**
  * Called when a block is connected. Removes from mempool and updates the miner fee estimator.
  */
-void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight, std::list<CTransactionRef>& conflicts, bool fCurrentEstimate)
+void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight,
+                                std::list<CTransactionRef>* conflicts, bool fCurrentEstimate)
 {
     LOCK(cs);
     std::vector<CTxMemPoolEntry> entries;

diff --git a/src/txmempool.h b/src/txmempool.h
@@ -520,11 +520,12 @@ 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 removeRecursive(const CTransaction& tx, std::list<CTransactionRef>& removed);
+    void removeRecursive(const CTransaction& tx, std::list<CTransactionRef>* removed = nullptr);
     void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags);
     void removeWithAnchor(const uint256& invalidRoot);
-    void removeConflicts(const CTransaction& tx, std::list<CTransactionRef>& removed);
-    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight, std::list<CTransactionRef>& conflicts, bool fCurrentEstimate = true);
+    void removeConflicts(const CTransaction& tx, std::list<CTransactionRef>* removed = nullptr);
+    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight,
+                        std::list<CTransactionRef>* conflicts = nullptr, bool fCurrentEstimate = true);
     void clear();
     void _clear();  // lock-free
     bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb);

diff --git a/src/validation.cpp b/src/validation.cpp
@@ -1966,10 +1966,9 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
     std::vector<uint256> vHashUpdate;
     for (const auto& tx : block.vtx) {
         // ignore validation errors in resurrected transactions
-        std::list<CTransactionRef> removed;
         CValidationState stateDummy;
         if (tx->IsCoinBase() || tx->IsCoinStake() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, nullptr, true)) {
-            mempool.removeRecursive(*tx, removed);
+            mempool.removeRecursive(*tx);
         } else if (mempool.exists(tx->GetHash())) {
             vHashUpdate.push_back(tx->GetHash());
         }
@@ -2086,7 +2085,7 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st
     LogPrint(BCLog::BENCH, "  - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001);
 
     // Remove conflicting transactions from the mempool.
-    mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, connectTrace.txConflicted, !IsInitialBlockDownload());
+    mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, &connectTrace.txConflicted, !IsInitialBlockDownload());
     // Update chainActive & related variables.
     UpdateTip(pindexNew);
     // Update MN manager cache