refactor: CFeeRate encapsulates FeeFrac internally

polespinasa commented at 10:17 am on June 15, 2025: contributor

The FeeFrac type represents a fraction, intended to be used for sats/vbyte or sats/WU. It was added to improve accuracy when evaluating fee rates in cluster mempool. [1] But it can also be used to fix the precision issues that the current CFeeRate class has now.

At the moment, CFeeRate handles the fee rate as satoshis per kilovirtualbyte: CAmount / kvB using an integer. This PR fix CFeeRate precision issues by encapsulating FeeFrac internally keeping backwards compatibility.

This PR can also be used as a based to use multiple units on RPC calls as detailed in this issue [2].

Some previous discussions: [1] #30535 [2] https://github.com/bitcoin/bitcoin/issues/32093

DrahtBot commented at 10:17 am on June 15, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32750.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

param@[in] -> @param[in] [standard Doxygen tag syntax]

drahtbot_id_4_m

polespinasa renamed this:
~~Refactor CFeeRate to use FeeFrac internally~~
refactor: CFeeRate encapsulates FeeFrac internally
on Jun 15, 2025

DrahtBot added the label Refactoring on Jun 15, 2025

DrahtBot added the label CI failed on Jun 15, 2025

DrahtBot commented at 11:20 am on June 15, 2025: contributor

🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer, no depends: https://github.com/bitcoin/bitcoin/runs/44121425336 LLM reason (✨ experimental): The CI failure is caused by an implicit integer conversion that triggered an UndefinedBehaviorSanitizer error during fuzzing.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

ismaelsadeeq commented at 1:47 pm on June 15, 2025: member

Concept ACK I’ve taken a look into this previously in #30535#pullrequestreview-2227244550

ismaelsadeeq commented at 2:03 pm on June 15, 2025: member

I think you can fix the fuzz crash with

 0diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
 1index 92616b62bea..f69cb59ab7b 100644
 2--- a/src/test/fuzz/fee_rate.cpp
 3+++ b/src/test/fuzz/fee_rate.cpp
 4@@ -20,7 +20,7 @@ FUZZ_TARGET(fee_rate)
 5     const CFeeRate fee_rate{satoshis_per_k};
 6 
 7     (void)fee_rate.GetFeePerK();
 8-    const auto bytes = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
 9+    const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
10     if (!MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) {
11         (void)fee_rate.GetFee(bytes);
12     }

Also the // Compare approximately with CFeeRate test in src/test/fuzz/fee_frac.cpp is now not required.

polespinasa force-pushed on Jun 16, 2025

in src/policy/feerate.h:38 in 417bd07ed5 outdated

36 {
37 private:
38-    /** Fee rate in sat/kvB (satoshis per 1000 virtualbytes) */
39-    CAmount nSatoshisPerK;
40+    /** Fee rate in sats/vB (satoshis per N virtualbytes) */
41+    FeePerVSize nSatoshisPerV;

sipa commented at 8:05 pm on June 16, 2025:

Coding style nit: use m_satoshis_per_vb or m_feerate or so? (see doc/developer-notes.md, don’t try to mimic existing style when rewriting code).

polespinasa commented at 7:49 am on June 17, 2025:

Done 👍

in src/policy/feerate.cpp:24 in 417bd07ed5 outdated

35-
36-    if (nFee == 0 && nSize != 0) {
37-        if (nSatoshisPerK > 0) nFee = CAmount(1);
38-        if (nSatoshisPerK < 0) nFee = CAmount(-1);
39+    if (nSatoshisPerV.IsEmpty()) { return CAmount(0);}
40+    if (num_bytes < 0 || nSatoshisPerV.size <= 0) { return CAmount(-1);}

sipa commented at 8:07 pm on June 16, 2025:

nSatoshisPerV.size <= 0 is not possible, it can only be 0, but that is caught by the line above.

polespinasa commented at 7:49 am on June 17, 2025:

Done 👍

in src/policy/feerate.cpp:27 in 417bd07ed5 outdated

38-        if (nSatoshisPerK < 0) nFee = CAmount(-1);
39+    if (nSatoshisPerV.IsEmpty()) { return CAmount(0);}
40+    if (num_bytes < 0 || nSatoshisPerV.size <= 0) { return CAmount(-1);}
41+    CAmount nFee = CAmount(nSatoshisPerV.EvaluateFeeUp(num_bytes));
42+    if (nFee == 0 && num_bytes != 0) {
43+        if (nSatoshisPerV.EvaluateFeeUp(nSatoshisPerV.size) > 0) return CAmount(1);

sipa commented at 8:09 pm on June 16, 2025:

This line is equivalent to if (nSatoshisPerV.fee > 0) return CAmount(1);, and that cannot happen, because if it was the case, then nFee would have been strictly positive already.

polespinasa commented at 7:50 am on June 17, 2025:

You’re right, good catch, thx! Done 👍

in src/policy/feerate.cpp:28 in 417bd07ed5 outdated

39+    if (nSatoshisPerV.IsEmpty()) { return CAmount(0);}
40+    if (num_bytes < 0 || nSatoshisPerV.size <= 0) { return CAmount(-1);}
41+    CAmount nFee = CAmount(nSatoshisPerV.EvaluateFeeUp(num_bytes));
42+    if (nFee == 0 && num_bytes != 0) {
43+        if (nSatoshisPerV.EvaluateFeeUp(nSatoshisPerV.size) > 0) return CAmount(1);
44+        if (nSatoshisPerV.EvaluateFeeUp(nSatoshisPerV.size) < 0) return CAmount(-1);

sipa commented at 8:10 pm on June 16, 2025:

This line is equivalent to if (nSatoshisPerV.fee < 0) return CAmount(-1);

polespinasa commented at 7:50 am on June 17, 2025:

Done 👍

in src/policy/feerate.h:65 in 417bd07ed5 outdated

77-    friend bool operator<=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK <= b.nSatoshisPerK; }
78-    friend bool operator>=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK >= b.nSatoshisPerK; }
79-    friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; }
80-    CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; }
81+    CAmount GetFeePerK() const { return CAmount(nSatoshisPerV.EvaluateFeeDown(1000)); }
82+    friend bool operator<(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerV << b.nSatoshisPerV; }

sipa commented at 8:18 pm on June 16, 2025:

Since C++20, all of these comparison functions can be replaced with just:

0friend std::weak_ordering operator<=>(const CFeeRate& a, const CFeeRate& b) noexcept
1{
2    return FeeRateCompare(a.nSatoshisPerV, b.nSatoshisPerV);
3}
4friend bool operator==(const CFeeRate& a, const CFeeRate& b) noexcept
5{
6    return FeeRateCompare(a.nSatoshisPerV, b.nSatoshisPerV) == std::weak_ordering::equivalent;
7}

The 4 others will be automatically synthesized from these.

polespinasa commented at 7:51 am on June 17, 2025:

Didn’t know about that, thanks!! Done 👍

DrahtBot removed the label CI failed on Jun 16, 2025

in src/policy/feerate.cpp:35 in 417bd07ed5 outdated

47     return nFee;
48 }
49 
50 std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
51 {
52+    const CAmount rate_per_kvb = nSatoshisPerV.fee * 1000 / nSatoshisPerV.size;

ajtowns commented at 5:24 am on June 17, 2025:

rate_per_kvb = GetFeePerK(); ?

polespinasa commented at 7:52 am on June 17, 2025:

Nice catch! Done 👍

Refactor CFeeRate to use FeeFrac internally

Co-authored-by: Abubakar Sadiq Ismail <48946461+ismaelsadeeq@users.noreply.github.com>

ccb53ed059

polespinasa force-pushed on Jun 17, 2025

refactor: CFeeRate encapsulates FeeFrac internally #32750

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)