From d3b8a54a81209420ef6447dd4581e1b6b8550647 Mon Sep 17 00:00:00 2001 From: Pol Espinasa Date: Sat, 17 May 2025 17:58:42 +0200 Subject: [PATCH] Refactor CFeeRate to use FeeFrac internally Co-authored-by: Abubakar Sadiq Ismail <48946461+ismaelsadeeq@users.noreply.github.com> --- src/policy/feerate.cpp | 35 +++++++---------- src/policy/feerate.h | 52 +++++++++++++++----------- src/test/amount_tests.cpp | 17 +++++---- src/test/fuzz/fee_rate.cpp | 4 +- src/test/fuzz/feefrac.cpp | 2 +- src/test/fuzz/fees.cpp | 2 +- src/validation.cpp | 6 +-- src/wallet/fees.cpp | 6 +-- src/wallet/fees.h | 2 +- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/fuzz/fees.cpp | 3 +- 11 files changed, 66 insertions(+), 65 deletions(-) diff --git a/src/policy/feerate.cpp b/src/policy/feerate.cpp index eb0cba5c67a..f74da8a7e22 100644 --- a/src/policy/feerate.cpp +++ b/src/policy/feerate.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -7,39 +7,30 @@ #include #include -#include -CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes) +CFeeRate::CFeeRate(const CAmount& nFeePaid, int32_t virtual_bytes) { - const int64_t nSize{num_bytes}; - - if (nSize > 0) { - nSatoshisPerK = nFeePaid * 1000 / nSize; + if (virtual_bytes > 0) { + m_feerate = FeePerVSize(nFeePaid, virtual_bytes); } else { - nSatoshisPerK = 0; + m_feerate = FeePerVSize(); } } -CAmount CFeeRate::GetFee(uint32_t num_bytes) const +CAmount CFeeRate::GetFee(int32_t virtual_bytes) const { - const int64_t nSize{num_bytes}; - - // 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); - if (nSatoshisPerK < 0) nFee = CAmount(-1); - } - + Assume(virtual_bytes >= 0); + if (m_feerate.IsEmpty()) { return CAmount(0);} + CAmount nFee = CAmount(m_feerate.EvaluateFeeUp(virtual_bytes)); + if (nFee == 0 && virtual_bytes != 0 && m_feerate.fee < 0) return CAmount(-1); return nFee; } std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const { + const CAmount feerate_per_kvb = GetFeePerK(); switch (fee_estimate_mode) { - case FeeEstimateMode::SAT_VB: return strprintf("%d.%03d %s/vB", nSatoshisPerK / 1000, nSatoshisPerK % 1000, CURRENCY_ATOM); - default: return strprintf("%d.%08d %s/kvB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT); + case FeeEstimateMode::SAT_VB: return strprintf("%d.%03d %s/vB", feerate_per_kvb / 1000, feerate_per_kvb % 1000, CURRENCY_ATOM); + default: return strprintf("%d.%08d %s/kvB", feerate_per_kvb / COIN, feerate_per_kvb % COIN, CURRENCY_UNIT); } } diff --git a/src/policy/feerate.h b/src/policy/feerate.h index d742a43acc7..69d33e3a296 100644 --- a/src/policy/feerate.h +++ b/src/policy/feerate.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -8,6 +8,7 @@ #include #include +#include #include @@ -27,52 +28,59 @@ enum class FeeEstimateMode { }; /** - * Fee rate in satoshis per kilovirtualbyte: CAmount / kvB + * Fee rate in satoshis per virtualbyte: CAmount / vB + * the feerate is represented internally as FeeFrac */ class CFeeRate { private: - /** Fee rate in sat/kvB (satoshis per 1000 virtualbytes) */ - CAmount nSatoshisPerK; + /** Fee rate in sats/vB (satoshis per N virtualbytes) */ + FeePerVSize m_feerate; public: - /** Fee rate of 0 satoshis per kvB */ - CFeeRate() : nSatoshisPerK(0) { } + /** Fee rate of 0 satoshis per 0 vB */ + CFeeRate() = default; template // Disallow silent float -> int conversion - explicit CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) { - } + explicit CFeeRate(const I m_feerate_kvb) : m_feerate(FeePerVSize(m_feerate_kvb, 1000)) {} /** * Construct a fee rate from a fee in satoshis and a vsize in vB. * * param@[in] nFeePaid The fee paid by a transaction, in satoshis - * param@[in] num_bytes The vsize of a transaction, in vbytes + * param@[in] virtual_bytes The vsize of a transaction, in vbytes + * + * Passing any virtual_bytes less than or equal to 0 will result in 0 fee rate per 0 size. */ - CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes); + CFeeRate(const CAmount& nFeePaid, int32_t virtual_bytes); /** * Return the fee in satoshis for the given vsize 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; + CAmount GetFee(int32_t virtual_bytes) const; /** * Return the fee in satoshis for a vsize of 1000 vbytes */ - CAmount GetFeePerK() const { return nSatoshisPerK; } - friend bool operator<(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK < b.nSatoshisPerK; } - friend bool operator>(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK > b.nSatoshisPerK; } - friend bool operator==(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK == b.nSatoshisPerK; } - friend bool operator<=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK <= b.nSatoshisPerK; } - friend bool operator>=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK >= b.nSatoshisPerK; } - friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; } - CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; } + CAmount GetFeePerK() const { return CAmount(m_feerate.EvaluateFeeDown(1000)); } + friend std::weak_ordering operator<=>(const CFeeRate& a, const CFeeRate& b) noexcept + { + return FeeRateCompare(a.m_feerate, b.m_feerate); + } + friend bool operator==(const CFeeRate& a, const CFeeRate& b) noexcept + { + return FeeRateCompare(a.m_feerate, b.m_feerate) == std::weak_ordering::equivalent; + } + CFeeRate& operator+=(const CFeeRate& a) { + m_feerate = FeePerVSize(GetFeePerK() + a.GetFeePerK(), 1000); + return *this; + } std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::BTC_KVB) const; - friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.nSatoshisPerK); } - friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.nSatoshisPerK); } + friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.m_feerate.fee, f.m_feerate.size); } + friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.m_feerate.fee, f.m_feerate.size); } - SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); } + SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.m_feerate.fee, obj.m_feerate.size); } }; #endif // BITCOIN_POLICY_FEERATE_H diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp index e5ab1cfb902..b500c9686d7 100644 --- a/src/test/amount_tests.cpp +++ b/src/test/amount_tests.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2016-2021 The Bitcoin Core developers +// Copyright (c) 2016-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -73,18 +73,21 @@ BOOST_AUTO_TEST_CASE(GetFeeTest) BOOST_CHECK(CFeeRate(CAmount(-1), 0) == CFeeRate(0)); BOOST_CHECK(CFeeRate(CAmount(0), 0) == CFeeRate(0)); BOOST_CHECK(CFeeRate(CAmount(1), 0) == CFeeRate(0)); + BOOST_CHECK(CFeeRate(CAmount(1), -1000) == CFeeRate(0)); // default value BOOST_CHECK(CFeeRate(CAmount(-1), 1000) == CFeeRate(-1)); BOOST_CHECK(CFeeRate(CAmount(0), 1000) == CFeeRate(0)); BOOST_CHECK(CFeeRate(CAmount(1), 1000) == CFeeRate(1)); - // lost precision (can only resolve satoshis per kB) - BOOST_CHECK(CFeeRate(CAmount(1), 1001) == CFeeRate(0)); - BOOST_CHECK(CFeeRate(CAmount(2), 1001) == CFeeRate(1)); + // Previously, precision was limited to three decimal digits + // due to only supporting satoshis per kB, so CFeeRate(CAmount(1), 1001) was equal to CFeeRate(0) + // Since #32750, higher precision is maintained. + BOOST_CHECK(CFeeRate(CAmount(1), 1001) > CFeeRate(0) && CFeeRate(CAmount(1), 1001) < CFeeRate(1)); + BOOST_CHECK(CFeeRate(CAmount(2), 1001) > CFeeRate(1) && CFeeRate(CAmount(2), 1001) < CFeeRate(2)); // some more integer checks - BOOST_CHECK(CFeeRate(CAmount(26), 789) == CFeeRate(32)); - BOOST_CHECK(CFeeRate(CAmount(27), 789) == CFeeRate(34)); + BOOST_CHECK(CFeeRate(CAmount(26), 789) > CFeeRate(32) && CFeeRate(CAmount(26), 789) < CFeeRate(33)); + BOOST_CHECK(CFeeRate(CAmount(27), 789) > CFeeRate(34) && CFeeRate(CAmount(27), 789) < CFeeRate(35)); // Maximum size in bytes, should not crash - CFeeRate(MAX_MONEY, std::numeric_limits::max()).GetFeePerK(); + CFeeRate(MAX_MONEY, std::numeric_limits::max()).GetFeePerK(); // check multiplication operator // check multiplying by zero diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp index 92616b62bea..d4d161776e5 100644 --- a/src/test/fuzz/fee_rate.cpp +++ b/src/test/fuzz/fee_rate.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2020-2021 The Bitcoin Core developers +// Copyright (c) 2020-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -20,7 +20,7 @@ FUZZ_TARGET(fee_rate) const CFeeRate fee_rate{satoshis_per_k}; (void)fee_rate.GetFeePerK(); - const auto bytes = fuzzed_data_provider.ConsumeIntegral(); + const auto bytes = fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()); if (!MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) { (void)fee_rate.GetFee(bytes); } diff --git a/src/test/fuzz/feefrac.cpp b/src/test/fuzz/feefrac.cpp index 2c4b34d7d4d..6bad3919164 100644 --- a/src/test/fuzz/feefrac.cpp +++ b/src/test/fuzz/feefrac.cpp @@ -223,7 +223,7 @@ FUZZ_TARGET(feefrac_mul_div) if (mul64 < std::numeric_limits::max() / 1000 && mul64 > std::numeric_limits::min() / 1000 && quot_abs < arith_uint256{std::numeric_limits::max() / 1000}) { - CFeeRate feerate(mul64, (uint32_t)div); + CFeeRate feerate(mul64, div); CAmount feerate_fee{feerate.GetFee(mul32)}; auto allowed_gap = static_cast(mul32 / 1000 + 3 + round_down); assert(feerate_fee - res_fee >= -allowed_gap); diff --git a/src/test/fuzz/fees.cpp b/src/test/fuzz/fees.cpp index 5c760be13d8..994df4ebf44 100644 --- a/src/test/fuzz/fees.cpp +++ b/src/test/fuzz/fees.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2020-2021 The Bitcoin Core developers +// Copyright (c) 2020-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/src/validation.cpp b/src/validation.cpp index 14fdd93c278..f2718161e26 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1396,7 +1396,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& auto iter = m_pool.GetIter(ws.m_ptx->GetHash()); Assume(iter.has_value()); const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : - CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; + CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : std::vector{ws.m_ptx->GetWitnessHash()}; results.emplace(ws.m_ptx->GetWitnessHash(), @@ -1460,7 +1460,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); - const CFeeRate effective_feerate{ws.m_modified_fees, static_cast(ws.m_vsize)}; + const CFeeRate effective_feerate{ws.m_modified_fees, static_cast(ws.m_vsize)}; // Tx was accepted, but not added if (args.m_test_accept) { return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, @@ -1612,7 +1612,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } if (args.m_test_accept) { const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : - CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; + CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : std::vector{ws.m_ptx->GetWitnessHash()}; results.emplace(ws.m_ptx->GetWitnessHash(), diff --git a/src/wallet/fees.cpp b/src/wallet/fees.cpp index eda6d319ec8..a786062a22f 100644 --- a/src/wallet/fees.cpp +++ b/src/wallet/fees.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -12,13 +12,13 @@ namespace wallet { CAmount GetRequiredFee(const CWallet& wallet, unsigned int nTxBytes) { - return GetRequiredFeeRate(wallet).GetFee(nTxBytes); + return GetRequiredFeeRate(wallet).GetFee(static_cast(nTxBytes)); } CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, FeeCalculation* feeCalc) { - return GetMinimumFeeRate(wallet, coin_control, feeCalc).GetFee(nTxBytes); + return GetMinimumFeeRate(wallet, coin_control, feeCalc).GetFee(static_cast(nTxBytes)); } CFeeRate GetRequiredFeeRate(const CWallet& wallet) diff --git a/src/wallet/fees.h b/src/wallet/fees.h index af7f759553b..b815d1c0004 100644 --- a/src/wallet/fees.h +++ b/src/wallet/fees.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2021 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 49241f33554..3f8a2b71b97 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -244,7 +244,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000); // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount - CAmount input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + CAmount input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*virtual_bytes=*/68); // bech32 input size (default test output type) add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); diff --git a/src/wallet/test/fuzz/fees.cpp b/src/wallet/test/fuzz/fees.cpp index a8da63b488f..80458ac89cd 100644 --- a/src/wallet/test/fuzz/fees.cpp +++ b/src/wallet/test/fuzz/fees.cpp @@ -45,8 +45,7 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup) } (void)GetDiscardRate(wallet); - const auto tx_bytes{fuzzed_data_provider.ConsumeIntegral()}; - + const auto tx_bytes{fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max())}; if (fuzzed_data_provider.ConsumeBool()) { wallet.m_pay_tx_fee = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; wallet.m_min_fee = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)};