Sort mempool by min(feerate, ancestor_feerate) #12118

sdaftuar · 2018-01-08T18:58:51Z

This more closely approximates the desirability of a given transaction for
mining, and should result in less re-sorting when transactions get removed from
the mempool after being mined.

I measured this as approximately a 5% speedup in removeForBlock.

promag · 2018-01-08T23:51:55Z

src/txmempool.h

+    // Picks min(feerate, feerate_with_ancestors)
+    bool UseAncestorScore(const CTxMemPoolEntry &a) const
+    {
+        double f1 = (double)a.GetModifiedFee() * a.GetSizeWithAncestors();


Remove double cast?

promag · 2018-01-08T23:54:29Z

src/txmempool.h

+        bool fUseAAncestors = UseAncestorScore(a);
+        bool fUseBAncestors = UseAncestorScore(b);
+
+        double aModFee = fUseAAncestors ? a.GetModFeesWithAncestors() : a.GetModifiedFee();


Why floating point? Aren't these all 64 integers?

promag · 2018-01-09T00:01:41Z

src/txmempool.h


-        double bFees = b.GetModFeesWithAncestors();
-        double bSize = b.GetSizeWithAncestors();
+        double bModFee = fUseBAncestors ? b.GetModFeesWithAncestors() : b.GetModifiedFee();


Instead of factoring out UseAncestorScore, do (missing comments):

void GetModFeeAndSize(const CTxMemPoolEntry& a, int64_t& mod_fee, int64_t& size) { int64_t f1 = a.GetModifiedFee() * a.GetSizeWithAncestors(); int64_t f2 = a.GetModFeesWithAncestors() * a.GetTxSize(); if (f1 > f2) { mod_fee = a.GetModFeesWithAncestors(); size = a.GetSizeWithAncestors(); } else { mod_fee = a.GetModifiedFee(); size = a.GetTxSize(); } }

promag · 2018-01-09T00:01:59Z

src/txmempool.h


        if (f1 == f2) {
            return a.GetTx().GetHash() < b.GetTx().GetHash();
        }

        return f1 > f2;
    }
+
+    // Calculate which score to use for an entry (avoiding division).
+    // Picks min(feerate, feerate_with_ancestors)


Nit, missing ..

sdaftuar · 2018-01-09T16:02:14Z

@promag Thanks for taking a look. I mimicked CompareTxMempoolEntryByDescendantScore in this PR to make review easier, since the two concepts have a symmetry.

The use of double for the intermediate value is to prevent overflow of an int64_t, which is theoretically possible. That's also why there's an explicit double cast before multiplication.

I'll rewrite the helper function along the lines of your suggestion.

This more closely approximates the desirability of a given transaction for mining.

promag · 2018-01-09T23:57:57Z

The use of double for the intermediate value is to prevent overflow of an int64_t, which is theoretically possible.

How about http://releases.llvm.org/4.0.0/tools/clang/docs/LanguageExtensions.html#checked-arithmetic-builtins?

sdaftuar · 2018-01-10T15:19:01Z

@promag The style of where to put the & doesn't seem to be in our style guide and is inconsistent already in this file, so I'm going to leave it the way I have it (which happens to be my preference for where to put the & :) ).

I don't think it's a problem for code-correctness if the arithmetic overflows, so I'd rather just handle it as we currently do than introduce complication from calculating things two ways.

promag · 2018-01-10T15:45:00Z

utACK 9a51319.

I'm going to leave it the way I have it

Fine.

so I'd rather just handle it as we currently do

It was just curiosity. Maybe GetModFeeAndSize values could be cached in CTxMemPoolEntry to improve even further? (adding 128 bytes per entry)

ryanofsky

utACK 9a51319

I measured this as approximately a 5% speedup in removeForBlock.

I'm always interested to know how you benchmark these things, in case you want to post details.

The style of where to put the & doesn't seem to be in our style guide

Not important, but we do currently use the opposite setting for the code formatter:

bitcoin/src/.clang-format

Line 40 in 45173fa

PointerAlignment: Left

sdaftuar · 2018-01-11T02:21:46Z

I measured this as approximately a 5% speedup in removeForBlock.

I'm always interested to know how you benchmark these things, in case you want to post details.

@ryanofsky I ran through nearly 3 months of historical block & transaction data (roughly March-May 2017), which I record and can play back to a patched bitcoind. I ran two historical studies, one with this patch and one without, each with added benchmarks to specifically measure the time spent in removeForBlock.

TheBlueMatt · 2018-01-11T17:59:44Z

utACK 9a51319

gmaxwell · 2018-01-11T20:34:03Z

ACK

jimpo · 2018-01-12T08:32:59Z

Maybe augment the MempoolAncestorIndexingTest with a case of a higher fee parent with a lower fee child (for which the sort order has changed).

sdaftuar · 2018-01-12T17:43:58Z

@jimpo Sounds good; I added a test.

Transaction selection for mining tracks ancestor feerates that are modified based on transactions that have already been selected. This commit de-duplicates the code so that the ancestor feerate sorting used by the mempool can also be directly applied to the miner.

sdaftuar · 2018-01-13T21:07:51Z

I just remembered that the mining code had this ancestor feerate sorting logic duplicated; I just added a commit that attempts to re-use the mempool code so that the new behavior will apply there as well.

TheBlueMatt

uACK 0a22a52

TheBlueMatt · 2018-01-14T17:30:24Z

src/miner.cpp

@@ -352,7 +352,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
            // Try to compare the mapTx entry to the mapModifiedTx entry
            iter = mempool.mapTx.project<0>(mi);
            if (modit != mapModifiedTx.get<ancestor_score>().end() &&
-                    CompareModifiedEntry()(*modit, CTxMemPoolModifiedEntry(iter))) {
+                    CompareTxMemPoolEntryByAncestorFee()(*modit, CTxMemPoolModifiedEntry(iter))) {


Just to check my understanding, this should have zero effect, correct? The highest element in mapTx and mapModifiedTx should always be the element we want, if the min() is hit, it will never have sorted as the top element in either mapModifedTx or mapTx, as all modifiedTx entries also have their parents in modifiedTx?

Yes -- this change was only because I wanted to delete the code for CompareModifiedEntry, which is otherwise no longer needed.

all modifiedTx entries also have their parents in modifiedTx

Not quite -- the parents of entries in mapModifiedTx are not in mapModifiedTx if they have been included in the candidate block. Also a parent could be missing from mapModifiedTx if we already considered it for inclusion and it failed for some reason (eg the package it's part of would be too big).

laanwj · 2018-01-15T14:27:32Z

utACK 0a22a52

0a22a52 Use mempool's ancestor sort in transaction selection (Suhas Daftuar) 7abfa53 Add test for new ancestor feerate sort behavior (Suhas Daftuar) 9a51319 Sort mempool by min(feerate, ancestor_feerate) (Suhas Daftuar) 6773f92 Refactor CompareTxMemPoolEntryByDescendantScore (Suhas Daftuar) Pull request description: This more closely approximates the desirability of a given transaction for mining, and should result in less re-sorting when transactions get removed from the mempool after being mined. I measured this as approximately a 5% speedup in removeForBlock. Tree-SHA512: ffa36b567c5dfe3e8908c545a459b6a5ec0de26e7dc81b1050dd235cac9046564b4409a3f8c5ba97bd8b30526e8fec8f78480a912e317979467f32305c3dd37b

promag · 2018-01-15T14:43:22Z

re-utACK 0a22a52. Added test looks good.

@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

Summary: 0a22a52 Use mempool's ancestor sort in transaction selection (Suhas Daftuar) 7abfa53 Add test for new ancestor feerate sort behavior (Suhas Daftuar) 9a51319 Sort mempool by min(feerate, ancestor_feerate) (Suhas Daftuar) 6773f92 Refactor CompareTxMemPoolEntryByDescendantScore (Suhas Daftuar) Pull request description: This more closely approximates the desirability of a given transaction for mining, and should result in less re-sorting when transactions get removed from the mempool after being mined. I measured this as approximately a 5% speedup in removeForBlock. Tree-SHA512: ffa36b567c5dfe3e8908c545a459b6a5ec0de26e7dc81b1050dd235cac9046564b4409a3f8c5ba97bd8b30526e8fec8f78480a912e317979467f32305c3dd37b Backport of Core PR12118 bitcoin/bitcoin#12118 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3711

Summary: 0a22a52 Use mempool's ancestor sort in transaction selection (Suhas Daftuar) 7abfa53 Add test for new ancestor feerate sort behavior (Suhas Daftuar) 9a51319 Sort mempool by min(feerate, ancestor_feerate) (Suhas Daftuar) 6773f92 Refactor CompareTxMemPoolEntryByDescendantScore (Suhas Daftuar) Pull request description: This more closely approximates the desirability of a given transaction for mining, and should result in less re-sorting when transactions get removed from the mempool after being mined. I measured this as approximately a 5% speedup in removeForBlock. Tree-SHA512: ffa36b567c5dfe3e8908c545a459b6a5ec0de26e7dc81b1050dd235cac9046564b4409a3f8c5ba97bd8b30526e8fec8f78480a912e317979467f32305c3dd37b Backport of Core PR12118 bitcoin/bitcoin#12118 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3711

0a22a52 Use mempool's ancestor sort in transaction selection (Suhas Daftuar) 7abfa53 Add test for new ancestor feerate sort behavior (Suhas Daftuar) 9a51319 Sort mempool by min(feerate, ancestor_feerate) (Suhas Daftuar) 6773f92 Refactor CompareTxMemPoolEntryByDescendantScore (Suhas Daftuar) Pull request description: This more closely approximates the desirability of a given transaction for mining, and should result in less re-sorting when transactions get removed from the mempool after being mined. I measured this as approximately a 5% speedup in removeForBlock. Tree-SHA512: ffa36b567c5dfe3e8908c545a459b6a5ec0de26e7dc81b1050dd235cac9046564b4409a3f8c5ba97bd8b30526e8fec8f78480a912e317979467f32305c3dd37b

@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

fanquake added the Mempool label Jan 8, 2018

promag reviewed Jan 9, 2018

View reviewed changes

sdaftuar added 2 commits January 9, 2018 11:53

Refactor CompareTxMemPoolEntryByDescendantScore

6773f92

Sort mempool by min(feerate, ancestor_feerate)

9a51319

This more closely approximates the desirability of a given transaction for mining.

sdaftuar force-pushed the 2018-01-fix-mempool-score branch from f0aef6c to 9a51319 Compare January 9, 2018 17:48

sdaftuar mentioned this pull request Jan 10, 2018

Remove unused mempool index #12127

Merged

ryanofsky reviewed Jan 10, 2018

View reviewed changes

laanwj added this to the 0.16.0 milestone Jan 11, 2018

Add test for new ancestor feerate sort behavior

7abfa53

TheBlueMatt reviewed Jan 14, 2018

View reviewed changes

laanwj merged commit 0a22a52 into bitcoin:master Jan 15, 2018

sdaftuar mentioned this pull request Jan 19, 2018

Mempool cleanups #12225

Merged

kiminuo mentioned this pull request Apr 12, 2021

Add feerate histogram to getmempoolinfo #21422

Closed

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

Sort mempool by min(feerate, ancestor_feerate) #12118

Sort mempool by min(feerate, ancestor_feerate) #12118

sdaftuar commented Jan 8, 2018

promag Jan 8, 2018

promag Jan 8, 2018

promag Jan 9, 2018

promag Jan 9, 2018

sdaftuar commented Jan 9, 2018

promag commented Jan 9, 2018

sdaftuar commented Jan 10, 2018

promag commented Jan 10, 2018

ryanofsky left a comment

sdaftuar commented Jan 11, 2018

TheBlueMatt commented Jan 11, 2018

gmaxwell commented Jan 11, 2018

jimpo commented Jan 12, 2018

sdaftuar commented Jan 12, 2018

sdaftuar commented Jan 13, 2018

TheBlueMatt left a comment

TheBlueMatt Jan 14, 2018

sdaftuar Jan 16, 2018 •

edited

laanwj commented Jan 15, 2018

promag commented Jan 15, 2018

Sort mempool by min(feerate, ancestor_feerate) #12118

Sort mempool by min(feerate, ancestor_feerate) #12118

Conversation

sdaftuar commented Jan 8, 2018

promag Jan 8, 2018

Choose a reason for hiding this comment

promag Jan 8, 2018

Choose a reason for hiding this comment

promag Jan 9, 2018

Choose a reason for hiding this comment

promag Jan 9, 2018

Choose a reason for hiding this comment

sdaftuar commented Jan 9, 2018

promag commented Jan 9, 2018

sdaftuar commented Jan 10, 2018

promag commented Jan 10, 2018

ryanofsky left a comment

Choose a reason for hiding this comment

sdaftuar commented Jan 11, 2018

TheBlueMatt commented Jan 11, 2018

gmaxwell commented Jan 11, 2018

jimpo commented Jan 12, 2018

sdaftuar commented Jan 12, 2018

sdaftuar commented Jan 13, 2018

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Jan 14, 2018

Choose a reason for hiding this comment

sdaftuar Jan 16, 2018 • edited

Choose a reason for hiding this comment

laanwj commented Jan 15, 2018

promag commented Jan 15, 2018

sdaftuar Jan 16, 2018 •

edited