From 0fbaef9676a1dcb84bcf95afd8d994831ab327b6 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 10 Sep 2021 20:24:44 -0400 Subject: [PATCH 1/3] fees: Always round up fee calculated from a feerate When calculating the fee for a given tx size from a fee rate, we should always round up to the next satoshi. Otherwise, if we round down (via truncation), the calculated fee may result in a fee with a feerate slightly less than targeted. This is particularly important for coin selection as a slightly lower feerate than expected can result in a variety of issues. --- src/policy/feerate.cpp | 6 +++++- src/policy/feerate.h | 1 + src/test/amount_tests.cpp | 10 +++++----- src/test/transaction_tests.cpp | 4 ++-- test/functional/wallet_keypool.py | 2 +- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/policy/feerate.cpp b/src/policy/feerate.cpp index 25b9282b4ed..ce149067b76 100644 --- a/src/policy/feerate.cpp +++ b/src/policy/feerate.cpp @@ -7,6 +7,8 @@ #include +#include + 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(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 b16f3f82515..8ba896bb01f 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 114fe3907c6..aa23d716710 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 c813fbea321..252a85c2822 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/wallet_keypool.py b/test/functional/wallet_keypool.py index c7149932342..ada97417e4c 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -179,7 +179,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")) From ce2cc44afd51f3df4ee7f14ea05b8da229183923 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 10 Sep 2021 20:27:41 -0400 Subject: [PATCH 2/3] tests: Test for assertion when feerate is rounded down When calculating a txs absolute fee, if the fee is rounded down to the nearest satoshi, it is possible for the coin selection algorithms to undercalculate the fee needed. This can lead to an assertion error in some situations. One such scenario is added to rpc_fundrawtransaction.py. --- test/functional/rpc_fundrawtransaction.py | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index b0e46c6ca70..90d57c9c76a 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() From 80dc829be7f8c3914074b85bb4c125baba18cb2c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 8 Oct 2021 13:57:48 -0400 Subject: [PATCH 3/3] tests: Calculate fees more similarly to CFeeRate::GetFee Because of floating point precision issues, not all of the rounding done is always correct. To fix this, the fee calculation for assert_fee_amount is changed to better reflect how CFeeRate::GetFee does it. First the feerate is converted to an int representing sat/kvb. Then this is multiplied by the transaction size, divivided by 1000, and rounded up to the nearest sat. The result is then converted back to BTC (divided by 1e8) and then rounded down to the nearest sat to avoid precision errors. --- test/functional/test_framework/util.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 9f5bca6884d..0678e2e6fe2 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)