Refactor CBlockPolicyEstimator #9942

morcos · 2017-03-07T19:37:11Z

This moves towards disentangling CBlockPolicyEstimator from CTxMemPool by making it its own global object. At the end of this PR, the CTxMemPool still has to know about CBlockPolicyEstimator, but this lays the ground work so that in the future CBlockPolicyEstimator can just subscribe to transaction and block events and the mempool will know nothing about it.

The last two commits are preparation for future work where there will be multiple TxConfirmStats in the CBlockPolicyEstimator which all refer to the same set of buckets.

I've grouped the commits into this PR, because this is the of the commits which introduces no change into the fee estimates, so is possible to merge cleanly by itself. There are a set of major changes to fee estimation built on top of this that isn't finished yet.

TheBlueMatt

utACK 35e9c1a

TheBlueMatt · 2017-03-07T21:44:21Z

src/policy/fees.cpp

+        LOCK(cs_feeEstimator);
+        int nVersionRequired, nVersionThatWrote, nFileBestSeenHeight;
+        filein >> nVersionRequired >> nVersionThatWrote;
+        filein >> nFileBestSeenHeight;


Probably best to still check the nVersion before reading any further, no?

oops. thats a pretty big oversight. will fix.

laanwj · 2017-03-09T14:30:40Z

utACK 40d31d0

laanwj · 2017-03-24T12:30:30Z

Should the fixup be squashed first before merging this?

morcos · 2017-03-27T15:21:27Z

Squashed 40d31d0 (moveTxConfirmStats.tag1) -> 645946d

JeremyRubin · 2017-04-04T19:29:08Z

utAck 645946d.

Later work needs to clean up the TxConfirmedStats pointer usage, but that is a complex refactor and outside the scope of this PR.

and change to storing as a pointer.

morcos · 2017-04-10T19:07:19Z

Rebased. No changes from 645946d

ryanofsky · 2017-04-11T13:25:15Z

utACK 68af651. Nice cleanups, seems good to move this stuff out of the mempool.

sdaftuar · 2017-04-13T15:06:02Z

utACK 68af651

jtimon · 2017-04-18T17:54:04Z

For previous related work see #7145 and #7115

utACK individual commits: ae7327b f6187d6 dbb9e36 2332f19
fast review ACK individual commits: 14e10aa 5ba81e5 68af651

theuni

utACK 68af651. Nit is not critical.

I'm not familiar with this code, but the changes are well-defined and straightforward enough.

theuni · 2017-04-19T16:25:24Z

src/policy/fees.cpp

@@ -312,7 +312,12 @@ CBlockPolicyEstimator::CBlockPolicyEstimator()
        vfeelist.push_back(bucketBoundary);
    }
    vfeelist.push_back(INF_FEERATE);
-    feeStats.Initialize(vfeelist, MAX_BLOCK_CONFIRMS, DEFAULT_DECAY);
+    feeStats = new TxConfirmStats(vfeelist, MAX_BLOCK_CONFIRMS, DEFAULT_DECAY);


unique_ptr please?

heh... i agree, but i wanted to move code to .cpp file and then i was screwed by lack of a destructor. I'm sure there is a better way, but I went around in circles chasing my tail for a while.. Maybe we could save that for an actual programmer to clean up later.

68af651 MOVEONLY: move TxConfirmStats to cpp (Alex Morcos) 2332f19 Initialize TxConfirmStats in constructor (Alex Morcos) 5ba81e5 Read and Write fee estimate file directly from CBlockPolicyEstimator (Alex Morcos) 14e10aa Call estimate(Smart)Fee directly from CBlockPolicyEstimator (Alex Morcos) dbb9e36 Give CBlockPolicyEstimator it's own lock (Alex Morcos) f6187d6 Make processBlockTx private. (Alex Morcos) ae7327b Make feeEstimator its own global instance of CBlockPolicyEstimator (Alex Morcos) Tree-SHA512: dbf3bd2b30822e609a35f3da519b62d23f8a50e564750695ddebd08553b4c01874ae3e07d792c6cc78cc377d2db33b951ffedc46ac7edaf5793f9ebb931713af

Signed-off-by: Pasta <Pasta@dash.org>

68af651 MOVEONLY: move TxConfirmStats to cpp (Alex Morcos) 2332f19 Initialize TxConfirmStats in constructor (Alex Morcos) 5ba81e5 Read and Write fee estimate file directly from CBlockPolicyEstimator (Alex Morcos) 14e10aa Call estimate(Smart)Fee directly from CBlockPolicyEstimator (Alex Morcos) dbb9e36 Give CBlockPolicyEstimator it's own lock (Alex Morcos) f6187d6 Make processBlockTx private. (Alex Morcos) ae7327b Make feeEstimator its own global instance of CBlockPolicyEstimator (Alex Morcos) Tree-SHA512: dbf3bd2b30822e609a35f3da519b62d23f8a50e564750695ddebd08553b4c01874ae3e07d792c6cc78cc377d2db33b951ffedc46ac7edaf5793f9ebb931713af finish bitcoin#9942 by removing removed functions Signed-off-by: Pasta <Pasta@dash.org>

morcos force-pushed the moveTxConfirmStats branch from c3b3de4 to 35e9c1a Compare March 7, 2017 20:44

TheBlueMatt reviewed Mar 7, 2017

View reviewed changes

fanquake added the Refactoring label Mar 7, 2017

morcos force-pushed the moveTxConfirmStats branch from 40d31d0 to 645946d Compare March 27, 2017 15:15

morcos added 7 commits April 10, 2017 13:51

Make feeEstimator its own global instance of CBlockPolicyEstimator

ae7327b

Make processBlockTx private.

f6187d6

Give CBlockPolicyEstimator it's own lock

dbb9e36

Call estimate(Smart)Fee directly from CBlockPolicyEstimator

14e10aa

Read and Write fee estimate file directly from CBlockPolicyEstimator

5ba81e5

Initialize TxConfirmStats in constructor

2332f19

and change to storing as a pointer.

MOVEONLY: move TxConfirmStats to cpp

68af651

morcos force-pushed the moveTxConfirmStats branch from 645946d to 68af651 Compare April 10, 2017 19:06

morcos mentioned this pull request Apr 12, 2017

Better fee estimates #10199

Merged

theuni approved these changes Apr 19, 2017

View reviewed changes

laanwj merged commit 68af651 into bitcoin:master Apr 20, 2017

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request May 27, 2019

finish bitcoin#9942 by removing removed functions

6545d37

Signed-off-by: Pasta <Pasta@dash.org>

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!

Refactor CBlockPolicyEstimator #9942

Refactor CBlockPolicyEstimator #9942

Uh oh!

morcos commented Mar 7, 2017

Uh oh!

TheBlueMatt left a comment

Uh oh!

TheBlueMatt Mar 7, 2017

Uh oh!

morcos Mar 8, 2017

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

laanwj commented Mar 24, 2017

Uh oh!

morcos commented Mar 27, 2017 •

edited

Loading

Uh oh!

JeremyRubin commented Apr 4, 2017

Uh oh!

morcos commented Apr 10, 2017 •

edited

Loading

Uh oh!

ryanofsky commented Apr 11, 2017

Uh oh!

sdaftuar commented Apr 13, 2017

Uh oh!

jtimon commented Apr 18, 2017 •

edited

Loading

Uh oh!

theuni left a comment

Uh oh!

theuni Apr 19, 2017

Uh oh!

morcos Apr 19, 2017

Uh oh!

Uh oh!

Refactor CBlockPolicyEstimator #9942

Refactor CBlockPolicyEstimator #9942

Uh oh!

Conversation

morcos commented Mar 7, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

laanwj commented Mar 24, 2017

Uh oh!

morcos commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Apr 4, 2017

Uh oh!

morcos commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 11, 2017

Uh oh!

sdaftuar commented Apr 13, 2017

Uh oh!

jtimon commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

theuni Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morcos commented Mar 27, 2017 •

edited

Loading

morcos commented Apr 10, 2017 •

edited

Loading

jtimon commented Apr 18, 2017 •

edited

Loading