Improve fee estimation #9138

morcos · 2016-11-11T21:29:48Z

This PR is partly a refactor, partly bug fix, and partly improved robustness in edge cases. Outside of the edge cases (when your node is behind) the results are unchanged.

Explanations of the changes in the individual commits.

rebroad · 2016-11-13T03:37:06Z

src/main.cpp

+        return false;
+    if (chainActive.Height() < pindexBestHeader->nHeight - 1)
+        return false;
+    return true;


nit - usually a blank line before this line (but probably not a standard)

rebroad · 2016-11-13T03:39:57Z

src/policy/fees.cpp

    }

    // How many blocks did it take for miners to include this transaction?
    // blocksToConfirm is 1-based, so a transaction included in the earliest
    // possible block has confirmation count of 1
-    int blocksToConfirm = nBlockHeight - entry.GetHeight();
+    int blocksToConfirm = nBlockHeight - entry->GetHeight();


what did this fix please?

Please see commit message "Pass pointers to existing CTxMemPoolEntries to fee estimation"
The idea is this would save unnecessary copying.

morcos · 2016-11-21T20:11:35Z

rebased

gmaxwell · 2016-11-23T22:40:03Z

Concept ACK.

jonasschnelli · 2016-11-25T07:55:53Z

Ran this code on top of master on one of my nodes.
Just compared it to the node running master (same network but independent mempool), see below.
The differences for a conf. target of 2 blocks are quite large.

Next step would be to run master and this PR with the same mempool and maybe dump 2,4,8,20 target every ~10mins.

masternode ~/node/bitcoin $ ./src/bitcoin-cli getmempoolinfo
{
  "size": 22022,
  "bytes": 133099706,
  "usage": 252978016,
  "maxmempool": 300000000,
  "mempoolminfee": 0.00001014
}
masternode ~/node/bitcoin $ ./src/bitcoin-cli estimatesmartfee 2
{
  "feerate": 0.00761447,
  "blocks": 2
}
masternode ~/node/bitcoin $ ./src/bitcoin-cli estimatesmartfee 3
{
  "feerate": 0.00069881,
  "blocks": 3
}
masternode ~/node/bitcoin $ ./src/bitcoin-cli estimatesmartfee 8
{
  "feerate": 0.00069881,
  "blocks": 8
}

thisprnode ~/bitcoin/bitcoin $ ./src/bitcoin-cli getmempoolinfo
{
  "size": 24593,
  "bytes": 140534928,
  "usage": 268029424,
  "maxmempool": 300000000,
  "mempoolminfee": 0.00001000
}
thisprnode ~/bitcoin/bitcoin $ ./src/bitcoin-cli estimatesmartfee 2
{
  "feerate": 0.00083575,
  "blocks": 2
}
thisprnode ~/bitcoin/bitcoin $ ./src/bitcoin-cli estimatesmartfee 3
{
  "feerate": 0.00077770,
  "blocks": 3
}
thisprnode ~/bitcoin/bitcoin $ ./src/bitcoin-cli estimatesmartfee 8
{
  "feerate": 0.00069872,
  "blocks": 8
}

morcos · 2016-11-25T22:08:08Z

@jonasschnelli I simulated this PR's code against master and the differences were very slight. If your testing setups were identical (started with the same fee estimates and started the nodes at the same time), then the way you might see substantial differences is if your testing nodes have to catch up a little bit when you start them, then this code will decay your old estimates and the old code wouldn't. So this code will more quickly reflect current conditions instead of still having a heavy weight for old fee estimates. Furthermore if you started this code after being down for more than a few weeks, you might even start seeing messages about fee estimates need to warm up as the historical data will have decayed so much.

sdaftuar · 2016-11-29T01:14:05Z

src/policy/fees.cpp

@@ -392,6 +382,9 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight,
    for (unsigned int i = 0; i < entries.size(); i++)
        processBlockTx(nBlockHeight, entries[i]);

+    // Update best seen height after removals so they find the right mempool txs


Can you clarify this comment? I have been staring at this for a while and can't figure out what you meant.

sdaftuar · 2016-11-29T01:41:00Z

src/main.h

@@ -128,6 +128,8 @@ static const int64_t BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 500000;
 static const unsigned int DEFAULT_LIMITFREERELAY = 0;
 static const bool DEFAULT_RELAYPRIORITY = true;
 static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60;
+/** Maximum age of our tip for us to be considered current for fee estimation */


nit: Can you update the comment to include the unit (ie seconds)?

sdaftuar · 2016-11-29T01:45:41Z

Code review ACK. Will test.

sdaftuar · 2016-11-29T13:51:15Z

src/policy/fees.cpp

-        // This transaction depended on other transactions in the mempool to
-        // be included in a block before it was able to be included, so
-        // we shouldn't include it in our calculations
+    if (!removeTx(entry.GetTx().GetHash())) {


Does calling this here result in one extra call to removeTx() for each in-mempool, in-block transaction? It looks like we also will call this in CTxMempool::removeUnchecked, which I guess is harmless, but worth commenting.

morcos · 2016-11-29T20:59:41Z

Addressed @sdaftuar's comment requests.
Fixed a pre-existing rare edge case which would print errors and make estimates slightly off.
Fixed a bug in this PR.

@gmaxwell once you see the BUGFIX commit, I'll squash just that one into the right place in the history.

sdaftuar · 2016-11-30T17:59:23Z

src/main.cpp

+{
+    if (IsInitialBlockDownload())
+        return false;
+    if (chainActive.Tip()->GetBlockTime() < (GetTime() - MAX_FEE_ESTIMATION_TIP_AGE))


nit: I think we should AssertLockHeld(cs_main) here.

sdaftuar · 2016-11-30T19:55:01Z

Tested this code in simulation on 10 days in July, comparing the transactions tracked and the fee estimates returned before and after this PR. ACK apart from the AssertLockHeld nit above.

morcos · 2016-12-03T14:03:12Z

dancing again on the grave of main.cpp

This was a clean rebase and I added @sdaftuar's AssertLockHeld.

In a couple days I'll squash the last two commits...

morcos · 2016-12-05T19:52:38Z

rebased (and squashed in the bugfix and assert)

@fanquake please milestone for 0.14.0

sdaftuar · 2016-12-09T18:18:55Z

re-ACK 98fffcb

morcos · 2016-12-10T15:22:47Z

Simple rebase due to expected conflict on function signature of CTxMemPool:removeForBlock

sdaftuar · 2016-12-10T19:11:22Z

re-ACK 761a9be

sipa

Code review ACK. I don't understand the fee estimation code enough to judge the changes to the semantics.

sipa · 2016-12-10T19:12:15Z

src/policy/fees.cpp

+        mapMemPoolTxs.erase(hash);
+        return true;
+    }
+    else {


Coding style nit: else on the same line.

sipa · 2016-12-10T19:35:26Z

src/txmempool.cpp

    for (const auto& tx : vtx)
    {
        uint256 hash = tx->GetHash();

        indexed_transaction_set::iterator i = mapTx.find(hash);
        if (i != mapTx.end())
-            entries.push_back(*i);
+            entries.push_back(&(*i));


Parentheses aren't needed here.

morcos · 2016-12-30T14:04:35Z

Rebased to fix a silent merge conflict (the change in wallet.cpp)
Also fixed @sipa's nits, otherwise unchanged from reviewed code.

gmaxwell · 2017-01-04T15:44:33Z

reviewed and tested this before it got conflicted ACK. needs rebase.

Once priority estimation was removed, not all transactions in the mempool are tracked in the fee estimation mempool tracking. So there is no error if a transaction is not found for removal.

This was an oversight, where blocks and mempool tracking were ignored during IBD, but transactions that arrived during IBD but were included in blocks after IBD were not ignored.

Fee estimation can just check its own mapMemPoolTxs to determine the same information. Note that now fee estimation for block processing must happen before those transactions are removed, but this shoudl be a speedup.

All decisions about whether the transactions are valid data points are made at the time the transaction arrives. Updating on blocks all the time will now cause stale fee estimates to decay quickly when we restart a node.

Make a more conservative notion of whether the node is caught up to the rest of the network and only count transactions as fee estimation data points if the node is caught up.

morcos · 2017-01-04T17:35:28Z

rebased

sdaftuar · 2017-01-05T22:04:10Z

re-ACK 44b64b9

sipa · 2017-01-05T22:14:08Z

utACK 44b64b9

44b64b9 Fix edge case with stale fee estimates (Alex Morcos) 78ae62d Add clarifying comments to fee estimation (Alex Morcos) 5fe0f47 Add extra logging to processBlock in fee estimation. (Alex Morcos) dc008c4 Add IsCurrentForFeeEstimatation (Alex Morcos) ebafdca Pass pointers to existing CTxMemPoolEntries to fee estimation (Alex Morcos) d825838 Always update fee estimates on new blocks. (Alex Morcos) 6f06b26 rename bool to validFeeEstimate (Alex Morcos) 84f7ab0 Remove member variable hadNoDependencies from CTxMemPoolEntry (Alex Morcos) 60ac00d Don't track transactions at all during IBD. (Alex Morcos) 4df4479 Remove extraneous LogPrint from fee estimation (Alex Morcos)

…priority removal f5b2e54 Fix comparisons for integer expressions of different signedness (random-zebra) 784d8b4 [Doc] Add Mempool priority changes to release notes (random-zebra) 6163d4b Remove -blockminsize option (Suhas Daftuar) b304fb9 Allow setting minrelaytxfee to 0 (Alex Morcos) 006136e Make boost::multi_index comparators const (Suhas Daftuar) 5858fb5 [Tests] Remove priority check for sapling txes (random-zebra) e267d02 [Cleanup] Remove coin age priority completely (random-zebra) 4298938 [RPC] Remove priorityDelta from prioritisetransaction (random-zebra) cd1535a [rpc] Remove priority information from mempool RPC calls (Alex Morcos) 3b4d207 [Refactor][RPC] entryToJSON/EntryDescriptionString out of mempoolToJSON (random-zebra) 142415e Fix edge case with stale fee estimates (Alex Morcos) 9f47b0f Add clarifying comments to fee estimation (Alex Morcos) 57b7922 Add extra logging to processBlock in fee estimation. (Alex Morcos) 59486ef Add IsCurrentForFeeEstimatation (Alex Morcos) d23ebfd Pass pointers to existing CTxMemPoolEntries to fee estimation (Alex Morcos) be5982e Always update fee estimates on new blocks. (Alex Morcos) aa97809 rename bool to validFeeEstimate (Alex Morcos) 82aba64 Remove member variable hadNoDependencies from CTxMemPoolEntry (random-zebra) c3758e1 Don't track transactions at all during IBD. (Alex Morcos) 9ca59ae [rpc] rawtx: Prepare fLimitFree to make it an option (MarcoFalke) f682e75 [wallet] Set fLimitFree = true (MarcoFalke) 0221d5a [QA] Fix random failure in wallet_abandonconflict.py (random-zebra) 33c9aa1 [Trivial] Pass hash by const ref to CBlockPolicyEstimator::removeTx (random-zebra) 7be33e0 Remove extraneous LogPrint from fee estimation (Alex Morcos) 7b6e8a3 [test] Remove priority from tests (Alex Morcos) c2ecf27 No longer allow "free" transactions (Alex Morcos) 5c577c5 [Cleanup] Remove feature_nulldummy.py unused functional test (random-zebra) 3097847 [cleanup] Remove estimatePriority and estimateSmartPriority (random-zebra) f283e8a [debug] Change -printpriority option (Alex Morcos) dcddcaf [Cleanup] Remove unused (always false) fAllowFree arg in GetMinRelayFee (random-zebra) 03d2ee9 [mining] Remove -blockprioritysize. (Alex Morcos) cfaeb97 [BUG] Don't add priority txes (random-zebra) 01882da Add unit tests for ancestor feerate mining (Suhas Daftuar) 49b1a9b [Policy] Limit total SHIELD size and check spork 20 in addPackageTxs (random-zebra) cc473ed Use ancestor-feerate based transaction selection for mining (random-zebra) dfaf4e7 [Wallet] Remove sendfree (random-zebra) Pull request description: This PR implements CPFP transaction selection mechanism, which is the simplest "fee bumping technique" (we might want to consider implementing RBF too later on). The algorithm in `CreateNewBlock` now selects transactions based on their feerate, inclusive of unconfirmed ancestor transactions. This means that a low-fee transaction can become more likely to be selected if a high-fee transaction that spends its outputs is relayed: the fee burned in the "child" transaction, actually pays for the parent tx too (hence the name). This PR also removes completely the (already deprecated) notion of coinage-based priority and "free transactions area". Changes backported / adapted from: - bitcoin#7600 [Suhas Daftuar] - bitcoin#8287 [MarcoFalke] - bitcoin#8295 [Suhas Daftuar] - bitcoin#9138 [Alex Morcos] - bitcoin#9602 [Alex Morcos] - bitcoin#11847 [Suhas Daftuar] ACKs for top commit: Fuzzbawls: utACK f5b2e54 furszy: re-ACK f5b2e54 after rebase and last commit. Tree-SHA512: aef64628008699c81735660fcbe51789b7dc9d2a670d0c695399a2821f01f5d72e46d72b5f57d7b28c0e0028b60b4d6294ee101b8038ea46d237c7b8729a79da

morcos mentioned this pull request Nov 11, 2016

Remove extraneous LogPrint from fee estimation #9123

Closed

fanquake added Mempool TX fees and policy labels Nov 12, 2016

rebroad reviewed Nov 13, 2016

View reviewed changes

morcos force-pushed the refactorFeeEstimation branch from 989a2c4 to 2ea74cf Compare November 14, 2016 15:27

morcos force-pushed the refactorFeeEstimation branch from 2ea74cf to 6b8681b Compare November 21, 2016 20:10

sdaftuar reviewed Nov 29, 2016

View reviewed changes

sdaftuar reviewed Nov 30, 2016

View reviewed changes

morcos force-pushed the refactorFeeEstimation branch 2 times, most recently from ed32b1a to afead86 Compare December 3, 2016 14:02

morcos force-pushed the refactorFeeEstimation branch from afead86 to 98fffcb Compare December 5, 2016 19:51

maflcko added this to the 0.14.0 milestone Dec 5, 2016

morcos force-pushed the refactorFeeEstimation branch from 98fffcb to 761a9be Compare December 10, 2016 15:21

sipa reviewed Dec 10, 2016

View reviewed changes

morcos force-pushed the refactorFeeEstimation branch from 761a9be to d6b2ff7 Compare December 30, 2016 14:03

morcos added 10 commits January 4, 2017 12:03

Remove extraneous LogPrint from fee estimation

4df4479

Once priority estimation was removed, not all transactions in the mempool are tracked in the fee estimation mempool tracking. So there is no error if a transaction is not found for removal.

Don't track transactions at all during IBD.

60ac00d

This was an oversight, where blocks and mempool tracking were ignored during IBD, but transactions that arrived during IBD but were included in blocks after IBD were not ignored.

Remove member variable hadNoDependencies from CTxMemPoolEntry

84f7ab0

Fee estimation can just check its own mapMemPoolTxs to determine the same information. Note that now fee estimation for block processing must happen before those transactions are removed, but this shoudl be a speedup.

rename bool to validFeeEstimate

6f06b26

Always update fee estimates on new blocks.

d825838

All decisions about whether the transactions are valid data points are made at the time the transaction arrives. Updating on blocks all the time will now cause stale fee estimates to decay quickly when we restart a node.

Pass pointers to existing CTxMemPoolEntries to fee estimation

ebafdca

Add IsCurrentForFeeEstimatation

dc008c4

Make a more conservative notion of whether the node is caught up to the rest of the network and only count transactions as fee estimation data points if the node is caught up.

Add extra logging to processBlock in fee estimation.

5fe0f47

Add clarifying comments to fee estimation

78ae62d

Fix edge case with stale fee estimates

44b64b9

morcos force-pushed the refactorFeeEstimation branch from d6b2ff7 to 44b64b9 Compare January 4, 2017 17:35

sipa merged commit 44b64b9 into bitcoin:master Jan 5, 2017

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019

Merge bitcoin#9138: Improve fee estimation

41fe5ce

random-zebra mentioned this pull request May 10, 2021

[Policy] Child-pays-for-parent Implementation + CoinAge priority removal PIVX-Project/PIVX#2378

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

Improve fee estimation #9138

Improve fee estimation #9138

morcos commented Nov 11, 2016

rebroad Nov 13, 2016

rebroad Nov 13, 2016

morcos Nov 13, 2016

morcos commented Nov 21, 2016

gmaxwell commented Nov 23, 2016

jonasschnelli commented Nov 25, 2016

morcos commented Nov 25, 2016

sdaftuar Nov 29, 2016

sdaftuar Nov 29, 2016

sdaftuar commented Nov 29, 2016

sdaftuar Nov 29, 2016

morcos commented Nov 29, 2016

sdaftuar Nov 30, 2016

sdaftuar commented Nov 30, 2016

morcos commented Dec 3, 2016

morcos commented Dec 5, 2016

sdaftuar commented Dec 9, 2016

morcos commented Dec 10, 2016 •

edited

sdaftuar commented Dec 10, 2016

sipa left a comment

sipa Dec 10, 2016

sipa Dec 10, 2016

morcos commented Dec 30, 2016

gmaxwell commented Jan 4, 2017

morcos commented Jan 4, 2017

sdaftuar commented Jan 5, 2017

sipa commented Jan 5, 2017

Improve fee estimation #9138

Improve fee estimation #9138

Conversation

morcos commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morcos commented Nov 21, 2016

gmaxwell commented Nov 23, 2016

jonasschnelli commented Nov 25, 2016

morcos commented Nov 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdaftuar commented Nov 29, 2016

Choose a reason for hiding this comment

morcos commented Nov 29, 2016

Choose a reason for hiding this comment

sdaftuar commented Nov 30, 2016

morcos commented Dec 3, 2016

morcos commented Dec 5, 2016

sdaftuar commented Dec 9, 2016

morcos commented Dec 10, 2016 • edited

sdaftuar commented Dec 10, 2016

sipa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morcos commented Dec 30, 2016

gmaxwell commented Jan 4, 2017

morcos commented Jan 4, 2017

sdaftuar commented Jan 5, 2017

sipa commented Jan 5, 2017

morcos commented Dec 10, 2016 •

edited