Better fee estimates #10199
Better fee estimates #10199
Conversation
Great!
I'll have a look at this soon. |
ping @crwatkins (he presented some fee estimation overview at the wallet standards group meeting in Berlin this month). |
"\nWARNING: This interface is unstable and may disappear or change!\n" | ||
"\nWARNING: This is an advanced API call that is tightly coupled to the specific\n" | ||
" implementation of fee estimation. The parameters it can be called with\n" | ||
" and the results it returns will change if the internal implementaion changes.\n" |
jlopp
Apr 19, 2017
Contributor
Nit: implementation
Nit: implementation
I'm not sure if this is a problem specific to your code, though requiring a leading 0 seems like a bug:
|
@jlopp |
int nBlocks = request.params[0].get_int(); | ||
double threshold = 0.95; | ||
if (!request.params[1].isNull()) | ||
threshold = request.params[1].get_real(); |
jlopp
Apr 19, 2017
Contributor
Would prefer more explicit range bounding on the threshold with an "Invalid threshold" error response; it currently allows numbers < 0 and > 1 and just returns a -1 feerate.
Would prefer more explicit range bounding on the threshold with an "Invalid threshold" error response; it currently allows numbers < 0 and > 1 and just returns a -1 feerate.
morcos
Apr 20, 2017
Author
Member
The reason I didn't bother with that is I felt estimaterawfee should be a very low level feature that doesn't need all the user friendly bells and whistles.. But it could certainly be cleaned up to add that either in this PR or a follow on..
The reason I didn't bother with that is I felt estimaterawfee should be a very low level feature that doesn't need all the user friendly bells and whistles.. But it could certainly be cleaned up to add that either in this PR or a follow on..
Move buckets and bucketMap to be stored as part of overall serialization of estimator. Add some placeholder data so file format is only changed once. Maintain 3 different TxConfirmStats with potential for different decays and scales.
clean rebase after #9942 was merged |
Thanks @jonasschnelli. In Berlin, I noted that at least 50% of the wallets listed on bitcoin.org use Core as a fee estimation source directly or through other servers based on Core and any statistical or algorithmic biases might compound over time because of the large number of clients using the single source. I presented some anecdotal, non-scientific observations about fees and some reasons that they might be artificially high. Some of my concerns about the current esitmatefee were
I believe that the changes in this PR mitigate some of those concerns, and thus I am in favor of these changes. Nice work! I'm still a little concerned about so many wallets "competing" using the same identical algorithm. @morcos do you envision some devs (wallets) would use estimaterawfee to tweak the algorithm for their users? |
@crwatkins I can only speak for BitGo, but we run custom compiled versions of Core with our own half life and confidence values. We then use the output as a baseline for a more complex fee algorithm that we adjust upward and downward based upon other data sources. I'm already working on transitioning us to using the estimaterawfee functionality. |
@crwatkins Yes that was the basic idea of including estimaterawfee (that and it's useful for debugging). The exact strategy you use for determining what fee you want to put on a transaction depends on so many factors that its hard to have a one size fits all solution, but thats exactly what Bitcoin Core has to try to ship. So rather than attempt to defend why the algorithm I picked is the one algorithm to rule them all, I thought it would also make sense to expose a way to examine all the collected data and let people build their own algorithms if they'd like to. |
@morcos Exclellent. Thanks! |
Reviewing |
Looks good. With the exception of the fallback during upgrade and the one strange sharp edge LGTM. Needs testing because the code being correct isnt really enough, but no doubt better than today's estimates. |
|
||
// Start counting from highest(default) or lowest feerate transactions | ||
for (int bucket = startbucket; bucket >= 0 && bucket <= maxbucketindex; bucket += step) { | ||
if (newBucketRange) { |
TheBlueMatt
Apr 28, 2017
Contributor
Can you update the commit message from "Track start of new bucket range more carefully" to something more descriptive.
Can you update the commit message from "Track start of new bucket range more carefully" to something more descriptive.
" \"leftmempool\" : x.x, (numeric) number of txs over history horizon in the feerate range that left mempool unconfirmed after target\n" | ||
"}\n" | ||
"\n" | ||
"A negative value is returned if no answer can be given.\n" |
TheBlueMatt
Apr 28, 2017
Contributor
nit: "A negative feerate is returned" (other values are zero, so might as well just specify feerate here).
nit: "A negative feerate is returned" (other values are zero, so might as well just specify feerate here).
100 * nConf / (totalNum + extraNum), nConf, totalNum, extraNum); | ||
// If we were passing until we reached last few buckets with insufficient data, then report those as failed | ||
if (passing && !newBucketRange) { | ||
unsigned int failMinBucket = curNearBucket < curFarBucket ? curNearBucket : curFarBucket; |
TheBlueMatt
Apr 28, 2017
Contributor
nit: std::min/max here like you did above?
nit: std::min/max here like you did above?
std::vector<std::vector<int> > curBlockConf; // curBlockConf[Y][X] | ||
std::vector<std::vector<double>> confAvg; // confAvg[Y][X] | ||
|
||
std::vector<std::vector<double>> failAvg; // future use |
TheBlueMatt
Apr 28, 2017
Contributor
Should probably update this comment in the commit that puts failAvg to use.
Should probably update this comment in the commit that puts failAvg to use.
std::map<double, unsigned int> tempMap; | ||
|
||
std::unique_ptr<TxConfirmStats> tempFeeStats(new TxConfirmStats(tempBuckets, tempMap, MAX_BLOCK_CONFIRMS, tempDecay)); | ||
tempFeeStats->Read(filein, nVersionThatWrote, tempNum); |
TheBlueMatt
Apr 28, 2017
Contributor
As discussed it'd be nice to use this chunk and tempFeeStats to just generate one fee estimate, cache that, and then start fresh (as you do) but use the cached fee estimate while our new buckets fill.
As discussed it'd be nice to use this chunk and tempFeeStats to just generate one fee estimate, cache that, and then start fresh (as you do) but use the cached fee estimate while our new buckets fill.
morcos
May 9, 2017
Author
Member
leaving for a later improvement
leaving for a later improvement
@@ -465,6 +469,8 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo | |||
|
|||
mapMemPoolTxs[hash].blockHeight = txHeight; | |||
mapMemPoolTxs[hash].bucketIndex = feeStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); | |||
shortStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); |
TheBlueMatt
Apr 28, 2017
Contributor
nit: might be nice to assert that the bucketIndex is the same across all three calls here?
nit: might be nice to assert that the bucketIndex is the same across all three calls here?
|
||
/** Write estimation data to a file */ | ||
bool Write(CAutoFile& fileout) const; | ||
|
||
/** Read estimation data from a file */ | ||
bool Read(CAutoFile& filein); | ||
|
||
/** Empty mempool transactions on shutdown to record failure to confirm for txs still in mempool */ | ||
void ClearInMempool(CTxMemPool& pool); |
TheBlueMatt
Apr 28, 2017
Contributor
nit: maybe a better name for this would be FlushFailedPreShutdown()?
nit: maybe a better name for this would be FlushFailedPreShutdown()?
@@ -448,7 +448,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) | |||
mapLinks.erase(it); | |||
mapTx.erase(it); | |||
nTransactionsUpdated++; | |||
if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash);} | |||
if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash, false);} |
TheBlueMatt
Apr 28, 2017
Contributor
nit: it seems kinda strange that the "inBlock" flag is always set to false, even though it is sometimes called when the transaction was, indeed, in a block. Not a bug because it is a dummy call in fees.cpp, but maybe the API should stay the same and have the flag filled in inside of fees.cpp.
nit: it seems kinda strange that the "inBlock" flag is always set to false, even though it is sometimes called when the transaction was, indeed, in a block. Not a bug because it is a dummy call in fees.cpp, but maybe the API should stay the same and have the flag filled in inside of fees.cpp.
morcos
May 9, 2017
Author
Member
This will eventually be cleaned up when fee estimation listens to CValidationInterface
This will eventually be cleaned up when fee estimation listens to CValidationInterface
return CFeeRate(0); | ||
|
||
if ((unsigned int)confTarget > maxUsableEstimate) { | ||
confTarget = maxUsableEstimate; |
TheBlueMatt
Apr 28, 2017
Contributor
This seems strange to me. Most of our APIs will already gracefully fail if you pass in too high a conf target, I'd rather we do the same here (same in GUI).
This seems strange to me. Most of our APIs will already gracefully fail if you pass in too high a conf target, I'd rather we do the same here (same in GUI).
morcos
May 4, 2017
Author
Member
The design of estimateSmartFee is to be used by the wallet to intelligently as possible automatically put a fee on a transaction being sent. In the GUI, the slider clearly indicates what target the fee is being returned for. For the RPC call estimatesmartfee, this is also clearly returned. The difficulty comes with automatic estimation that happens from a sendtoaddress or sendmany RPC call. If in the future we add a way to select a transaction confirm target with the desired RPC call, perhaps it would make sense to fail, but for now it seems to me that its more important for the transaction to go out, even if its paying a fee for a much faster target.
The design of estimateSmartFee is to be used by the wallet to intelligently as possible automatically put a fee on a transaction being sent. In the GUI, the slider clearly indicates what target the fee is being returned for. For the RPC call estimatesmartfee, this is also clearly returned. The difficulty comes with automatic estimation that happens from a sendtoaddress or sendmany RPC call. If in the future we add a way to select a transaction confirm target with the desired RPC call, perhaps it would make sense to fail, but for now it seems to me that its more important for the transaction to go out, even if its paying a fee for a much faster target.
fileout << CLIENT_VERSION; // version that wrote the file | ||
fileout << nBestSeenHeight; | ||
if (BlockSpan() > HistoricalBlockSpan()/2) { |
TheBlueMatt
Apr 28, 2017
Contributor
This seems like a sharp edge - you can restart a node and end up getting a fee estimate much higher than 30 seconds ago before you shut down. Maybe change the /2 to historicalBest more recent than OLDEST_ESTIMATE_HISTORY*3/4 or so? Would still have the issue but at least its less likely to be hit for those who regularly run a node just during the day or so?
This seems like a sharp edge - you can restart a node and end up getting a fee estimate much higher than 30 seconds ago before you shut down. Maybe change the /2 to historicalBest more recent than OLDEST_ESTIMATE_HISTORY*3/4 or so? Would still have the issue but at least its less likely to be hit for those who regularly run a node just during the day or so?
morcos
May 4, 2017
Author
Member
After discussion, I agree that this logic could be more intelligent, but I think that can be left for a later improvement.
After discussion, I agree that this logic could be more intelligent, but I think that can be left for a later improvement.
utACK 57e71c8 provided If I find the time I'll do some real-world testing once the buckets fill up. |
@@ -53,13 +55,17 @@ class TxConfirmStats | |||
|
|||
double decay; | |||
|
|||
unsigned int scale; |
instagibbs
May 1, 2017
Member
Add description?
Add description?
@@ -68,7 +74,8 @@ class TxConfirmStats | |||
* @param maxConfirms max number of confirms to track | |||
* @param decay how much to decay the historical moving average per block | |||
*/ | |||
TxConfirmStats(const std::vector<double>& defaultBuckets, unsigned int maxConfirms, double decay); | |||
TxConfirmStats(const std::vector<double>& defaultBuckets, const std::map<double, unsigned int>& defaultBucketMap, |
instagibbs
May 1, 2017
Member
add description above?
add description above?
/** Estimate feerate needed to get be included in a block within confTarget | ||
* blocks. If no answer can be given at confTarget, return an estimate at | ||
* the closest target where one can be given. 'conservative' estimates are | ||
* valid over longer time horizons also. |
instagibbs
May 1, 2017
Member
teeny nit: "also" unneeded
teeny nit: "also" unneeded
confTarget = maxUsableEstimate; | ||
} | ||
|
||
assert(confTarget > 0); //estimateCombinedFee and estimateConservativeFee take unsigned ints |
instagibbs
May 1, 2017
Member
I think this is impossible to hit, due to confTarget <= 0
check and maxUsableEstimate <= 1
check. (though this might make sense as belt and suspenders)
I think this is impossible to hit, due to confTarget <= 0
check and maxUsableEstimate <= 1
check. (though this might make sense as belt and suspenders)
"\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n" | ||
"confirmation within nblocks blocks if possible and return the number of blocks\n" | ||
"for which the estimate is valid. Uses virtual transaction size as defined\n" | ||
"in BIP 141 (witness data is discounted).\n" | ||
"\nArguments:\n" | ||
"1. nblocks (numeric)\n" | ||
"1. nblocks (numeric)\n" | ||
"2. conservative (bool, optional, default=true) Whether to return a more conservative estimate calculated from a longer history\n" |
instagibbs
May 1, 2017
Member
It's not clear to me as the API reader what "conservative" means here, depending on if I'm worrying about over-paying or waiting too long, or being conservative in the amount of data I'm using to estimate.
suggested: "Whether to return a possibly higher feerate estimate calculated from a longer history"
It's not clear to me as the API reader what "conservative" means here, depending on if I'm worrying about over-paying or waiting too long, or being conservative in the amount of data I'm using to estimate.
suggested: "Whether to return a possibly higher feerate estimate calculated from a longer history"
" \"decay\" : x.x, (numeric) exponential decay for historical moving average of confirmation data\n" | ||
" \"scale\" : x, (numeric) The resolution of confirmation targets at this time horizon\n" | ||
" \"pass.\" information about the last range of feerates to succeed in meeting the threshold\n" | ||
" \"fail.\" information about the first range of feerates to fail to meet the threshold\n" |
instagibbs
May 1, 2017
Member
nit: s/first/highest/
nit: s/first/highest/
" \"feerate\" : x.x, (numeric) estimate fee-per-kilobyte (in BTC)\n" | ||
" \"decay\" : x.x, (numeric) exponential decay for historical moving average of confirmation data\n" | ||
" \"scale\" : x, (numeric) The resolution of confirmation targets at this time horizon\n" | ||
" \"pass.\" information about the last range of feerates to succeed in meeting the threshold\n" |
instagibbs
May 1, 2017
Member
nit: s/last/lowest/
nit: s/last/lowest/
" \"startrange\" : x.x, (numeric) start of feerate range\n" | ||
" \"endrange\" : x.x, (numeric) end of feerate range\n" | ||
" \"withintarget\" : x.x, (numeric) number of txs over history horizon in the feerate range that were confirmed within target\n" | ||
" \"totalconfirmed\" : x.x, (numeric) number of txs over history horizon in the feerate range total\n" |
instagibbs
May 1, 2017
Member
s/total/that were confirmed at any point/
s/total/that were confirmed at any point/
# estimatesmartfee should return the same result | ||
assert_equal(node.estimatesmartfee(i+1)["feerate"], e) | ||
if i >= 13: # for n>=14 estimatesmartfee(n/2) should be at least as high as estimatefee(n) | ||
assert(node.estimatesmartfee((i+1)//2)["feerate"] > float(e) - delta) |
instagibbs
May 1, 2017
Member
could also do a quick check that the transitive property is true for conservative
estimates
could also do a quick check that the transitive property is true for conservative
estimates
|
||
/** Require an avg of 0.1 tx in the combined feerate bucket per block to have stat significance */ | ||
static constexpr double SUFFICIENT_FEETXS = 0.1; | ||
/** Require an avg of 0.5 tx when using short decay since there are less blocks considered*/ |
instagibbs
May 1, 2017
Member
grammar nit: s/less/fewer/
grammar nit: s/less/fewer/
{ | ||
if (request.fHelp || request.params.size() < 1|| request.params.size() > 3) | ||
throw std::runtime_error( | ||
"estimaterawfee nblocks\n" |
instagibbs
May 1, 2017
Member
missing threshold
missing threshold
median = doubleEst; | ||
} | ||
|
||
if (conservative) { |
TheBlueMatt
May 1, 2017
Contributor
I believe we may want to add a check that the previous code actually got a result here. And, generally, that all of the four estimate*Fee calls here actually succeed. It seems super strange to accept only one result as fine if the rest fail (eg on startup you may get a response from short horizon on SUFFICIENT_TXS_SHORT, but not on medium/long, also @sdaftuar saw a case this morning where he had an offline node for a while which succeeded in getting a result for conservative but not for non-conservative for a 3 conftarget, I believe because estimateConservativeFee gave a result from the medium/long horizons when the short horizon could not).
I believe we may want to add a check that the previous code actually got a result here. And, generally, that all of the four estimate*Fee calls here actually succeed. It seems super strange to accept only one result as fine if the rest fail (eg on startup you may get a response from short horizon on SUFFICIENT_TXS_SHORT, but not on medium/long, also @sdaftuar saw a case this morning where he had an offline node for a while which succeeded in getting a result for conservative but not for non-conservative for a 3 conftarget, I believe because estimateConservativeFee gave a result from the medium/long horizons when the short horizon could not).
morcos
May 3, 2017
Author
Member
After offline discussion, it appears the bigger issue is what to do when the short time horizon estimates have decayed to not being able to give you an answer. It seems like what happens in conservative mode is probably the right thing where you take advantage of the fact that the longer time horizon estimates have not decayed yet. We probably want the ability to utilize this as a fall back in non-conservative mode as well. This can happen either from shutting down a node for a couple of days which means the short time horizon will decay too much, or even just from losing network activity from something like a laptop over the weekend.
As it is, I think its probably still a strict improvement to existing estimates, but could use further refinement.
After offline discussion, it appears the bigger issue is what to do when the short time horizon estimates have decayed to not being able to give you an answer. It seems like what happens in conservative mode is probably the right thing where you take advantage of the fact that the longer time horizon estimates have not decayed yet. We probably want the ability to utilize this as a fall back in non-conservative mode as well. This can happen either from shutting down a node for a couple of days which means the short time horizon will decay too much, or even just from losing network activity from something like a laptop over the weekend.
As it is, I think its probably still a strict improvement to existing estimates, but could use further refinement.
if (!conservative) { | ||
// When we aren't using conservative estimates, if a lower confTarget from a more recent horizon returns a lower answer use it. | ||
double medMax = feeStats->EstimateMedianVal(feeStats->GetMaxConfirms(), SUFFICIENT_FEETXS, successThreshold, true, nBestSeenHeight); | ||
if (estimate == -1 || medMax < estimate) { |
morcos
May 3, 2017
Author
Member
in all 3 of these replacement evaluations (also line 709 and line 722), we probably don't want to do the replacement if the new estimate is -1, should be rare.
in all 3 of these replacement evaluations (also line 709 and line 722), we probably don't want to do the replacement if the new estimate is -1, should be rare.
Addressed the comments in fixup commmits e95eca7 (smarterfee.ver5) and then squashed --> 9a34419 (smarterfee.ver5.squash) |
Code review ACK, with some nits. I haven't reviewed the changes to the estimation algorithm itself. |
@@ -580,10 +587,15 @@ bool CBlockPolicyEstimator::Write(CAutoFile& fileout) const | |||
{ | |||
try { | |||
LOCK(cs_feeEstimator); | |||
fileout << 139900; // version required to read: 0.13.99 or later | |||
fileout << 149900; // version required to read: 0.14.99 or later |
sipa
May 9, 2017
Member
Any reason to keep this number tied to client versions instead of just an independent "fee estimates file version number"?
Any reason to keep this number tied to client versions instead of just an independent "fee estimates file version number"?
morcos
May 9, 2017
Author
Member
no reason..., but this also seems simple enough?
no reason..., but this also seems simple enough?
@@ -141,19 +207,38 @@ class CBlockPolicyEstimator | |||
|
|||
/** Classes to track historical data on transaction confirmations */ | |||
TxConfirmStats* feeStats; | |||
TxConfirmStats* shortStats; |
sipa
May 9, 2017
Member
Convert these to std::unique_ptrs?
Convert these to std::unique_ptrs?
morcos
May 9, 2017
Author
Member
This was the change made in #9942, and I couldn't get unique_ptrs to work while also moving the code to the cpp file. I now also have other ideas how to clean up the wonky design of TxConfirmStats inside CBlockPolicyEstimator, but I don't want to do it now and churn this PR.
This was the change made in #9942, and I couldn't get unique_ptrs to work while also moving the code to the cpp file. I now also have other ideas how to clean up the wonky design of TxConfirmStats inside CBlockPolicyEstimator, but I don't want to do it now and churn this PR.
@@ -159,6 +171,10 @@ class CBlockPolicyEstimator | |||
|
|||
class FeeFilterRounder | |||
{ | |||
private: | |||
static constexpr double MAX_FILTER_FEERATE = 1e7; | |||
static constexpr double FEE_FILTER_SPACING = 1.1; |
sipa
May 9, 2017
Member
Can you add a comment to explain the relation with the CBlockPolicyEstimator::FEE_SPACE constant?
Can you add a comment to explain the relation with the CBlockPolicyEstimator::FEE_SPACE constant?
morcos
May 9, 2017
Author
Member
yeah, there is no relation.
when I made the filter spacing, I just copied the other one arbitrarily, but then I decided if I was changing the estimate spacing, there was no reason to also make a change to the filter spacing. I can add a comment.
yeah, there is no relation.
when I made the filter spacing, I just copied the other one arbitrarily, but then I decided if I was changing the estimate spacing, there was no reason to also make a change to the filter spacing. I can add a comment.
"\nWARNING: This interface is unstable and may disappear or change!\n" | ||
"\nWARNING: This is an advanced API call that is tightly coupled to the specific\n" | ||
" implementation of fee estimation. The parameters it can be called with\n" | ||
" and the results it returns will change if the internal implementaion changes.\n" |
morcos
May 9, 2017
Author
Member
oops
oops
if (horizonInt < 0 || horizonInt > 2) { | ||
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid horizon for fee estimates"); | ||
} | ||
else { |
sipa
May 9, 2017
Member
Style nit: else
on the same line as }
Style nit: else
on the same line as }
morcos
May 9, 2017
Author
Member
kk
kk
"2. threshold (numeric, optional) The proportion of transactions in a given feerate range that must have been\n" | ||
" confirmed within nblocks in order to consider those feerates as high enough and proceed to check\n" | ||
" lower buckets. Default: 0.95\n" | ||
"3. horizon (numeric, optional) How long a history of estimates to consider. 0=short, 1=medium, 2=long.\n" |
sipa
May 9, 2017
Member
To make the RPC interface less dependent on the specific implementation, perhaps don't make horizon a parameter, and just return information for all applicable horizons for the given value of nblocks?
To make the RPC interface less dependent on the specific implementation, perhaps don't make horizon a parameter, and just return information for all applicable horizons for the given value of nblocks?
morcos
May 9, 2017
Author
Member
slightly prefer existing, but I could change it if prevailing opinion is otherwise..
estimaterawfee is meant to be implementation dependent
slightly prefer existing, but I could change it if prevailing opinion is otherwise..
estimaterawfee is meant to be implementation dependent
sipa
May 17, 2017
Member
The reason for asking is that while the output of this RPC may be implementation dependent, it would be nice if (to the extent possible), its arguments aren't. The specific buckets used seem much more likely to change in other revisions.
Just a nit.
The reason for asking is that while the output of this RPC may be implementation dependent, it would be nice if (to the extent possible), its arguments aren't. The specific buckets used seem much more likely to change in other revisions.
Just a nit.
|
||
double CBlockPolicyEstimator::estimateCombinedFee(unsigned int confTarget, double successThreshold, bool conservative) const | ||
{ | ||
double estimate = -1; |
sipa
May 9, 2017
Member
Can you write this function as a loop over the 3 horizons, instead of a cascaded if/then/else?
Can you write this function as a loop over the 3 horizons, instead of a cascaded if/then/else?
morcos
May 9, 2017
Author
Member
I can take a pass at rewriting the logic to be a bit clearer, but I'm not sure a loop is the right thing. There are basically 6 different combinations of estimates you might want to check depending on what range your target is in and whether or not you want to check shorter horizons. But the logic might be cleaner if those are just spelled out.
I can take a pass at rewriting the logic to be a bit clearer, but I'm not sure a loop is the right thing. There are basically 6 different combinations of estimates you might want to check depending on what range your target is in and whether or not you want to check shorter horizons. But the logic might be cleaner if those are just spelled out.
@@ -173,6 +173,10 @@ class CBlockPolicyEstimator | |||
private: | |||
CFeeRate minTrackedFee; //!< Passed to constructor to avoid dependency on main | |||
unsigned int nBestSeenHeight; | |||
unsigned int firstRecordedHeight; | |||
unsigned int historicalFirst; |
sipa
May 9, 2017
Member
Nit: unused until 2 commits later.
Nit: unused until 2 commits later.
morcos
May 9, 2017
Author
Member
I wanted to get the file format change over and done with...
I wanted to get the file format change over and done with...
unsigned int CBlockPolicyEstimator::MaxUsableEstimate() const | ||
{ | ||
// Block spans are divided by 2 to make sure there are enough potential failing data points for the estimate | ||
return std::min(longStats->GetMaxConfirms(), std::max(BlockSpan(), HistoricalBlockSpan()) / 2); |
sipa
May 9, 2017
Member
Nit: If you'd return which of the two BlockSpans was chosen in a pair or a bool, you wouldn't need to repeat the decision logic in the debug print statement of processBlock.
Nit: If you'd return which of the two BlockSpans was chosen in a pair or a bool, you wouldn't need to repeat the decision logic in the debug print statement of processBlock.
morcos
May 9, 2017
Author
Member
ok
ok
morcos
May 10, 2017
Author
Member
after actually doing this, it seemed uglier, so leaving it as is
after actually doing this, it seemed uglier, so leaving it as is
…zons. Make feerate buckets smaller (5% instead of 10%) and make the 3 different horizons have half lifes of 3 hours, 1 day and 1 week respectively.
Instead of stopping if it encounters a "sufficient" number of transactions which don't meet the threshold for being confirmed within the target, it keeps looking to add more transactions to see if there is a temporary blip in the data. This allows a smaller number of required data points.
Added an extra commit to improve JSON output as per @sipa. Was kind of annoying to squash, so will leave it if that's ok? @laanwj I think this is reviewed enough that we'd be better off merging and getting some more experimentation with the estimates.. It takes 2 full weeks for the longest estimates to even be exposed. The biggest open questions are whether I should add an ability to use old estimates during transition and whether we are going to build in GUI support. The tests could also use some improvement but I don't think they're notably worse than they were. |
utACK 38bc1ec |
utACK 38bc1ec |
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
I tried reading the description from help, and I don't really understand this:
but from the actual return:
Without reading the code, it's not clear what |
@RHavar without actually looking at the code, I believe this refers to the "cheapest bucket" block which that feerate was found. |
@instagibbs Yeah, you seem right. But I don't think that was inferable from the help information :P What's the motivation for
? Anyway, fantastic work. This is a huge improvement, something that has been long needed. Well done @morcos |
I've updated https://estimatefee.com/ to now use The bitcoin node has only been up a few hours, so it'll probably take a while to collect enough information. P.S. you kinda broke my domain name lol |
@RHavar you might be right that the description is not the clearest. |
@morcos In that case I'd suggest renaming it from "blocks" to "target" and changing the description to what you wrote above :P |
Not sure where to put it, but just FYI I made a simple tool that looks up smart fees once per 10 minutes and puts it here https://estimatesmartfee.com/html.html or https://estimatesmartfee.com/json.json If you want to use it for testing or something |
also FYIs, I put together a small bash script that uses this for quickly grabbing fees from the command line: https://bitcoin.stackexchange.com/a/61348/1235 thanks! |
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
38bc1ec Make more json-like output from estimaterawfee (Alex Morcos) 2d2e170 Comments and improved documentation (Alex Morcos) ef589f8 minor cleanup: remove unnecessary variable (Alex Morcos) 3ee76d6 Introduce a scale factor (Alex Morcos) 5f1f0c6 Historical block span (Alex Morcos) aa19b8e Clean up fee estimate debug printing (Alex Morcos) 10f7cbd Track first recorded height (Alex Morcos) 3810e97 Rewrite estimateSmartFee (Alex Morcos) c7447ec Track failures in fee estimation. (Alex Morcos) 4186d3f Expose estimaterawfee (Alex Morcos) 2681153 minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop. (Alex Morcos) 1ba43cc Make EstimateMedianVal smarter about small failures. (Alex Morcos) d3e30bc Refactor to update moving average on fly (Alex Morcos) e5007ba Change parameters for fee estimation and estimates on all 3 time horizons. (Alex Morcos) c0a273f Change file format for fee estimates. (Alex Morcos) Tree-SHA512: 186e7508d86a1f351bb656edcd84ee9091f5f2706331eda9ee29da9c8eb5bf67b8c1f2abf6662835560e7f613b1377099054f20767f41ddcdbc89c4f9e78946d
This is a substantial improvement of fee estimation code built off #9942.
Although in many cases the estimates will not appear to differ by much from previous estimates, they will handle some situations much better.
A longer writeup of the original algorithm and the changes in this PR can be found here:
https://gist.github.com/morcos/d3637f015bc4e607e1fd10d8351e9f41
A summary of major changes:
A summary of open issues: