Skip to content

Commit

Permalink
Merge #19630: Cleanup fee estimation code
Browse files Browse the repository at this point in the history 
a3abeec policy/fees: remove a floating-point division by zero (Antoine Poinsot)
c36869b policy/fees: unify some duplicated for loops (Antoine Poinsot)
569d92a policy/fees: small readability improvements (Antoine Poinsot)
5b8cb35 policy/fee: remove requireGreater parameter in EstimateMedianVal() (Antoine Poinsot)
dba8196 policy/fees: correct decay explanation comments (Antoine Poinsot)

Pull request description:

  This (*does not* change behaviour and) cleans up a bit of unused code in `CBlockPolicyEstimator` and friends, and slightly improves readability of the rest (comment correction etc.). The last commit is a small reformatting one which I could not resist but am happy to remove at will.

ACKs for top commit:
  jnewbery:
    utACK a3abeec
  MarcoFalke:
    ACK a3abeec 💹
  ariard:
    Code Review ACK a3abeec.

Tree-SHA512: b7620bcd23a2ffa8f7ed859467868fc0f6488279e3ee634f6d408872cb866ad086a037e8ace76599a05b7e9c07768adf5016b0ae782d153196b9c030db4c34a5
  • Loading branch information
@fanquake
fanquake committed Sep 29, 2020
2 parents 6af9b31 + a3abeec commit ec9b449
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 61 deletions.
112 changes: 54 additions & 58 deletions src/policy/fees.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class TxConfirmStats

// Sum the total feerate of all tx's in each bucket
// Track the historical moving average of this total over blocks
std::vector<double> avg;
std::vector<double> m_feerate_avg;

// Combine the conf counts with tx counts to calculate the confirmation % for each Y,X
// Combine the total value with the tx counts to calculate the avg feerate per bucket
Expand Down Expand Up @@ -114,12 +114,10 @@ class TxConfirmStats
* @param confTarget target number of confirmations
* @param sufficientTxVal required average number of transactions per block in a bucket range
* @param minSuccess the success probability we require
* @param requireGreater return the lowest feerate such that all higher values pass minSuccess OR
* return the highest feerate such that all lower values fail minSuccess
* @param nBlockHeight the current block height
*/
double EstimateMedianVal(int confTarget, double sufficientTxVal,
double minSuccess, bool requireGreater, unsigned int nBlockHeight,
double minSuccess, unsigned int nBlockHeight,
EstimationResult *result = nullptr) const;

/** Return the max number of confirms we're tracking */
Expand All @@ -139,22 +137,18 @@ class TxConfirmStats
TxConfirmStats::TxConfirmStats(const std::vector<double>& defaultBuckets,
const std::map<double, unsigned int>& defaultBucketMap,
unsigned int maxPeriods, double _decay, unsigned int _scale)
: buckets(defaultBuckets), bucketMap(defaultBucketMap)
: buckets(defaultBuckets), bucketMap(defaultBucketMap), decay(_decay), scale(_scale)
{
decay = _decay;
assert(_scale != 0 && "_scale must be non-zero");
scale = _scale;
confAvg.resize(maxPeriods);
for (unsigned int i = 0; i < maxPeriods; i++) {
confAvg[i].resize(buckets.size());
}
failAvg.resize(maxPeriods);
for (unsigned int i = 0; i < maxPeriods; i++) {
confAvg[i].resize(buckets.size());
failAvg[i].resize(buckets.size());
}

txCtAvg.resize(buckets.size());
avg.resize(buckets.size());
m_feerate_avg.resize(buckets.size());

resizeInMemoryCounters(buckets.size());
}
Expand All @@ -172,68 +166,61 @@ void TxConfirmStats::resizeInMemoryCounters(size_t newbuckets) {
void TxConfirmStats::ClearCurrent(unsigned int nBlockHeight)
{
for (unsigned int j = 0; j < buckets.size(); j++) {
oldUnconfTxs[j] += unconfTxs[nBlockHeight%unconfTxs.size()][j];
oldUnconfTxs[j] += unconfTxs[nBlockHeight % unconfTxs.size()][j];
unconfTxs[nBlockHeight%unconfTxs.size()][j] = 0;
}
}


void TxConfirmStats::Record(int blocksToConfirm, double val)
void TxConfirmStats::Record(int blocksToConfirm, double feerate)
{
// blocksToConfirm is 1-based
if (blocksToConfirm < 1)
return;
int periodsToConfirm = (blocksToConfirm + scale - 1)/scale;
unsigned int bucketindex = bucketMap.lower_bound(val)->second;
int periodsToConfirm = (blocksToConfirm + scale - 1) / scale;
unsigned int bucketindex = bucketMap.lower_bound(feerate)->second;
for (size_t i = periodsToConfirm; i <= confAvg.size(); i++) {
confAvg[i - 1][bucketindex]++;
}
txCtAvg[bucketindex]++;
avg[bucketindex] += val;
m_feerate_avg[bucketindex] += feerate;
}

void TxConfirmStats::UpdateMovingAverages()
{
assert(confAvg.size() == failAvg.size());
for (unsigned int j = 0; j < buckets.size(); j++) {
for (unsigned int i = 0; i < confAvg.size(); i++)
confAvg[i][j] = confAvg[i][j] * decay;
for (unsigned int i = 0; i < failAvg.size(); i++)
failAvg[i][j] = failAvg[i][j] * decay;
avg[j] = avg[j] * decay;
txCtAvg[j] = txCtAvg[j] * decay;
for (unsigned int i = 0; i < confAvg.size(); i++) {
confAvg[i][j] *= decay;
failAvg[i][j] *= decay;
}
m_feerate_avg[j] *= decay;
txCtAvg[j] *= decay;
}
}

// returns -1 on error conditions
double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
double successBreakPoint, bool requireGreater,
unsigned int nBlockHeight, EstimationResult *result) const
double successBreakPoint, unsigned int nBlockHeight,
EstimationResult *result) const
{
// Counters for a bucket (or range of buckets)
double nConf = 0; // Number of tx's confirmed within the confTarget
double totalNum = 0; // Total number of tx's that were ever confirmed
int extraNum = 0; // Number of tx's still in mempool for confTarget or longer
double failNum = 0; // Number of tx's that were never confirmed but removed from the mempool after confTarget
int periodTarget = (confTarget + scale - 1)/scale;

int maxbucketindex = buckets.size() - 1;

// requireGreater means we are looking for the lowest feerate such that all higher
// values pass, so we start at maxbucketindex (highest feerate) and look at successively
// smaller buckets until we reach failure. Otherwise, we are looking for the highest
// feerate such that all lower values fail, and we go in the opposite direction.
unsigned int startbucket = requireGreater ? maxbucketindex : 0;
int step = requireGreater ? -1 : 1;
const int periodTarget = (confTarget + scale - 1) / scale;
const int maxbucketindex = buckets.size() - 1;

// We'll combine buckets until we have enough samples.
// The near and far variables will define the range we've combined
// The best variables are the last range we saw which still had a high
// enough confirmation rate to count as success.
// The cur variables are the current range we're counting.
unsigned int curNearBucket = startbucket;
unsigned int bestNearBucket = startbucket;
unsigned int curFarBucket = startbucket;
unsigned int bestFarBucket = startbucket;
unsigned int curNearBucket = maxbucketindex;
unsigned int bestNearBucket = maxbucketindex;
unsigned int curFarBucket = maxbucketindex;
unsigned int bestFarBucket = maxbucketindex;

bool foundAnswer = false;
unsigned int bins = unconfTxs.size();
Expand All @@ -242,8 +229,8 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
EstimatorBucket passBucket;
EstimatorBucket failBucket;

// Start counting from highest(default) or lowest feerate transactions
for (int bucket = startbucket; bucket >= 0 && bucket <= maxbucketindex; bucket += step) {
// Start counting from highest feerate transactions
for (int bucket = maxbucketindex; bucket >= 0; --bucket) {
if (newBucketRange) {
curNearBucket = bucket;
newBucketRange = false;
Expand All @@ -253,7 +240,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
totalNum += txCtAvg[bucket];
failNum += failAvg[periodTarget - 1][bucket];
for (unsigned int confct = confTarget; confct < GetMaxConfirms(); confct++)
extraNum += unconfTxs[(nBlockHeight - confct)%bins][bucket];
extraNum += unconfTxs[(nBlockHeight - confct) % bins][bucket];
extraNum += oldUnconfTxs[bucket];
// If we have enough transaction data points in this range of buckets,
// we can test for success
Expand All @@ -263,7 +250,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
double curPct = nConf / (totalNum + failNum + extraNum);

// Check to see if we are no longer getting confirmed at the success rate
if ((requireGreater && curPct < successBreakPoint) || (!requireGreater && curPct > successBreakPoint)) {
if (curPct < successBreakPoint) {
if (passing == true) {
// First time we hit a failure record the failed bucket
unsigned int failMinBucket = std::min(curNearBucket, curFarBucket);
Expand Down Expand Up @@ -317,7 +304,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
if (txCtAvg[j] < txSum)
txSum -= txCtAvg[j];
else { // we're in the right bucket
median = avg[j] / txCtAvg[j];
median = m_feerate_avg[j] / txCtAvg[j];
break;
}
}
Expand All @@ -338,13 +325,22 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
failBucket.leftMempool = failNum;
}

LogPrint(BCLog::ESTIMATEFEE, "FeeEst: %d %s%.0f%% decay %.5f: feerate: %g from (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
confTarget, requireGreater ? ">" : "<", 100.0 * successBreakPoint, decay,
float passed_within_target_perc = 0.0;
float failed_within_target_perc = 0.0;
if ((passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool)) {
passed_within_target_perc = 100 * passBucket.withinTarget / (passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool);
}
if ((failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool)) {
failed_within_target_perc = 100 * failBucket.withinTarget / (failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool);
}

LogPrint(BCLog::ESTIMATEFEE, "FeeEst: %d > %.0f%% decay %.5f: feerate: %g from (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
confTarget, 100.0 * successBreakPoint, decay,
median, passBucket.start, passBucket.end,
100 * passBucket.withinTarget / (passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool),
passed_within_target_perc,
passBucket.withinTarget, passBucket.totalConfirmed, passBucket.inMempool, passBucket.leftMempool,
failBucket.start, failBucket.end,
100 * failBucket.withinTarget / (failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool),
failed_within_target_perc,
failBucket.withinTarget, failBucket.totalConfirmed, failBucket.inMempool, failBucket.leftMempool);


Expand All @@ -361,7 +357,7 @@ void TxConfirmStats::Write(CAutoFile& fileout) const
{
fileout << decay;
fileout << scale;
fileout << avg;
fileout << m_feerate_avg;
fileout << txCtAvg;
fileout << confAvg;
fileout << failAvg;
Expand All @@ -384,8 +380,8 @@ void TxConfirmStats::Read(CAutoFile& filein, int nFileVersion, size_t numBuckets
throw std::runtime_error("Corrupt estimates file. Scale must be non-zero");
}

filein >> avg;
if (avg.size() != numBuckets) {
filein >> m_feerate_avg;
if (m_feerate_avg.size() != numBuckets) {
throw std::runtime_error("Corrupt estimates file. Mismatch in feerate average bucket count");
}
filein >> txCtAvg;
Expand Down Expand Up @@ -664,7 +660,7 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
if (successThreshold > 1)
return CFeeRate(0);

double median = stats->EstimateMedianVal(confTarget, sufficientTxs, successThreshold, true, nBestSeenHeight, result);
double median = stats->EstimateMedianVal(confTarget, sufficientTxs, successThreshold, nBestSeenHeight, result);

if (median < 0)
return CFeeRate(0);
Expand Down Expand Up @@ -725,26 +721,26 @@ double CBlockPolicyEstimator::estimateCombinedFee(unsigned int confTarget, doubl
if (confTarget >= 1 && confTarget <= longStats->GetMaxConfirms()) {
// Find estimate from shortest time horizon possible
if (confTarget <= shortStats->GetMaxConfirms()) { // short horizon
estimate = shortStats->EstimateMedianVal(confTarget, SUFFICIENT_TXS_SHORT, successThreshold, true, nBestSeenHeight, result);
estimate = shortStats->EstimateMedianVal(confTarget, SUFFICIENT_TXS_SHORT, successThreshold, nBestSeenHeight, result);
}
else if (confTarget <= feeStats->GetMaxConfirms()) { // medium horizon
estimate = feeStats->EstimateMedianVal(confTarget, SUFFICIENT_FEETXS, successThreshold, true, nBestSeenHeight, result);
estimate = feeStats->EstimateMedianVal(confTarget, SUFFICIENT_FEETXS, successThreshold, nBestSeenHeight, result);
}
else { // long horizon
estimate = longStats->EstimateMedianVal(confTarget, SUFFICIENT_FEETXS, successThreshold, true, nBestSeenHeight, result);
estimate = longStats->EstimateMedianVal(confTarget, SUFFICIENT_FEETXS, successThreshold, nBestSeenHeight, result);
}
if (checkShorterHorizon) {
EstimationResult tempResult;
// If a lower confTarget from a more recent horizon returns a lower answer use it.
if (confTarget > feeStats->GetMaxConfirms()) {
double medMax = feeStats->EstimateMedianVal(feeStats->GetMaxConfirms(), SUFFICIENT_FEETXS, successThreshold, true, nBestSeenHeight, &tempResult);
double medMax = feeStats->EstimateMedianVal(feeStats->GetMaxConfirms(), SUFFICIENT_FEETXS, successThreshold, nBestSeenHeight, &tempResult);
if (medMax > 0 && (estimate == -1 || medMax < estimate)) {
estimate = medMax;
if (result) *result = tempResult;
}
}
if (confTarget > shortStats->GetMaxConfirms()) {
double shortMax = shortStats->EstimateMedianVal(shortStats->GetMaxConfirms(), SUFFICIENT_TXS_SHORT, successThreshold, true, nBestSeenHeight, &tempResult);
double shortMax = shortStats->EstimateMedianVal(shortStats->GetMaxConfirms(), SUFFICIENT_TXS_SHORT, successThreshold, nBestSeenHeight, &tempResult);
if (shortMax > 0 && (estimate == -1 || shortMax < estimate)) {
estimate = shortMax;
if (result) *result = tempResult;
Expand All @@ -763,10 +759,10 @@ double CBlockPolicyEstimator::estimateConservativeFee(unsigned int doubleTarget,
double estimate = -1;
EstimationResult tempResult;
if (doubleTarget <= shortStats->GetMaxConfirms()) {
estimate = feeStats->EstimateMedianVal(doubleTarget, SUFFICIENT_FEETXS, DOUBLE_SUCCESS_PCT, true, nBestSeenHeight, result);
estimate = feeStats->EstimateMedianVal(doubleTarget, SUFFICIENT_FEETXS, DOUBLE_SUCCESS_PCT, nBestSeenHeight, result);
}
if (doubleTarget <= feeStats->GetMaxConfirms()) {
double longEstimate = longStats->EstimateMedianVal(doubleTarget, SUFFICIENT_FEETXS, DOUBLE_SUCCESS_PCT, true, nBestSeenHeight, &tempResult);
double longEstimate = longStats->EstimateMedianVal(doubleTarget, SUFFICIENT_FEETXS, DOUBLE_SUCCESS_PCT, nBestSeenHeight, &tempResult);
if (longEstimate > estimate) {
estimate = longEstimate;
if (result) *result = tempResult;
Expand Down
4 changes: 2 additions & 2 deletions src/policy/fees.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ class CBlockPolicyEstimator

/** Decay of .962 is a half-life of 18 blocks or about 3 hours */
static constexpr double SHORT_DECAY = .962;
/** Decay of .998 is a half-life of 144 blocks or about 1 day */
/** Decay of .9952 is a half-life of 144 blocks or about 1 day */
static constexpr double MED_DECAY = .9952;
/** Decay of .9995 is a half-life of 1008 blocks or about 1 week */
/** Decay of .99931 is a half-life of 1008 blocks or about 1 week */
static constexpr double LONG_DECAY = .99931;

/** Require greater than 60% of X feerate transactions to be confirmed within Y/2 blocks*/
Expand Down
1 change: 0 additions & 1 deletion test/sanitizer_suppressions/ubsan
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# -fsanitize=undefined suppressions
# =================================
float-divide-by-zero:policy/fees.cpp
float-divide-by-zero:validation.cpp
float-divide-by-zero:wallet/wallet.cpp

Expand Down

0 comments on commit ec9b449

Please sign in to comment.