Merge bitcoin/bitcoin#22949: fee: Round up fee calculation to avoid a lower than expected feerate

80dc829be7 tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676 fees: Always round up fee calculated from a feerate (Andrew Chow)

Pull request description:

  When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.

  A test is added for the assertion, along with a comment explaining what happens.

  It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.

ACKs for top commit:
  ryanofsky:
    Code review ACK 80dc829be7
  promag:
    Tested ACK 80dc829be7.
  lsilva01:
    tACK 80dc829
  meshcollider:
    utACK 80dc829be7

Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642

src/policy/feerate.cpp

@@ -7,6 +7,8 @@
 
 #include <tinyformat.h>
 
+#include <cmath>
+
 CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)
 {
     const int64_t nSize{num_bytes};
@@ -22,7 +24,9 @@ CAmount CFeeRate::GetFee(uint32_t num_bytes) const
 {
     const int64_t nSize{num_bytes};
 
-    CAmount nFee = nSatoshisPerK * nSize / 1000;
+    // Be explicit that we're converting from a double to int64_t (CAmount) here.
+    // We've previously had issues with the silent double->int64_t conversion.
+    CAmount nFee{static_cast<CAmount>(std::ceil(nSatoshisPerK * nSize / 1000.0))};
 
     if (nFee == 0 && nSize != 0) {
         if (nSatoshisPerK > 0) nFee = CAmount(1);

src/policy/feerate.h

@@ -48,6 +48,7 @@ public:
     CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes);
     /**
      * Return the fee in satoshis for the given size in bytes.
+     * If the calculated fee would have fractional satoshis, then the returned fee will always be rounded up to the nearest satoshi.
      */
     CAmount GetFee(uint32_t num_bytes) const;
     /**

src/test/amount_tests.cpp

@@ -48,13 +48,13 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
     BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), CAmount(-9e3));
 
     feeRate = CFeeRate(123);
-    // Truncates the result, if not integer
+    // Rounds up the result, if not integer
     BOOST_CHECK_EQUAL(feeRate.GetFee(0), CAmount(0));
     BOOST_CHECK_EQUAL(feeRate.GetFee(8), CAmount(1)); // Special case: returns 1 instead of 0
-    BOOST_CHECK_EQUAL(feeRate.GetFee(9), CAmount(1));
-    BOOST_CHECK_EQUAL(feeRate.GetFee(121), CAmount(14));
-    BOOST_CHECK_EQUAL(feeRate.GetFee(122), CAmount(15));
-    BOOST_CHECK_EQUAL(feeRate.GetFee(999), CAmount(122));
+    BOOST_CHECK_EQUAL(feeRate.GetFee(9), CAmount(2));
+    BOOST_CHECK_EQUAL(feeRate.GetFee(121), CAmount(15));
+    BOOST_CHECK_EQUAL(feeRate.GetFee(122), CAmount(16));
+    BOOST_CHECK_EQUAL(feeRate.GetFee(999), CAmount(123));
     BOOST_CHECK_EQUAL(feeRate.GetFee(1e3), CAmount(123));
     BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), CAmount(1107));
 

src/test/transaction_tests.cpp

@@ -810,10 +810,10 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
     // nDustThreshold = 182 * 3702 / 1000
     dustRelayFee = CFeeRate(3702);
     // dust:
-    t.vout[0].nValue = 673 - 1;
+    t.vout[0].nValue = 674 - 1;
     CheckIsNotStandard(t, "dust");
     // not dust:
-    t.vout[0].nValue = 673;
+    t.vout[0].nValue = 674;
     CheckIsStandard(t);
     dustRelayFee = CFeeRate(DUST_RELAY_TX_FEE);