Refactor CBlockPolicyEstimator

morcos commented at 7:37 pm on March 7, 2017: member

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.

morcos force-pushed on Mar 7, 2017

in src/policy/fees.cpp: in bc761c622a outdated

494-    // if nVersionThatWrote < 139900 then another TxConfirmStats (for priority) follows but can be ignored.
495+    try {
496+        LOCK(cs_feeEstimator);
497+        int nVersionRequired, nVersionThatWrote, nFileBestSeenHeight;
498+        filein >> nVersionRequired >> nVersionThatWrote;
499+        filein >> nFileBestSeenHeight;

TheBlueMatt commented at 9:44 pm on March 7, 2017:

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

morcos commented at 2:16 pm on March 8, 2017:

oops. thats a pretty big oversight. will fix.

TheBlueMatt commented at 10:28 pm on March 7, 2017: member

utACK 35e9c1a18d9eb3f32690776d9eff88db1c673f3c

fanquake added the label Refactoring on Mar 7, 2017

laanwj commented at 2:30 pm on March 9, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/9942/commits/40d31d07c8cbf25aef346bb58d1360d96bbec653

laanwj commented at 12:30 pm on March 24, 2017: member

Should the fixup be squashed first before merging this?

morcos force-pushed on Mar 27, 2017

morcos commented at 3:21 pm on March 27, 2017: member

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

JeremyRubin commented at 7:29 pm on April 4, 2017: contributor

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.

Make feeEstimator its own global instance of CBlockPolicyEstimator ae7327b832

Make processBlockTx private. f6187d6e39

Give CBlockPolicyEstimator it's own lock dbb9e3699b

Call estimate(Smart)Fee directly from CBlockPolicyEstimator 14e10aa842

Read and Write fee estimate file directly from CBlockPolicyEstimator 5ba81e54e0

Initialize TxConfirmStats in constructor

and change to storing as a pointer.

2332f19bef

MOVEONLY: move TxConfirmStats to cpp 68af651498

morcos force-pushed on Apr 10, 2017

morcos commented at 7:07 pm on April 10, 2017: member

Rebased. No changes from 645946d

ryanofsky commented at 1:25 pm on April 11, 2017: member

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

sdaftuar commented at 3:06 pm on April 13, 2017: member

utACK 68af6514987d9d7bfcd67caa9394edda6ab5ef2c

laanwj added this to the "Blockers" column in a project

jtimon commented at 5:54 pm on April 18, 2017: contributor

For previous related work see #7145 and #7115

utACK individual commits: ae7327b8322d36d00047e92fe699371185de1c68 f6187d6e393b5ca587604b678f91496d50149a20 dbb9e3699b8e835fd72a5db2c22927d828484c32 2332f19bef025c22fab5a96a0cd2d52d22489aa2 fast review ACK individual commits: 14e10aa842b8583f9648accd5d151dbdf342b9dc 5ba81e54e0390ec0be7dbc8ebea0c35933442a8a 68af6514987d9d7bfcd67caa9394edda6ab5ef2c

in src/policy/fees.cpp:315 in 2332f19bef outdated

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

theuni commented at 4:25 pm on April 19, 2017:

unique_ptr please?

morcos commented at 4:45 pm on April 19, 2017:

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.

theuni approved

theuni commented at 4:28 pm on April 19, 2017: member

utACK 68af6514987d9d7bfcd67caa9394edda6ab5ef2c. Nit is not critical.

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

laanwj merged this on Apr 20, 2017

laanwj closed this on Apr 20, 2017

laanwj referenced this in commit 14c948987f on Apr 20, 2017

fanquake removed this from the "Blockers" column in a project

PastaPastaPasta referenced this in commit f4ae43a924 on May 21, 2019

PastaPastaPasta referenced this in commit 678844c7f4 on May 21, 2019

PastaPastaPasta referenced this in commit c4daa34aea on May 22, 2019

PastaPastaPasta referenced this in commit ff4755aad2 on May 22, 2019

PastaPastaPasta referenced this in commit 6927388801 on May 22, 2019

PastaPastaPasta referenced this in commit 57e953cedb on May 22, 2019

PastaPastaPasta referenced this in commit 56e6cb2d49 on May 23, 2019

PastaPastaPasta referenced this in commit 6545d37199 on May 27, 2019

PastaPastaPasta referenced this in commit f8c5a14f3d on May 27, 2019

UdjinM6 referenced this in commit ae6cda419a on May 28, 2019

barrystyle referenced this in commit ba9c7bc49c on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

Refactor CBlockPolicyEstimator #9942