From dba8196b447b6a85be66890db70928100e867d8b Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Jul 2020 18:33:57 +0200 Subject: [PATCH 1/5] policy/fees: correct decay explanation comments This was confusing: which one is the good one ? After testing the value is right but not the comment, so fix it. Signed-off-by: Antoine Poinsot --- src/policy/fees.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/policy/fees.h b/src/policy/fees.h index e79dbc9868..8ea8816dc3 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -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*/ From 5b8cb35621891b681f9b49a9de5f6d8da4ccdecc Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Jul 2020 18:48:57 +0200 Subject: [PATCH 2/5] policy/fee: remove requireGreater parameter in EstimateMedianVal() It was always passed as true, and complicates the (already complex) logic of the function. Signed-off-by: Antoine Poinsot --- src/policy/fees.cpp | 50 ++++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 25458eead2..b44e16de4f 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -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 */ @@ -206,8 +204,8 @@ void TxConfirmStats::UpdateMovingAverages() // 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 @@ -215,25 +213,17 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, 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; - // 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(); @@ -242,8 +232,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; @@ -263,7 +253,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); @@ -338,8 +328,8 @@ 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, + 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), passBucket.withinTarget, passBucket.totalConfirmed, passBucket.inMempool, passBucket.leftMempool, @@ -664,7 +654,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); @@ -725,26 +715,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; @@ -763,10 +753,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; From 569d92a4d2924a1f6d50775980b591552f6372e7 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Jul 2020 21:19:38 +0200 Subject: [PATCH 3/5] policy/fees: small readability improvements Signed-off-by: Antoine Poinsot --- src/policy/fees.cpp | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index b44e16de4f..7820d00fd6 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -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 avg; + std::vector 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 @@ -137,11 +137,9 @@ class TxConfirmStats TxConfirmStats::TxConfirmStats(const std::vector& defaultBuckets, const std::map& 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()); @@ -152,7 +150,7 @@ TxConfirmStats::TxConfirmStats(const std::vector& defaultBuckets, } txCtAvg.resize(buckets.size()); - avg.resize(buckets.size()); + m_feerate_avg.resize(buckets.size()); resizeInMemoryCounters(buckets.size()); } @@ -170,24 +168,24 @@ 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() @@ -197,8 +195,8 @@ void TxConfirmStats::UpdateMovingAverages() 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; + m_feerate_avg[j] *= decay; + txCtAvg[j] *= decay; } } @@ -212,8 +210,8 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, 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; + 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 @@ -243,7 +241,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 @@ -307,7 +305,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; } } @@ -351,7 +349,7 @@ void TxConfirmStats::Write(CAutoFile& fileout) const { fileout << decay; fileout << scale; - fileout << avg; + fileout << m_feerate_avg; fileout << txCtAvg; fileout << confAvg; fileout << failAvg; @@ -374,8 +372,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; From c36869bbf6a38626833b4aea53be024c48ede475 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 31 Jul 2020 20:28:40 +0200 Subject: [PATCH 4/5] policy/fees: unify some duplicated for loops Signed-off-by: Antoine Poinsot --- src/policy/fees.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 7820d00fd6..47ff4a9cbf 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -141,11 +141,9 @@ TxConfirmStats::TxConfirmStats(const std::vector& defaultBuckets, { assert(_scale != 0 && "_scale must be non-zero"); 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()); } @@ -190,11 +188,12 @@ void TxConfirmStats::Record(int blocksToConfirm, double 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; + 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; } From a3abeec33a6ae903e514c7a7b6f587b7c17288a0 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 15 Aug 2020 01:57:59 +0200 Subject: [PATCH 5/5] policy/fees: remove a floating-point division by zero Reported-by: practicalswift Signed-off-by: Antoine Poinsot --- src/policy/fees.cpp | 13 +++++++++++-- test/sanitizer_suppressions/ubsan | 1 - 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 47ff4a9cbf..0f31093dbb 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -325,13 +325,22 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, failBucket.leftMempool = failNum; } + 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); diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index b3d9b9e6ec..75257d886b 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -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