Merge #19630: Cleanup fee estimation code

a3abeec33a6ae903e514c7a7b6f587b7c17288a0 policy/fees: remove a floating-point division by zero (Antoine Poinsot)
c36869bbf6a38626833b4aea53be024c48ede475 policy/fees: unify some duplicated for loops (Antoine Poinsot)
569d92a4d2924a1f6d50775980b591552f6372e7 policy/fees: small readability improvements (Antoine Poinsot)
5b8cb35621891b681f9b49a9de5f6d8da4ccdecc policy/fee: remove requireGreater parameter in EstimateMedianVal() (Antoine Poinsot)
dba8196b447b6a85be66890db70928100e867d8b 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 a3abeec33a6ae903e514c7a7b6f587b7c17288a0
  MarcoFalke:
    ACK a3abeec33a6ae903e514c7a7b6f587b7c17288a0 💹
  ariard:
    Code Review ACK a3abeec.

Tree-SHA512: b7620bcd23a2ffa8f7ed859467868fc0f6488279e3ee634f6d408872cb866ad086a037e8ace76599a05b7e9c07768adf5016b0ae782d153196b9c030db4c34a5

3 files changed, 56 insertions, 61 deletions

diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
index 25458eead2..0f31093dbb 100644
--- a/src/policy/fees.cpp
+++ b/src/policy/fees.cpp
@@ -55,7 +55,7 @@ private:
 
     // 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
@@ -114,12 +114,10 @@ public:
      * @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 */
@@ -139,22 +137,18 @@ public:
 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());
 }
@@ -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();
@@ -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;
@@ -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
@@ -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);
@@ -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;
             }
         }
@@ -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);
 
 
@@ -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;
@@ -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;
@@ -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);
@@ -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;
@@ -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;
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 @@ private:
 
     /** 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*/
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