From bd7e08e36bf2e1238ddf8cc01433f8db82f848c9 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 10 Sep 2021 20:24:44 -0400 Subject: 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. Github-Pull: #22949 Rebased-From: 0fbaef9676a1dcb84bcf95afd8d994831ab327b6 --- src/policy/feerate.cpp | 6 +++++- src/policy/feerate.h | 1 + src/test/amount_tests.cpp | 10 +++++----- src/test/transaction_tests.cpp | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-) (limited to 'src') 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 +#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 d296d32774..57bb5d02c0 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 77b7758a17..12193ccb59 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 40c53cb2ec..17bd561807 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -807,12 +807,12 @@ 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; reason.clear(); BOOST_CHECK(!IsStandardTx(CTransaction(t), reason)); BOOST_CHECK_EQUAL(reason, "dust"); // not dust: - t.vout[0].nValue = 673; + t.vout[0].nValue = 674; BOOST_CHECK(IsStandardTx(CTransaction(t), reason)); dustRelayFee = CFeeRate(DUST_RELAY_TX_FEE); -- cgit v1.2.3