Mempool cleanups #12225

sdaftuar · 2018-01-19T16:13:33Z

Following up on #12127 and #12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of queryHashes() and a small information leak when prioritizing transactions.

Left undone is nuking queryHashes altogether; that would require changing the behavior of the getrawmempool rpc call, which I think I might be in favor of doing, but wanted to save for its own PR.

meshcollider · 2018-01-20T04:23:39Z

utACK 16a8389

promag

What is usually bigger?

CTxMemPool::mapTx
CBlockPolicyEstimator::mapMemPoolTxs

My suggestion is to loop the shorter map.

promag · 2018-01-20T15:58:26Z

src/policy/fees.cpp

@@ -984,10 +984,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
 void CBlockPolicyEstimator::FlushUnconfirmed(CTxMemPool& pool) {
    int64_t startclear = GetTimeMicros();
    std::vector<uint256> txids;


promag · 2018-01-20T16:00:08Z

src/policy/fees.cpp

-    for (auto& txid : txids) {
-        removeTx(txid, false);
+    LOCK2(pool.cs, cs_feeEstimator);
+    for (auto mi=pool.mapTx.begin(); mi != pool.mapTx.end(); ++mi) {


Nit, can early exit loop if mapMemPoolTxs is empty.

promag · 2018-01-20T16:03:27Z

src/policy/fees.cpp

-        removeTx(txid, false);
+    LOCK2(pool.cs, cs_feeEstimator);
+    for (auto mi=pool.mapTx.begin(); mi != pool.mapTx.end(); ++mi) {
+        removeTx(mi->GetTx().GetHash(), false);
    }
    int64_t endclear = GetTimeMicros();
    LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n",txids.size(), (endclear - startclear)*0.000001);


txids is empty, update logging.

sdaftuar · 2018-01-22T17:03:00Z

@promag After further investigation, it looks like FlushUnconfirmed() doesn't need the mempool at all, so I re-wrote it a bit.

promag · 2018-01-22T20:56:56Z

src/policy/fees.cpp

+    // Remove every entry in mapMemPoolTxs
+    while (!mapMemPoolTxs.empty()) {
+        auto mi = mapMemPoolTxs.begin();
+        removeTx(mi->first, false); // this calls erase() on mapMemPoolTxs


IMO decreasing one by one is avoidable when the end result is to be empty. Prefer something like?

LOCK(cs_feeEstimator); size_t num_entries = mapMemPoolTxs.size(); for (const auto& it = mapMemPoolTxs.begin(); it != mapMemPoolTxs.end(); ++it) { const auto& stat = it->second; feeStats->removeTx(stat.blockHeight, nBestSeenHeight, stat.bucketIndex, false); shortStats->removeTx(stat.blockHeight, nBestSeenHeight, stat.bucketIndex, false); longStats->removeTx(stat.blockHeight, nBestSeenHeight, stat.bucketIndex, false); } mapMemPoolTxs.clear();

I didn't want to duplicate the code in removeTx() or figure out how to refactor it to be more efficient for this purpose -- since FlushUnconfirmed() is only called in shutdown, I don't think the performance savings here from more code changes is worth the review/maintenance burden.

If you'd rather see a bigger refactor here to clean things up, I think I'd prefer to just remove this commit and someone can take it on in a new PR...

I think it's fine as it is. I'll take note to measure an alternative implementation.

promag · 2018-01-23T23:56:10Z

src/txmempool.h

@@ -242,14 +242,17 @@ class CompareTxMemPoolEntryByDescendantScore
 /** \class CompareTxMemPoolEntryByScore
 *
 *  Sort by score of entry ((fee+delta)/size) in descending order


Remove +delta?

sdaftuar · 2018-01-25T23:24:38Z

Squashed 12225.1 -> 669c943

promag · 2018-02-03T14:48:05Z

utACK 669c943.

laanwj · 2018-02-08T21:10:26Z

utACK 669c943

@TheBlueMatt

669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar) e868b22 fee estimator: avoid sorting mempool on shutdown (Suhas Daftuar) 0975406 Correct mempool mapTx comment (Suhas Daftuar) Pull request description: Following up on #12127 and #12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of `queryHashes()` and a small information leak when prioritizing transactions. Left undone is nuking queryHashes altogether; that would require changing the behavior of the `getrawmempool` rpc call, which I think I might be in favor of doing, but wanted to save for its own PR. Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162

@TheBlueMatt

Summary: 669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar) 0975406 Correct mempool mapTx comment (Suhas Daftuar) Pull request description: Following up on #12127 and #12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of `queryHashes()` and a small information leak when prioritizing transactions. Left undone is nuking queryHashes altogether; that would require changing the behavior of the `getrawmempool` rpc call, which I think I might be in favor of doing, but wanted to save for its own PR. Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162 Backport of Core PR12225 bitcoin/bitcoin#12225 Depends on D3711 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3789

@TheBlueMatt

669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar) e868b22 fee estimator: avoid sorting mempool on shutdown (Suhas Daftuar) 0975406 Correct mempool mapTx comment (Suhas Daftuar) Pull request description: Following up on bitcoin#12127 and bitcoin#12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of `queryHashes()` and a small information leak when prioritizing transactions. Left undone is nuking queryHashes altogether; that would require changing the behavior of the `getrawmempool` rpc call, which I think I might be in favor of doing, but wanted to save for its own PR. Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162

@TheBlueMatt

669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar) e868b22 fee estimator: avoid sorting mempool on shutdown (Suhas Daftuar) 0975406 Correct mempool mapTx comment (Suhas Daftuar) Pull request description: Following up on bitcoin#12127 and bitcoin#12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of `queryHashes()` and a small information leak when prioritizing transactions. Left undone is nuking queryHashes altogether; that would require changing the behavior of the `getrawmempool` rpc call, which I think I might be in favor of doing, but wanted to save for its own PR. Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162

@TheBlueMatt

669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar) e868b22 fee estimator: avoid sorting mempool on shutdown (Suhas Daftuar) 0975406 Correct mempool mapTx comment (Suhas Daftuar) Pull request description: Following up on bitcoin#12127 and bitcoin#12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of `queryHashes()` and a small information leak when prioritizing transactions. Left undone is nuking queryHashes altogether; that would require changing the behavior of the `getrawmempool` rpc call, which I think I might be in favor of doing, but wanted to save for its own PR. Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162

Correct mempool mapTx comment

0975406

fanquake added the Mempool label Jan 19, 2018

promag reviewed Jan 20, 2018

View reviewed changes

promag reviewed Jan 22, 2018

View reviewed changes

Speca92 approved these changes Jan 23, 2018

View reviewed changes

promag reviewed Jan 23, 2018

View reviewed changes

sdaftuar added 2 commits January 25, 2018 18:02

fee estimator: avoid sorting mempool on shutdown

e868b22

Avoid leaking prioritization information when relaying transactions

669c943

sdaftuar force-pushed the 2018-01-mempool-cleanups branch from 466267d to 669c943 Compare January 25, 2018 23:23

laanwj merged commit 669c943 into bitcoin:master Feb 8, 2018

ryanofsky mentioned this pull request Feb 14, 2018

External wallet files #11687

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Mempool cleanups #12225

Mempool cleanups #12225

Uh oh!

sdaftuar commented Jan 19, 2018

Uh oh!

meshcollider commented Jan 20, 2018

Uh oh!

promag left a comment •

edited

Loading

Uh oh!

promag Jan 20, 2018

Uh oh!

promag Jan 20, 2018

Uh oh!

promag Jan 20, 2018

Uh oh!

sdaftuar commented Jan 22, 2018

Uh oh!

promag Jan 22, 2018

Uh oh!

sdaftuar Jan 23, 2018

Uh oh!

promag Jan 23, 2018

Uh oh!

promag Jan 23, 2018

Uh oh!

sdaftuar Jan 25, 2018

Uh oh!

sdaftuar commented Jan 25, 2018

Uh oh!

promag commented Feb 3, 2018

Uh oh!

laanwj commented Feb 8, 2018

Uh oh!

Uh oh!

Mempool cleanups #12225

Mempool cleanups #12225

Uh oh!

Conversation

sdaftuar commented Jan 19, 2018

Uh oh!

meshcollider commented Jan 20, 2018

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Jan 25, 2018

Uh oh!

promag commented Feb 3, 2018

Uh oh!

laanwj commented Feb 8, 2018

Uh oh!

Uh oh!

promag left a comment •

edited

Loading