Skip to content

Commit 9bdf526

Browse files
laanwjlaanwj
committed
Merge #8515: A few mempool removal optimizations
0334430 Add some missing includes (Pieter Wuille) 4100499 Return shared_ptr<CTransaction> from mempool removes (Pieter Wuille) 51f2783 Make removed and conflicted arguments optional to remove (Pieter Wuille) f48211b Bypass removeRecursive in removeForReorg (Pieter Wuille)
2 parents e077e00 + 0334430 commit 9bdf526

File tree

7 files changed

+51
-50
lines changed

7 files changed

+51
-50
lines changed

‎src/main.cpp

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2768,10 +2768,9 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
27682768
std::vector<uint256> vHashUpdate;
27692769
BOOST_FOREACH(const CTransaction &tx, block.vtx) {
27702770
// ignore validation errors in resurrected transactions
2771-
list<CTransaction> removed;
27722771
CValidationState stateDummy;
27732772
if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, true)) {
2774-
mempool.removeRecursive(tx, removed);
2773+
mempool.removeRecursive(tx);
27752774
} else if (mempool.exists(tx.GetHash())) {
27762775
vHashUpdate.push_back(tx.GetHash());
27772776
}
@@ -2804,7 +2803,7 @@ static int64_t nTimePostConnect = 0;
28042803
* Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock
28052804
* corresponding to pindexNew, to bypass loading it again from disk.
28062805
*/
2807-
bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock, std::list<CTransaction> &txConflicted, std::vector<std::tuple<CTransaction,CBlockIndex*,int>> &txChanged)
2806+
bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock, std::vector<std::shared_ptr<const CTransaction>> &txConflicted, std::vector<std::tuple<CTransaction,CBlockIndex*,int>> &txChanged)
28082807
{
28092808
assert(pindexNew->pprev == chainActive.Tip());
28102809
// Read block from disk.
@@ -2840,7 +2839,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
28402839
int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4;
28412840
LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001);
28422841
// Remove conflicting transactions from the mempool.;
2843-
mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload());
2842+
mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, &txConflicted, !IsInitialBlockDownload());
28442843
// Update chainActive & related variables.
28452844
UpdateTip(pindexNew, chainparams);
28462845

@@ -2927,7 +2926,7 @@ static void PruneBlockIndexCandidates() {
29272926
* Try to make some progress towards making pindexMostWork the active block.
29282927
* pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork.
29292928
*/
2930-
static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound, std::list<CTransaction>& txConflicted, std::vector<std::tuple<CTransaction,CBlockIndex*,int>>& txChanged)
2929+
static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound, std::vector<std::shared_ptr<const CTransaction>>& txConflicted, std::vector<std::tuple<CTransaction,CBlockIndex*,int>>& txChanged)
29312930
{
29322931
AssertLockHeld(cs_main);
29332932
const CBlockIndex *pindexOldTip = chainActive.Tip();
@@ -3038,7 +3037,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
30383037
break;
30393038

30403039
const CBlockIndex *pindexFork;
3041-
std::list<CTransaction> txConflicted;
3040+
std::vector<std::shared_ptr<const CTransaction>> txConflicted;
30423041
bool fInitialDownload;
30433042
{
30443043
LOCK(cs_main);
@@ -3069,9 +3068,9 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
30693068

30703069
// throw all transactions though the signal-interface
30713070
// while _not_ holding the cs_main lock
3072-
BOOST_FOREACH(const CTransaction &tx, txConflicted)
3071+
for(std::shared_ptr<const CTransaction> tx : txConflicted)
30733072
{
3074-
GetMainSignals().SyncTransaction(tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
3073+
GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
30753074
}
30763075
// ... and about transactions that got confirmed:
30773076
for(unsigned int i = 0; i < txChanged.size(); i++)

‎src/test/blockencodings_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
8080

8181
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2].GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
8282

83-
std::list<CTransaction> removed;
84-
pool.removeRecursive(block.vtx[2], removed);
83+
std::vector<std::shared_ptr<const CTransaction>> removed;
84+
pool.removeRecursive(block.vtx[2], &removed);
8585
BOOST_CHECK_EQUAL(removed.size(), 1);
8686

8787
CBlock block2;

‎src/test/mempool_tests.cpp

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,15 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
5555

5656

5757
CTxMemPool testPool(CFeeRate(0));
58-
std::list<CTransaction> removed;
58+
std::vector<std::shared_ptr<const CTransaction>> removed;
5959

6060
// Nothing in pool, remove should do nothing:
61-
testPool.removeRecursive(txParent, removed);
61+
testPool.removeRecursive(txParent, &removed);
6262
BOOST_CHECK_EQUAL(removed.size(), 0);
6363

6464
// Just the parent:
6565
testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent));
66-
testPool.removeRecursive(txParent, removed);
66+
testPool.removeRecursive(txParent, &removed);
6767
BOOST_CHECK_EQUAL(removed.size(), 1);
6868
removed.clear();
6969

@@ -75,16 +75,16 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
7575
testPool.addUnchecked(txGrandChild[i].GetHash(), entry.FromTx(txGrandChild[i]));
7676
}
7777
// Remove Child[0], GrandChild[0] should be removed:
78-
testPool.removeRecursive(txChild[0], removed);
78+
testPool.removeRecursive(txChild[0], &removed);
7979
BOOST_CHECK_EQUAL(removed.size(), 2);
8080
removed.clear();
8181
// ... make sure grandchild and child are gone:
82-
testPool.removeRecursive(txGrandChild[0], removed);
82+
testPool.removeRecursive(txGrandChild[0], &removed);
8383
BOOST_CHECK_EQUAL(removed.size(), 0);
84-
testPool.removeRecursive(txChild[0], removed);
84+
testPool.removeRecursive(txChild[0], &removed);
8585
BOOST_CHECK_EQUAL(removed.size(), 0);
8686
// Remove parent, all children/grandchildren should go:
87-
testPool.removeRecursive(txParent, removed);
87+
testPool.removeRecursive(txParent, &removed);
8888
BOOST_CHECK_EQUAL(removed.size(), 5);
8989
BOOST_CHECK_EQUAL(testPool.size(), 0);
9090
removed.clear();
@@ -97,7 +97,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
9797
}
9898
// Now remove the parent, as might happen if a block-re-org occurs but the parent cannot be
9999
// put into the mempool (maybe because it is non-standard):
100-
testPool.removeRecursive(txParent, removed);
100+
testPool.removeRecursive(txParent, &removed);
101101
BOOST_CHECK_EQUAL(removed.size(), 6);
102102
BOOST_CHECK_EQUAL(testPool.size(), 0);
103103
removed.clear();
@@ -281,12 +281,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
281281
BOOST_CHECK_EQUAL(pool.size(), 10);
282282

283283
// Now try removing tx10 and verify the sort order returns to normal
284-
std::list<CTransaction> removed;
285-
pool.removeRecursive(pool.mapTx.find(tx10.GetHash())->GetTx(), removed);
284+
pool.removeRecursive(pool.mapTx.find(tx10.GetHash())->GetTx());
286285
CheckSort<descendant_score>(pool, snapshotOrder);
287286

288-
pool.removeRecursive(pool.mapTx.find(tx9.GetHash())->GetTx(), removed);
289-
pool.removeRecursive(pool.mapTx.find(tx8.GetHash())->GetTx(), removed);
287+
pool.removeRecursive(pool.mapTx.find(tx9.GetHash())->GetTx());
288+
pool.removeRecursive(pool.mapTx.find(tx8.GetHash())->GetTx());
290289
/* Now check the sort on the mining score index.
291290
* Final order should be:
292291
*
@@ -413,8 +412,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
413412
/* after tx6 is mined, tx7 should move up in the sort */
414413
std::vector<CTransaction> vtx;
415414
vtx.push_back(tx6);
416-
std::list<CTransaction> dummy;
417-
pool.removeForBlock(vtx, 1, dummy, false);
415+
pool.removeForBlock(vtx, 1, NULL, false);
418416

419417
sortedOrder.erase(sortedOrder.begin()+1);
420418
sortedOrder.pop_back();
@@ -549,12 +547,12 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
549547
pool.addUnchecked(tx7.GetHash(), entry.Fee(9000LL).FromTx(tx7, &pool));
550548

551549
std::vector<CTransaction> vtx;
552-
std::list<CTransaction> conflicts;
550+
std::vector<std::shared_ptr<const CTransaction>> conflicts;
553551
SetMockTime(42);
554552
SetMockTime(42 + CTxMemPool::ROLLING_FEE_HALFLIFE);
555553
BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + 1000);
556554
// ... we should keep the same min fee until we get a block
557-
pool.removeForBlock(vtx, 1, conflicts);
555+
pool.removeForBlock(vtx, 1);
558556
SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE);
559557
BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/2);
560558
// ... then feerate should drop 1/2 each halflife

‎src/test/miner_tests.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,7 @@ void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey,
137137
// Test that packages above the min relay fee do get included, even if one
138138
// of the transactions is below the min relay fee
139139
// Remove the low fee transaction and replace with a higher fee transaction
140-
std::list<CTransaction> dummy;
141-
mempool.removeRecursive(tx, dummy);
140+
mempool.removeRecursive(tx);
142141
tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee
143142
hashLowFeeTx = tx.GetHash();
144143
mempool.addUnchecked(hashLowFeeTx, entry.Fee(feeToUse+2).FromTx(tx));

‎src/test/policyestimator_tests.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
4646
for (unsigned int i = 0; i < 128; i++)
4747
garbage.push_back('X');
4848
CMutableTransaction tx;
49-
std::list<CTransaction> dummyConflicted;
5049
tx.vin.resize(1);
5150
tx.vin[0].scriptSig = garbage;
5251
tx.vout.resize(1);
@@ -81,7 +80,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
8180
txHashes[9-h].pop_back();
8281
}
8382
}
84-
mpool.removeForBlock(block, ++blocknum, dummyConflicted);
83+
mpool.removeForBlock(block, ++blocknum);
8584
block.clear();
8685
if (blocknum == 30) {
8786
// At this point we should need to combine 5 buckets to get enough data points
@@ -125,7 +124,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
125124
// Mine 50 more blocks with no transactions happening, estimates shouldn't change
126125
// We haven't decayed the moving average enough so we still have enough data points in every bucket
127126
while (blocknum < 250)
128-
mpool.removeForBlock(block, ++blocknum, dummyConflicted);
127+
mpool.removeForBlock(block, ++blocknum);
129128

130129
for (int i = 1; i < 10;i++) {
131130
BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() < origFeeEst[i-1] + deltaFee);
@@ -146,7 +145,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
146145
txHashes[j].push_back(hash);
147146
}
148147
}
149-
mpool.removeForBlock(block, ++blocknum, dummyConflicted);
148+
mpool.removeForBlock(block, ++blocknum);
150149
}
151150

152151
int answerFound;
@@ -167,7 +166,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
167166
txHashes[j].pop_back();
168167
}
169168
}
170-
mpool.removeForBlock(block, 265, dummyConflicted);
169+
mpool.removeForBlock(block, 265);
171170
block.clear();
172171
for (int i = 1; i < 10;i++) {
173172
BOOST_CHECK(mpool.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee);
@@ -187,7 +186,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
187186
block.push_back(*ptx);
188187
}
189188
}
190-
mpool.removeForBlock(block, ++blocknum, dummyConflicted);
189+
mpool.removeForBlock(block, ++blocknum);
191190
block.clear();
192191
}
193192
for (int i = 1; i < 10; i++) {

‎src/txmempool.cpp

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants
503503
}
504504
}
505505

506-
void CTxMemPool::removeRecursive(const CTransaction &origTx, std::list<CTransaction>& removed)
506+
void CTxMemPool::removeRecursive(const CTransaction &origTx, std::vector<std::shared_ptr<const CTransaction>>* removed)
507507
{
508508
// Remove transaction from memory pool
509509
{
@@ -530,8 +530,10 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, std::list<CTransact
530530
BOOST_FOREACH(txiter it, txToRemove) {
531531
CalculateDescendants(it, setAllRemoves);
532532
}
533-
BOOST_FOREACH(txiter it, setAllRemoves) {
534-
removed.push_back(it->GetTx());
533+
if (removed) {
534+
BOOST_FOREACH(txiter it, setAllRemoves) {
535+
removed->emplace_back(it->GetSharedTx());
536+
}
535537
}
536538
RemoveStaged(setAllRemoves, false);
537539
}
@@ -541,24 +543,24 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
541543
{
542544
// Remove transactions spending a coinbase which are now immature and no-longer-final transactions
543545
LOCK(cs);
544-
list<CTransaction> transactionsToRemove;
546+
setEntries txToRemove;
545547
for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) {
546548
const CTransaction& tx = it->GetTx();
547549
LockPoints lp = it->GetLockPoints();
548550
bool validLP = TestLockPointValidity(&lp);
549551
if (!CheckFinalTx(tx, flags) || !CheckSequenceLocks(tx, flags, &lp, validLP)) {
550552
// Note if CheckSequenceLocks fails the LockPoints may still be invalid
551553
// So it's critical that we remove the tx and not depend on the LockPoints.
552-
transactionsToRemove.push_back(tx);
554+
txToRemove.insert(it);
553555
} else if (it->GetSpendsCoinbase()) {
554556
BOOST_FOREACH(const CTxIn& txin, tx.vin) {
555557
indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash);
556558
if (it2 != mapTx.end())
557559
continue;
558560
const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash);
559-
if (nCheckFrequency != 0) assert(coins);
561+
if (nCheckFrequency != 0) assert(coins);
560562
if (!coins || (coins->IsCoinBase() && ((signed long)nMemPoolHeight) - coins->nHeight < COINBASE_MATURITY)) {
561-
transactionsToRemove.push_back(tx);
563+
txToRemove.insert(it);
562564
break;
563565
}
564566
}
@@ -567,13 +569,14 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
567569
mapTx.modify(it, update_lock_points(lp));
568570
}
569571
}
570-
BOOST_FOREACH(const CTransaction& tx, transactionsToRemove) {
571-
list<CTransaction> removed;
572-
removeRecursive(tx, removed);
572+
setEntries setAllRemoves;
573+
for (txiter it : txToRemove) {
574+
CalculateDescendants(it, setAllRemoves);
573575
}
576+
RemoveStaged(setAllRemoves, false);
574577
}
575578

576-
void CTxMemPool::removeConflicts(const CTransaction &tx, std::list<CTransaction>& removed)
579+
void CTxMemPool::removeConflicts(const CTransaction &tx, std::vector<std::shared_ptr<const CTransaction>>* removed)
577580
{
578581
// Remove transactions which depend on inputs of tx, recursively
579582
LOCK(cs);
@@ -594,7 +597,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx, std::list<CTransaction>
594597
* Called when a block is connected. Removes from mempool and updates the miner fee estimator.
595598
*/
596599
void CTxMemPool::removeForBlock(const std::vector<CTransaction>& vtx, unsigned int nBlockHeight,
597-
std::list<CTransaction>& conflicts, bool fCurrentEstimate)
600+
std::vector<std::shared_ptr<const CTransaction>>* conflicts, bool fCurrentEstimate)
598601
{
599602
LOCK(cs);
600603
std::vector<CTxMemPoolEntry> entries;

‎src/txmempool.h

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@
66
#ifndef BITCOIN_TXMEMPOOL_H
77
#define BITCOIN_TXMEMPOOL_H
88

9-
#include <list>
109
#include <memory>
1110
#include <set>
11+
#include <map>
12+
#include <vector>
13+
#include <utility>
14+
#include <string>
1215

1316
#include "amount.h"
1417
#include "coins.h"
@@ -521,11 +524,11 @@ class CTxMemPool
521524
bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, bool fCurrentEstimate = true);
522525
bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true);
523526

524-
void removeRecursive(const CTransaction &tx, std::list<CTransaction>& removed);
527+
void removeRecursive(const CTransaction &tx, std::vector<std::shared_ptr<const CTransaction>>* removed = NULL);
525528
void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags);
526-
void removeConflicts(const CTransaction &tx, std::list<CTransaction>& removed);
529+
void removeConflicts(const CTransaction &tx, std::vector<std::shared_ptr<const CTransaction>>* removed = NULL);
527530
void removeForBlock(const std::vector<CTransaction>& vtx, unsigned int nBlockHeight,
528-
std::list<CTransaction>& conflicts, bool fCurrentEstimate = true);
531+
std::vector<std::shared_ptr<const CTransaction>>* conflicts = NULL, bool fCurrentEstimate = true);
529532
void clear();
530533
void _clear(); //lock free
531534
bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb);

0 commit comments

Comments
 (0)