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

80dc829be7f8c3914074b85bb4c125baba18cb2c tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd51f3df4ee7f14ea05b8da229183923 tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676a1dcb84bcf95afd8d994831ab327b6 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 80dc829be7f8c3914074b85bb4c125baba18cb2c
  promag:
    Tested ACK 80dc829be7f8c3914074b85bb4c125baba18cb2c.
  lsilva01:
    tACK 80dc829
  meshcollider:
    utACK 80dc829be7f8c3914074b85bb4c125baba18cb2c

Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642

7 files changed, 57 insertions, 12 deletions

diff --git a/src/policy/feerate.cpp b/src/policy/feerate.cpp
index 25b9282b4e..ce149067b7 100644
--- a/src/policy/feerate.cpp
+++ b/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);
diff --git a/src/policy/feerate.h b/src/policy/feerate.h
index b16f3f8251..8ba896bb01 100644
--- a/src/policy/feerate.h
+++ b/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;
     /**
diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp
index 114fe3907c..aa23d71671 100644
--- a/src/test/amount_tests.cpp
+++ b/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));
 
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index c813fbea32..252a85c282 100644
--- a/src/test/transaction_tests.cpp
+++ b/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);
 
diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
index b0e46c6ca7..90d57c9c76 100755
--- a/test/functional/rpc_fundrawtransaction.py
+++ b/test/functional/rpc_fundrawtransaction.py
@@ -136,6 +136,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         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."""
@@ -1129,6 +1130,33 @@ class RawTransactionsTest(BitcoinTestFramework):
         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(address_type="bech32")
+        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(address_type="bech32"): 1 - 0.00000202}])
+        assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85})
+
 
 if __name__ == '__main__':
     RawTransactionsTest().main()
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 9f5bca6884..0678e2e6fe 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -36,12 +36,12 @@ def assert_approx(v, vexp, vspan=0.00001):
 
 def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
     """Assert the fee is in range."""
-    feerate_BTC_vB = feerate_BTC_kvB / 1000
-    target_fee = satoshi_round(tx_size * feerate_BTC_vB)
+    target_fee = get_fee(tx_size, feerate_BTC_kvB)
     if fee < target_fee:
         raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
     # allow the wallet's estimation to be at most 2 bytes off
-    if fee > (tx_size + 2) * feerate_BTC_vB:
+    high_fee = get_fee(tx_size + 2, feerate_BTC_kvB)
+    if fee > high_fee:
         raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
 
 
@@ -218,6 +218,18 @@ def str_to_b64str(string):
     return b64encode(string.encode('utf-8')).decode('ascii')
 
 
+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_btc_kvb):
+    """Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee"""
+    feerate_sat_kvb = int(feerate_btc_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 BTC result to nearest sat
+
+
 def satoshi_round(amount):
     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
 
diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py
index 79235646b0..5807a92b9d 100755
--- a/test/functional/wallet_keypool.py
+++ b/test/functional/wallet_keypool.py
@@ -193,7 +193,7 @@ class KeyPoolTest(BitcoinTestFramework):
         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.0008824})
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.0008823})
         assert_equal("psbt" in res, True)
         assert_equal(res["fee"], Decimal("0.00009706"))