Fix fee estimate rounding errors

Summary:
Fixes an issue that has been brought up where fees were truncated down during estimation,
causing the estimatefee rpc to return values less than the current minimum relay feerate.

We replace default truncation with mathimatical ceilings everywhere relevant for estimating
a fee to be paid. Note well: we leave all minimum calculation alone so as to not underbill
by accident.

Builds on D1670

Inspired by Core PR 11303 by Matt Corallo.

Test Plan:
  make VERBOSE=1 check && ./test/functional/test_runner.py --extended

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: teamcity

Differential Revision: https://reviews.bitcoinabc.org/D1815

src/policy/fees.cpp

@@ -74,10 +74,11 @@ void TxConfirmStats::UpdateMovingAverages() {
 }
 
 // returns -1 on error conditions
-double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
-                                         double successBreakPoint,
-                                         bool requireGreater,
-                                         unsigned int nBlockHeight) {
+CFeeRate TxConfirmStats::EstimateMedianFeeRate(int confTarget,
+                                               double sufficientTxVal,
+                                               double successBreakPoint,
+                                               bool requireGreater,
+                                               unsigned int nBlockHeight) {
     // Counters for a bucket (or range of buckets)
     // Number of tx's confirmed within the confTarget
     double nConf = 0;
@@ -186,7 +187,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
              buckets[maxBucket], 100 * nConf / (totalNum + extraNum), nConf,
              totalNum, extraNum);
 
-    return median;
+    return CFeeRate(int64_t(ceill(median)) * SATOSHI);
 }
 
 void TxConfirmStats::Write(CAutoFile &fileout) {
@@ -464,14 +465,14 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) {
         return CFeeRate(Amount::zero());
     }
 
-    double median = feeStats.EstimateMedianVal(
+    CFeeRate median = feeStats.EstimateMedianFeeRate(
         confTarget, SUFFICIENT_FEETXS, MIN_SUCCESS_PCT, true, nBestSeenHeight);
 
-    if (median < 0) {
+    if (median < CFeeRate(Amount::zero())) {
         return CFeeRate(Amount::zero());
     }
 
-    return CFeeRate(int64_t(median) * SATOSHI);
+    return median;
 }
 
 CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget,
@@ -491,12 +492,12 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget,
         confTarget = 2;
     }
 
-    double median = -1;
-    while (median < 0 &&
-           (unsigned int)confTarget <= feeStats.GetMaxConfirms()) {
-        median =
-            feeStats.EstimateMedianVal(confTarget++, SUFFICIENT_FEETXS,
-                                       MIN_SUCCESS_PCT, true, nBestSeenHeight);
+    CFeeRate median = CFeeRate(-1 * SATOSHI);
+    while (median < CFeeRate(Amount::zero()) &&
+           uint32_t(confTarget) <= feeStats.GetMaxConfirms()) {
+        median = feeStats.EstimateMedianFeeRate(confTarget++, SUFFICIENT_FEETXS,
+                                                MIN_SUCCESS_PCT, true,
+                                                nBestSeenHeight);
     }
 
     if (answerFoundAtTarget) {
@@ -509,16 +510,15 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget,
         pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) *
                        1000000)
             .GetFeePerK();
-    if (minPoolFee > Amount::zero() &&
-        minPoolFee > (int64_t(median) * SATOSHI)) {
+    if (minPoolFee > Amount::zero() && minPoolFee > median.GetFeePerK()) {
         return CFeeRate(minPoolFee);
     }
 
-    if (median < 0) {
+    if (median < CFeeRate(Amount::zero())) {
         return CFeeRate(Amount::zero());
     }
 
-    return CFeeRate(int64_t(median) * SATOSHI);
+    return median;
 }
 
 void CBlockPolicyEstimator::Write(CAutoFile &fileout) {

src/policy/fees.h

@@ -166,9 +166,9 @@ class TxConfirmStats {
      * minSuccess
      * @param nBlockHeight the current block height
      */
-    double EstimateMedianVal(int confTarget, double sufficientTxVal,
-                             double minSuccess, bool requireGreater,
-                             unsigned int nBlockHeight);
+    CFeeRate EstimateMedianFeeRate(int confTarget, double sufficientTxVal,
+                                   double minSuccess, bool requireGreater,
+                                   unsigned int nBlockHeight);
 
     /** Return the max number of confirms we're tracking */
     unsigned int GetMaxConfirms() { return confAvg.size(); }

src/test/mempool_tests.cpp

@@ -658,7 +658,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) {
                 CTxMemPool::ROLLING_FEE_HALFLIFE / 4);
     BOOST_CHECK_EQUAL(
         pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(),
-        (maxFeeRateRemoved.GetFeePerK() + feeIncrement) / 8);
+        (maxFeeRateRemoved.GetFeePerK() + feeIncrement) / 8 + SATOSHI);
     // ... with a 1/4 halflife when mempool is < 1/4 its target size
 
     SetMockTime(0);

src/txmempool.cpp

@@ -1180,7 +1180,7 @@ CTxMemPool::GetMemPoolChildren(txiter entry) const {
 CFeeRate CTxMemPool::GetMinFee(size_t sizelimit) const {
     LOCK(cs);
     if (!blockSinceLastRollingFeeBump || rollingMinimumFeeRate == 0) {
-        return CFeeRate(int64_t(rollingMinimumFeeRate) * SATOSHI);
+        return CFeeRate(int64_t(ceill(rollingMinimumFeeRate)) * SATOSHI);
     }
 
     int64_t time = GetTime();
@@ -1197,7 +1197,7 @@ CFeeRate CTxMemPool::GetMinFee(size_t sizelimit) const {
             pow(2.0, (time - lastRollingFeeUpdate) / halflife);
         lastRollingFeeUpdate = time;
     }
-    return CFeeRate(int64_t(rollingMinimumFeeRate) * SATOSHI);
+    return CFeeRate(int64_t(ceill(rollingMinimumFeeRate)) * SATOSHI);
 }
 
 void CTxMemPool::trackPackageRemoved(const CFeeRate &rate) {