Merge #11303: Fix estimatesmartfee rounding display issue

1789e4675 Force explicit double -> int conversion for CFeeRate constructor (Matt Corallo)
53a6590f4 Make float <-> int casts explicit outside of test, qt, CFeeRate (Matt Corallo)
0b1b9148c Remove countMaskInv caching in bench framework (Matt Corallo)

Pull request description:

  This fixes an issue where estimatesmartfee which matches at the min relay fee will return 999 sat/byte instead of 1000 sat/byte due to a float rounding issue. I went ahead and made all float <-> int conversion outside of test/qt explicit (test only had one or two more, Qt had quite a few, including many in the Qt headers themselves) and added overloads to CFeeRate to force callers to do an explicit round themselves. Easy to test with -Wfloat-conversion.

Tree-SHA512: 66087b08e5dfca67506da54ae057c2f9d86184415e8fa4fa0199e38839e06a3ce96c836fcb7593b7d960065f5240c594ff3a0cfa14333ac528421f5aeac835c9

9 files changed, 23 insertions, 21 deletions

diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp
index 849d924af2..7b307d6f42 100644
--- a/src/bench/bench.cpp
+++ b/src/bench/bench.cpp
@@ -55,13 +55,13 @@ bool benchmark::State::KeepRunning()
     else {
         now = gettimedouble();
         double elapsed = now - lastTime;
-        double elapsedOne = elapsed * countMaskInv;
+        double elapsedOne = elapsed / (countMask + 1);
         if (elapsedOne < minTime) minTime = elapsedOne;
         if (elapsedOne > maxTime) maxTime = elapsedOne;
 
         // We only use relative values, so don't have to handle 64-bit wrap-around specially
         nowCycles = perf_cpucycles();
-        uint64_t elapsedOneCycles = (nowCycles - lastCycles) * countMaskInv;
+        uint64_t elapsedOneCycles = (nowCycles - lastCycles) / (countMask + 1);
         if (elapsedOneCycles < minCycles) minCycles = elapsedOneCycles;
         if (elapsedOneCycles > maxCycles) maxCycles = elapsedOneCycles;
 
@@ -69,7 +69,6 @@ bool benchmark::State::KeepRunning()
           // If the execution was much too fast (1/128th of maxElapsed), increase the count mask by 8x and restart timing.
           // The restart avoids including the overhead of this code in the measurement.
           countMask = ((countMask<<3)|7) & ((1LL<<60)-1);
-          countMaskInv = 1./(countMask+1);
           count = 0;
           minTime = std::numeric_limits<double>::max();
           maxTime = std::numeric_limits<double>::min();
@@ -81,7 +80,6 @@ bool benchmark::State::KeepRunning()
           uint64_t newCountMask = ((countMask<<1)|1) & ((1LL<<60)-1);
           if ((count & newCountMask)==0) {
               countMask = newCountMask;
-              countMaskInv = 1./(countMask+1);
           }
         }
     }
diff --git a/src/bench/bench.h b/src/bench/bench.h
index 1f36f2a4bc..79109eaa56 100644
--- a/src/bench/bench.h
+++ b/src/bench/bench.h
@@ -41,7 +41,7 @@ namespace benchmark {
         std::string name;
         double maxElapsed;
         double beginTime;
-        double lastTime, minTime, maxTime, countMaskInv;
+        double lastTime, minTime, maxTime;
         uint64_t count;
         uint64_t countMask;
         uint64_t beginCycles;
@@ -55,7 +55,6 @@ namespace benchmark {
             minCycles = std::numeric_limits<uint64_t>::max();
             maxCycles = std::numeric_limits<uint64_t>::min();
             countMask = 1;
-            countMaskInv = 1./(countMask + 1);
         }
         bool KeepRunning();
     };
diff --git a/src/policy/feerate.h b/src/policy/feerate.h
index 7e519e3efa..3449cdd699 100644
--- a/src/policy/feerate.h
+++ b/src/policy/feerate.h
@@ -20,10 +20,15 @@ class CFeeRate
 {
 private:
     CAmount nSatoshisPerK; // unit is satoshis-per-1,000-bytes
+
 public:
     /** Fee rate of 0 satoshis per kB */
     CFeeRate() : nSatoshisPerK(0) { }
-    explicit CFeeRate(const CAmount& _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) { }
+    template<typename I>
+    CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
+        // We've previously had bugs creep in from silent double->int conversion...
+        static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
+    }
     /** Constructor for a fee rate in satoshis per kB. The size in bytes must not exceed (2^63 - 1)*/
     CFeeRate(const CAmount& nFeePaid, size_t nBytes);
     /**
diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
index b9476407cf..8056f385ab 100644
--- a/src/policy/fees.cpp
+++ b/src/policy/fees.cpp
@@ -714,7 +714,7 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
     if (median < 0)
         return CFeeRate(0);
 
-    return CFeeRate(median);
+    return CFeeRate(llround(median));
 }
 
 unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon horizon) const
@@ -901,7 +901,7 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation
 
     if (median < 0) return CFeeRate(0); // error condition
 
-    return CFeeRate(median);
+    return CFeeRate(llround(median));
 }
 
 
@@ -1043,5 +1043,5 @@ CAmount FeeFilterRounder::round(CAmount currentMinFee)
     if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) {
         it--;
     }
-    return *it;
+    return static_cast<CAmount>(*it);
 }
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 0184448213..a3d3df26a3 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -146,7 +146,7 @@ UniValue getpeerinfo(const JSONRPCRequest& request)
         obj.push_back(Pair("timeoffset", stats.nTimeOffset));
         if (stats.dPingTime > 0.0)
             obj.push_back(Pair("pingtime", stats.dPingTime));
-        if (stats.dMinPing < std::numeric_limits<int64_t>::max()/1e6)
+        if (stats.dMinPing < static_cast<double>(std::numeric_limits<int64_t>::max())/1e6)
             obj.push_back(Pair("minping", stats.dMinPing));
         if (stats.dPingWait > 0.0)
             obj.push_back(Pair("pingwait", stats.dPingWait));
diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp
index 51b28d09fa..116210a297 100644
--- a/src/test/mempool_tests.cpp
+++ b/src/test/mempool_tests.cpp
@@ -559,15 +559,15 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
     // ... we should keep the same min fee until we get a block
     pool.removeForBlock(vtx, 1);
     SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE);
-    BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/2);
+    BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), llround((maxFeeRateRemoved.GetFeePerK() + 1000)/2.0));
     // ... then feerate should drop 1/2 each halflife
 
     SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2);
-    BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 5 / 2).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/4);
+    BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 5 / 2).GetFeePerK(), llround((maxFeeRateRemoved.GetFeePerK() + 1000)/4.0));
     // ... with a 1/2 halflife when mempool is < 1/2 its target size
 
     SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
-    BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/8);
+    BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), llround((maxFeeRateRemoved.GetFeePerK() + 1000)/8.0));
     // ... with a 1/4 halflife when mempool is < 1/4 its target size
 
     SetMockTime(42 + 7*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index f68d677646..776d3f36ca 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -981,7 +981,7 @@ const CTxMemPool::setEntries & CTxMemPool::GetMemPoolChildren(txiter entry) cons
 CFeeRate CTxMemPool::GetMinFee(size_t sizelimit) const {
     LOCK(cs);
     if (!blockSinceLastRollingFeeBump || rollingMinimumFeeRate == 0)
-        return CFeeRate(rollingMinimumFeeRate);
+        return CFeeRate(llround(rollingMinimumFeeRate));
 
     int64_t time = GetTime();
     if (time > lastRollingFeeUpdate + 10) {
@@ -999,7 +999,7 @@ CFeeRate CTxMemPool::GetMinFee(size_t sizelimit) const {
             return CFeeRate(0);
         }
     }
-    return std::max(CFeeRate(rollingMinimumFeeRate), incrementalRelayFee);
+    return std::max(CFeeRate(llround(rollingMinimumFeeRate)), incrementalRelayFee);
 }
 
 void CTxMemPool::trackPackageRemoved(const CFeeRate& rate) {
diff --git a/src/txmempool.h b/src/txmempool.h
index b07886579c..929d223588 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -507,7 +507,7 @@ public:
      * check does nothing.
      */
     void check(const CCoinsViewCache *pcoins) const;
-    void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = dFrequency * 4294967295.0; }
+    void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = static_cast<uint32_t>(dFrequency * 4294967295.0); }
 
     // addUnchecked must updated state for all ancestors of a given transaction,
     // to track size/count of descendant transactions.  First version of
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b774305a34..f14278af3d 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -409,11 +409,11 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
             {
                 int64_t nStartTime = GetTimeMillis();
                 crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod);
-                pMasterKey.second.nDeriveIterations = pMasterKey.second.nDeriveIterations * (100 / ((double)(GetTimeMillis() - nStartTime)));
+                pMasterKey.second.nDeriveIterations = static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * (100 / ((double)(GetTimeMillis() - nStartTime))));
 
                 nStartTime = GetTimeMillis();
                 crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod);
-                pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + pMasterKey.second.nDeriveIterations * 100 / ((double)(GetTimeMillis() - nStartTime))) / 2;
+                pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast<unsigned int>(pMasterKey.second.nDeriveIterations * 100 / ((double)(GetTimeMillis() - nStartTime)))) / 2;
 
                 if (pMasterKey.second.nDeriveIterations < 25000)
                     pMasterKey.second.nDeriveIterations = 25000;
@@ -615,11 +615,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
     CCrypter crypter;
     int64_t nStartTime = GetTimeMillis();
     crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod);
-    kMasterKey.nDeriveIterations = 2500000 / ((double)(GetTimeMillis() - nStartTime));
+    kMasterKey.nDeriveIterations = static_cast<unsigned int>(2500000 / ((double)(GetTimeMillis() - nStartTime)));
 
     nStartTime = GetTimeMillis();
     crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod);
-    kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + kMasterKey.nDeriveIterations * 100 / ((double)(GetTimeMillis() - nStartTime))) / 2;
+    kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast<unsigned int>(kMasterKey.nDeriveIterations * 100 / ((double)(GetTimeMillis() - nStartTime)))) / 2;
 
     if (kMasterKey.nDeriveIterations < 25000)
         kMasterKey.nDeriveIterations = 25000;