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

Author: kwvg

src/policy/feerate.cpp

@@ -7,6 +7,8 @@
 #include <policy/feerate.h>
 #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

@@ -54,6 +54,7 @@ class CFeeRate
 
     /**
      * Return the fee in satoshis for the given size in vbytes.
+     * 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));
 

test/functional/rpc_fundrawtransaction.py

@@ -132,6 +132,7 @@ def run_test(self):
         self.test_include_unsafe()
         self.test_external_inputs()
         self.test_22670()
+        self.test_feerate_rounding()
 
     def test_change_position(self):
         """Ensure setting changePosition in fundraw with an exact match is handled properly."""
@@ -1080,5 +1081,34 @@ def do_fund_send(target):
         do_fund_send(lower_bound)
         do_fund_send(upper_bound)
 
+        self.restart_node(0, [])
+        self.connect_nodes(0, 1)
+        self.connect_nodes(0, 2)
+        self.connect_nodes(0, 3)
+
+    def test_feerate_rounding(self):
+        self.log.info("Test that rounding of GetFee does not result in an assertion")
+
+        self.nodes[1].createwallet("roundtest")
+        w = self.nodes[1].get_wallet_rpc("roundtest")
+
+        addr = w.getnewaddress()
+        self.nodes[0].sendtoaddress(addr, 1)
+        self.generate(self.nodes[0], 1)
+        self.sync_all()
+
+        # A P2WPKH input costs 68 vbytes; With a single P2WPKH output, the rest of the tx is 42 vbytes for a total of 110 vbytes.
+        # At a feerate of 1.85 sat/vb, the input will need a fee of 125.8 sats and the rest 77.7 sats
+        # The entire tx fee should be 203.5 sats.
+        # Coin selection rounds the fee individually instead of at the end (due to how CFeeRate::GetFee works).
+        # If rounding down (which is the incorrect behavior), then the calculated fee will be 125 + 77 = 202.
+        # If rounding up, then the calculated fee will be 126 + 78 = 204.
+        # In the former case, the calculated needed fee is higher than the actual fee being paid, so an assertion is reached
+        # To test this does not happen, we subtract 202 sats from the input value. If working correctly, this should
+        # fail with insufficient funds rather than bitcoind asserting.
+        rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(): 1 - 0.00000202}])
+        assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85})
+
+
 if __name__ == '__main__':
     RawTransactionsTest().main()

test/functional/test_framework/util.py

@@ -43,12 +43,12 @@ def assert_approx(v, vexp, vspan=0.00001):
 
 def assert_fee_amount(fee, tx_size, feerate_DASH_kvB):
     """Assert the fee is in range."""
-    feerate_DASH_vB = feerate_DASH_kvB / 1000
-    target_fee = satoshi_round(tx_size * feerate_DASH_vB)
+    target_fee = get_fee(tx_size, feerate_DASH_kvB)
     if fee < target_fee:
         raise AssertionError("Fee of %s DASH too low! (Should be %s DASH)" % (str(fee), str(target_fee)))
     # allow the wallet's estimation to be at most 2 bytes off
-    if fee > (tx_size + 2) * feerate_DASH_vB:
+    high_fee = get_fee(tx_size + 2, feerate_DASH_kvB)
+    if fee > high_fee:
         raise AssertionError("Fee of %s DASH too high! (Should be %s DASH)" % (str(fee), str(target_fee)))
 
 
@@ -242,6 +242,18 @@ def random_bitflip(data):
     return bytes(data)
 
 
+def ceildiv(a, b):
+    """Divide 2 ints and round up to next int rather than round down"""
+    return -(-a // b)
+
+
+def get_fee(tx_size, feerate_dash_kvb):
+    """Calculate the fee in DASH given a feerate is DASH/kvB. Reflects CFeeRate::GetFee"""
+    feerate_sat_kvb = int(feerate_dash_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors
+    target_fee_sat = ceildiv(feerate_sat_kvb * tx_size, 1000) # Round calculated fee up to nearest sat
+    return satoshi_round(target_fee_sat / Decimal(1e8)) # Truncate DASH result to nearest sat
+
+
 def satoshi_round(amount):
     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
 

test/functional/wallet_keypool_hd.py

@@ -215,7 +215,7 @@ def run_test(self):
         assert_equal("psbt" in res, True)
 
         # create a transaction without change at the maximum fee rate, such that the output is still spendable:
-        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00008824})
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00008822})
         assert_equal("psbt" in res, True)
         assert_equal(res["fee"], Decimal("0.00001694"))