refactor: CFeeRate encapsulates FeeFrac internally #32750

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
ACK	murchandamus, ismaelsadeeq, theStack, achow101
Concept ACK	Eunovo

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

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

param@[in] -> @param[in] [correct Doxygen tag syntax]
param@[in] -> @param[in] [correct 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.

<details><summary>Hints</summary>

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.

</details>

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

diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
index 92616b62bea..f69cb59ab7b 100644
--- a/src/test/fuzz/fee_rate.cpp
+++ b/src/test/fuzz/fee_rate.cpp
@@ -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<uint32_t>();
+    const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
     if (!MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) {
         (void)fee_rate.GetFee(bytes);
     }

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:

friend std::weak_ordering operator<=>(const CFeeRate& a, const CFeeRate& b) noexcept
{
    return FeeRateCompare(a.nSatoshisPerV, b.nSatoshisPerV);
}
friend bool operator==(const CFeeRate& a, const CFeeRate& b) noexcept
{
    return FeeRateCompare(a.nSatoshisPerV, b.nSatoshisPerV) == std::weak_ordering::equivalent;
}

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 👍

polespinasa force-pushed on Jun 17, 2025

DrahtBot added the label Needs rebase on Jun 19, 2025

in src/policy/feerate.h:66 in ccb53ed059 outdated

  76 | -    friend bool operator==(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK == b.nSatoshisPerK; }
  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(m_feerate.EvaluateFeeDown(1000)); }

Eunovo commented at 1:12 PM on June 20, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/ccb53ed05921a77fada46e22cf2e7410ef7f338c: GetFee() rounds up but GetFeePerK() rounds down, which means that the feerate returned will be less than the feerate actually used for calculations.

Why not round up for GetFeePerK()?

ismaelsadeeq commented at 2:32 PM on June 20, 2025:

I dont think thats correct, It actually round down implicitly when initializing nSatoshisPerK see the the constructor.

It round up in GetFee

Eunovo commented at 6:32 PM on June 20, 2025:

@ismaelsadeeq https://github.com/bitcoin/bitcoin/blob/master/src/policy/feerate.cpp#L12-L21 Have I missed something?

ismaelsadeeq commented at 6:54 PM on June 20, 2025:

Exactly what you quoted, see my previous review here https://github.com/polespinasa/bitcoin/pull/2#discussion_r2143529762

If you round up you will see change in behavior and test failures.

Eunovo commented at 7:27 PM on June 20, 2025:

I see. Thanks @ismaelsadeeq

in src/policy/feerate.h:76 in ccb53ed059 outdated

  86 | +    friend bool operator==(const CFeeRate& a, const CFeeRate& b) noexcept
  87 | +    {
  88 | +        return FeeRateCompare(a.m_feerate, b.m_feerate) == std::weak_ordering::equivalent;
  89 | +    }
  90 | +    CFeeRate& operator+=(const CFeeRate& a) {
  91 | +        m_feerate = FeePerVSize(GetFeePerK() + a.GetFeePerK(), 1000);

Eunovo commented at 1:29 PM on June 20, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/ccb53ed05921a77fada46e22cf2e7410ef7f338c: Why not use fractional addition?

polespinasa commented at 11:23 AM on June 23, 2025:

More or less that's what this is doing but relying on feefrac code (more clean). The two fractions are converted to denominator 1000. And then the numerators are added, the result of that sum is returned with denominator 1000.

Doesn't matter what units are we using because the units are specified when calling GetFee or GetFeePerK.

in src/policy/feerate.cpp:23 in ccb53ed059 outdated

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

Eunovo commented at 1:42 PM on June 20, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/ccb53ed05921a77fada46e22cf2e7410ef7f338c: IIUC previously, zero Sats was only returned when num_bytes == 0. Looks like you need to do m_feerate.IsEmpty() && num_bytes == 0.

polespinasa commented at 11:39 AM on June 23, 2025:

-CAmount CFeeRate::GetFee(uint32_t num_bytes) const
+CAmount CFeeRate::GetFee(int32_t num_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<CAmount>(std::ceil(nSatoshisPerK * nSize / 1000.0))};
-
-    if (nFee == 0 && nSize != 0) {
-        if (nSatoshisPerK > 0) nFee = CAmount(1);
-        if (nSatoshisPerK < 0) nFee = CAmount(-1);
-    }
-
+    if (m_feerate.IsEmpty()) { return CAmount(0);}
+    if (num_bytes < 0) { return CAmount(-1);}
+    CAmount nFee = CAmount(m_feerate.EvaluateFeeUp(num_bytes));
+    if (nFee == 0 && num_bytes != 0 && m_feerate.fee < 0) return CAmount(-1);
     return nFee;
 }

Not necessarily. For example, if num_bytes=10 and nSatoshisPerK=0 n_Fee will be 0. And the function will return 0. Because nSatoshisPerK=0 will not catch any of the if statements. That's the case m_feerate.IsEmpty() is evaluating.

If num_bytes=0 then nFee will also be 0. See: (at_size is num_bytes) https://github.com/bitcoin/bitcoin/blob/c5849663baa925a5291731e4e4013a08c811c646/src/util/feefrac.h#L202-L217

The next if is the same as it was before but the option to return 1 was deleted as that cannot happend, see: #32750 (review)

Eunovo commented at 2:13 PM on June 23, 2025:

@polespinasa You're right, zero is always returned when nSatoshisPerK is zero. You can mark this as resolved

in src/policy/feerate.cpp:23 in ccb53ed059 outdated

  37 | -        if (nSatoshisPerK > 0) nFee = CAmount(1);
  38 | -        if (nSatoshisPerK < 0) nFee = CAmount(-1);
  39 | -    }
  40 | -
  41 | +    if (m_feerate.IsEmpty()) { return CAmount(0);}
  42 | +    if (num_bytes < 0) { return CAmount(-1);}

Eunovo commented at 1:45 PM on June 20, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/ccb53ed05921a77fada46e22cf2e7410ef7f338c: num_bytes refers to the size of the transaction. It should not be negative. We should be doing assert(num_bytes >= 0) instead.

Eunovo commented at 11:55 AM on June 26, 2025:

@polespinasa now that the code has been updated to use uint32, I think you can mark this as resolved

Eunovo commented at 1:47 PM on June 20, 2025: contributor

Concept ACK

Left some comments

polespinasa force-pushed on Jun 23, 2025

DrahtBot removed the label Needs rebase on Jun 23, 2025

polespinasa commented at 2:03 PM on June 23, 2025: contributor

13efb13bbe0e904babd6f7646dda1885e80b8226 Rebased on top of Master (c5849663baa925a5291731e4e4013a08c811c646)

theStack commented at 2:26 AM on June 24, 2025: contributor

Concept ACK

in src/policy/feerate.cpp:10 in 13efb13bbe outdated

   8 | @@ -9,37 +9,29 @@
   9 |  
  10 |  #include <cmath>

davidgumberg commented at 12:55 AM on June 25, 2025:

can drop this since the std::ceil call is dropped.

polespinasa commented at 5:32 AM on June 25, 2025:

done

in src/policy/feerate.h:44 in 13efb13bbe outdated

  46 | +    /** Fee rate of 0 satoshis per vB */
  47 | +    CFeeRate() = default;
  48 |      template<std::integral I> // Disallow silent float -> int conversion
  49 | -    explicit CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
  50 | -    }
  51 | +    explicit CFeeRate(const I _m_feerate) : m_feerate(FeePerVSize(_m_feerate, 1000)) {}

davidgumberg commented at 1:10 AM on June 25, 2025:

nit: the argument name should probably be m_feerate_kvb

polespinasa commented at 5:32 AM on June 25, 2025:

done

in src/policy/feerate.cpp:12 in 13efb13bbe outdated

   8 | @@ -9,37 +9,29 @@
   9 |  
  10 |  #include <cmath>
  11 |  
  12 | -CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)

davidgumberg commented at 1:12 AM on June 25, 2025:

The reason that the fuzz tests fail before changing this to uint32_t (e.g. this commit: https://github.com/davidgumberg/bitcoin/commit/01a4557b10b31e620716cff04c554e6f1a95653a) is because CFeeRate::GetFee takes an unsigned int, but then passes it to EvaluateFeeUp() which implicitly converts at_size to a signed value, and large examples of uint32_t values become negative integers, this results in EvaluateFee() getting passed a negative number and asserting.

Wouldn't it be better to change the FeeFrac::EvaluateFee*() functions to take unsigned integers, e.g.:

diff --git a/src/util/feefrac.h b/src/util/feefrac.h
index 7577107e8c..9d034c6791 100644
--- a/src/util/feefrac.h
+++ b/src/util/feefrac.h
@@ -199,10 +199,9 @@ struct FeeFrac
      * is guaranteed to be the case when 0 <= at_size <= this->size.
      */
     template<bool RoundDown>
-    int64_t EvaluateFee(int32_t at_size) const noexcept
+    int64_t EvaluateFee(uint32_t at_size) const noexcept
     {
         Assume(size > 0);
-        Assume(at_size >= 0);
         if (fee >= 0 && fee < 0x200000000) [[likely]] {
             // Common case where (this->fee * at_size) is guaranteed to fit in a uint64_t.
             if constexpr (RoundDown) {
@@ -218,9 +217,9 @@ struct FeeFrac

 public:
     /** Compute the fee for a given size `at_size` using this object's feerate, rounding down. */
-    int64_t EvaluateFeeDown(int32_t at_size) const noexcept { return EvaluateFee<true>(at_size); }
+    int64_t EvaluateFeeDown(uint32_t at_size) const noexcept { return EvaluateFee<true>(at_size); }
     /** Compute the fee for a given size `at_size` using this object's feerate, rounding up. */
-    int64_t EvaluateFeeUp(int32_t at_size) const noexcept { return EvaluateFee<false>(at_size); }
+    int64_t EvaluateFeeUp(uint32_t at_size) const noexcept { return EvaluateFee<false>(at_size); }
 };

 /** Compare the feerate diagrams implied by the provided sorted chunks data.

Tested as follows:

cmake --preset libfuzzer && cmake --build build_fuzz -j $(nproc)
FUZZ=fee_rate_fuzz_target

polespinasa commented at 6:19 AM on June 25, 2025:

That would make sense. Implemented on a separated commit bf63317ae7395b0509f86e2c188b1c6cacf995de. @sipa, I would like to know your opinion on this. Is there any reason you used int32 for EvaluateFee in the first place? The two Assume at the beginning of the function cut the function to only positive numbers.

polespinasa commented at 2:43 PM on June 26, 2025:

@davidgumberg I'm having doubts about this. FeeFrac takes an int32 for size attribute. I'm not sure it is a good idea to mix types between functions.

Also Mul(fee, at_size) is taking a int32.

polespinasa force-pushed on Jun 25, 2025

polespinasa commented at 6:14 AM on June 25, 2025: contributor

db63d5bf81895d78d39fe8cf94ded6f63d81d614 removes an unused include and some nit on arg names. 990010f49ca97479f9fe46e288ed0f0fbb0fde94 modifies the code to use uint instead of ints see #32750 (review) for context

in src/policy/feerate.cpp:24 in 990010f49c outdated

  23 | +    const int64_t nSize{num_bytes};
  24 |      if (m_feerate.IsEmpty()) { return CAmount(0);}
  25 | -    if (num_bytes < 0) { return CAmount(-1);}
  26 | -    CAmount nFee = CAmount(m_feerate.EvaluateFeeUp(num_bytes));
  27 | -    if (nFee == 0 && num_bytes != 0 && m_feerate.fee < 0) return CAmount(-1);
  28 | +    if (nSize < 0) { return CAmount(-1);}

Eunovo commented at 11:59 AM on June 26, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/990010f49ca97479f9fe46e288ed0f0fbb0fde94: Is it still possible for nSize to be less than zero? Converting from uint32 to int64 should be safe

polespinasa commented at 3:14 PM on June 26, 2025:

Actually I think nSize is not needed at all and the check can be deleted. If EvaluateFeeUp is taking an uint32 there's no need to convert num_bytes to other types!!

in src/util/feefrac.h:206 in 990010f49c outdated

 203 | +    int64_t EvaluateFee(uint32_t at_size) const noexcept
 204 |      {
 205 |          Assume(size > 0);
 206 | -        Assume(at_size >= 0);
 207 | +        //Assume(at_size >= 0);
 208 |          if (fee >= 0 && fee < 0x200000000) [[likely]] {

sipa commented at 2:45 PM on June 26, 2025:

If you change at_size to be uint32, I don't think this check is still correct.

polespinasa commented at 3:16 PM on June 26, 2025:

You mean the commented Assume can be deleted?

sipa commented at 3:20 PM on June 26, 2025:

No, the if condition is wrong now. The result can overflow.

polespinasa commented at 3:39 PM on June 26, 2025:

Oh you're right! ~I think adding an Assume(at_size <= INT32_MAX) should be enough?~ Self-answer: Nope, it breaks fuzz test by trying to use numbers bigger than INT32_MAX.

sipa commented at 6:09 PM on June 26, 2025:

Well if you're going to assume at_size <= INT32_MAX, why bother changing the type?

polespinasa commented at 6:12 PM on June 26, 2025:

Yep, I think I will not :) #32750 (comment):

In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

polespinasa force-pushed on Jun 26, 2025

polespinasa commented at 4:43 PM on June 26, 2025: contributor

I think I'll drop bf63317ae7395b0509f86e2c188b1c6cacf995de and revert to only db63d5bf81 (unless someone sees a blocking reason and the need to use uint32 instead of int32).

Changing to uint32 needs many changes in FeeFrac (which I would like to avoid touching). In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

Friendly ping @davidgumberg @Eunovo @sipa to know your opinion :)

DrahtBot added the label CI failed on Jun 26, 2025

DrahtBot commented at 7:04 PM on June 26, 2025: contributor

🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer, no depends: https://github.com/bitcoin/bitcoin/runs/44861911741 LLM reason (✨ experimental): The failure is caused by an assertion failure in the evaluateFee function due to at_size exceeding INT32_MAX during fuzz testing.

<details><summary>Hints</summary>

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.

</details>

davidgumberg commented at 9:33 PM on June 26, 2025: contributor

Changing to uint32 needs many changes in FeeFrac (which I would like to avoid touching). In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

Yes, but leaving size as signed in FeeFrac means that this PR has to make changes to CFeeRate to make its sizes signed. FeeFrac and CFeeRate should both be signed or unsigned, I don't have any context on if/why someone might want signed sizes, but all the FeeFrac functions works with signed sizes, and it seems like it could be useful in some situations so changing CFeeRate to use signed sizes (as the PR was originally structured) seems fair enough.

If you do that, I think it would be good to add some checks for negative sizes passed to CFeeRate::CFeeRate() and CFeeRate::GetFee() in src/test/amount_tests.cpp.

Eunovo commented at 3:54 AM on June 27, 2025: contributor

Changing to uint32 needs many changes in FeeFrac (which I would like to avoid touching). In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

It's difficult for me to give an opinion here because I'm not sure why FeeFrac was designed with signed sizes in the first place. I see that FeeFrac::EvaluateFee does have an Assume(at_size >= 0) check, I think you should rely on this check when you revert instead of returning -1 (as was done in https://github.com/bitcoin/bitcoin/commit/ccb53ed05921a77fada46e22cf2e7410ef7f338c#diff-cc74b9dc8da4346ef6f877eab0aafa44feeea22d672a6516fe2d4f2fd65a74ccR24) before calling FeeFrac::EvaluateFeeUp. Returning early bypasses the assertion.

sipa commented at 2:19 PM on June 27, 2025: member

The reason why FeeFrac supports negative fees and negative sizes is because that allows it to be used on "set differences" (*), which was at some point expected to be used inside the cluster linearization algorithm. It isn't in the current version in master, however, but it ended up being very useful in PR #32545 (see this line which relies on it).

(*) Imagine you have a set of transactions, whose total fee/size (sum of fees divided by sum of sizes) is a FeeFrac c, and you have two potential subsets of transactions (both subsets of c) that can be moved to the front of the linearization, t1 and t2 (both FeeFrac objects, containing their sum of fees and sum of sizes). To determine whether t1 is better than t2, it turns out that (t1 - t2) >> c is the correct expression (i.e., (t1.fee - t2.fee) * c.size > c.fee * (t1.size - t2.size)), which works even when t2 has a larger size than t1 (and thus the difference t1 - t2 between them would have negative size).

This doesn't apply to the at_size argument for EvaluateFeeUp and EvaluateFeeDown however, which I can't imagine ever needing to be negative.

polespinasa force-pushed on Jun 28, 2025

polespinasa commented at 8:47 AM on June 28, 2025: contributor

db63d5bf81 reverts to previous status (do not review). Will apply the commented changes in the next push.

DrahtBot removed the label CI failed on Jun 28, 2025

polespinasa force-pushed on Jun 28, 2025

in src/wallet/test/fuzz/fees.cpp:49 in 0aaeba9fe7 outdated

  44 | @@ -45,8 +45,8 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup)
  45 |      }
  46 |      (void)GetDiscardRate(wallet);
  47 |  
  48 | -    const auto tx_bytes{fuzzed_data_provider.ConsumeIntegral<unsigned int>()};
  49 | -
  50 | +    const auto tx_bytes{fuzzed_data_provider.ConsumeIntegral<int32_t>()};
  51 | +    if (tx_bytes < 0) return;

polespinasa commented at 11:43 AM on June 28, 2025:

Not sure if there's a cleaner way to handle this. Without it negative values can be provided to GetFee() hitting the Assume(num_bytes >= 0) statement and failing the code.

Eunovo commented at 7:15 AM on July 2, 2025:

Try ConsumeIntegralInRange(0, std::numeric_limits<int32_t>::max())

ismaelsadeeq commented at 11:33 AM on July 2, 2025:

Also same at fee_rate fuzz test.

polespinasa commented at 7:11 AM on July 7, 2025:

Great! Thanks :) Done

polespinasa commented at 12:02 PM on June 28, 2025: contributor

c4af90f4a80832cbaa925d33dbf4ba0f0eab6374 adds tests for negative size in full constructor. 0aaeba9fe79df5ed51b3802056f93c0e7857a9fe uses Assume(num_bytes >= 0) instead of returning -1 if size is negative.

in src/policy/feerate.cpp:31 in c4af90f4a8 outdated

  45 |      return nFee;
  46 |  }
  47 |  
  48 |  std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
  49 |  {
  50 | +    const CAmount rate_per_kvb = GetFeePerK();

ismaelsadeeq commented at 4:18 PM on July 1, 2025:

nit: be less ambiguous?

    const CAmount feerate_per_kvb = GetFeePerK();

polespinasa commented at 7:03 AM on July 7, 2025:

done

in src/policy/feerate.h:9 in c4af90f4a8 outdated

   5 | @@ -6,6 +6,7 @@
   6 |  #ifndef BITCOIN_POLICY_FEERATE_H
   7 |  #define BITCOIN_POLICY_FEERATE_H
   8 |  
   9 | +#include <util/feefrac.h>

ismaelsadeeq commented at 8:17 PM on July 1, 2025:

nit: includes are alphabetical

ismaelsadeeq commented at 8:19 PM on July 1, 2025: member

The current iteration is looking better, I will do another round of review and testing tomorrow.

Just a few nits after a short glance

in src/test/amount_tests.cpp:1 in c4af90f4a8 outdated

Eunovo commented at 7:08 AM on July 2, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374: Consider

diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp
index 40f3119cf1..65278aaa41 100644
--- a/src/test/amount_tests.cpp
+++ b/src/test/amount_tests.cpp
@@ -78,13 +78,15 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
     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)
-    // we no longer lose precision
-    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](/bitcoin-bitcoin/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<int32_t>::max()).GetFeePerK();

polespinasa commented at 7:49 AM on July 7, 2025:

lgtm, done!

in src/test/fuzz/fee_rate.cpp:23 in c4af90f4a8 outdated

  19 | @@ -20,8 +20,8 @@ FUZZ_TARGET(fee_rate)
  20 |      const CFeeRate fee_rate{satoshis_per_k};
  21 |  
  22 |      (void)fee_rate.GetFeePerK();
  23 | -    const auto bytes = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
  24 | -    if (!MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) {
  25 | +    const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();

Eunovo commented at 7:13 AM on July 2, 2025:

https://github.com/bitcoin/bitcoin/pull/32750/commits/c4af90f4a80832cbaa925d33dbf4ba0f0eab6374:

diff --git a/src/test/fuzz/fee_rate.cpp b/src/test/fuzz/fee_rate.cpp
index 20578c325e..26146bc617 100644
--- a/src/test/fuzz/fee_rate.cpp
+++ b/src/test/fuzz/fee_rate.cpp
@@ -20,8 +20,8 @@ FUZZ_TARGET(fee_rate)
     const CFeeRate fee_rate{satoshis_per_k};
 
     (void)fee_rate.GetFeePerK();
-    const auto bytes = fuzzed_data_provider.ConsumeIntegral<int32_t>();
-    if (bytes >= 0 && !MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) {
+    const auto bytes = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(0, std::
numeric_limits<int32_t>::max());
+    if (!MultiplicationOverflow(int64_t{bytes}, satoshis_per_k)) {
         (void)fee_rate.GetFee(bytes);
     }
     (void)fee_rate.ToString();

polespinasa commented at 7:05 AM on July 7, 2025:

Great! Thanks

in src/policy/feerate.cpp:7 in c4af90f4a8 outdated

   6 | @@ -7,39 +7,30 @@
   7 |  #include <policy/feerate.h>

ismaelsadeeq commented at 10:46 AM on July 2, 2025:

nit licence update

in src/policy/feerate.cpp:11 in c4af90f4a8 outdated

   6 | @@ -7,39 +7,30 @@
   7 |  #include <policy/feerate.h>
   8 |  #include <tinyformat.h>
   9 |  
  10 | -#include <cmath>
  11 |  
  12 | -CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)
  13 | +CFeeRate::CFeeRate(const CAmount& nFeePaid, int32_t num_bytes)

ismaelsadeeq commented at 10:48 AM on July 2, 2025:

nit: since we are changing the datatype, also change the name to virtual_bytes?

in src/policy/feerate.h:32 in c4af90f4a8 outdated

  27 | @@ -27,52 +28,57 @@ enum class FeeEstimateMode {
  28 |  };
  29 |  
  30 |  /**
  31 | - * Fee rate in satoshis per kilovirtualbyte: CAmount / kvB
  32 | + * Fee rate in satoshis per virtualbyte: CAmount / vB
  33 | + * Wrapping FeeFrac to handle precision issues

ismaelsadeeq commented at 10:50 AM on July 2, 2025:

nit:

 * Fee rate in satoshis per virtualbyte: CAmount / vB
 * the feerate is represented internally as FeeFrac

in src/policy/feerate.h:41 in c4af90f4a8 outdated

  41 | +    FeePerVSize m_feerate;
  42 |  
  43 |  public:
  44 | -    /** Fee rate of 0 satoshis per kvB */
  45 | -    CFeeRate() : nSatoshisPerK(0) { }
  46 | +    /** Fee rate of 0 satoshis per vB */

ismaelsadeeq commented at 10:52 AM on July 2, 2025:

nit: I think it should be clear that when using this constructor we also have 0 virtual bytes, I think it is possible to have 0 fee rate per n virtual bytes I think.

    /** Fee rate of 0 satoshis per 0 vB */

polespinasa commented at 7:17 AM on July 7, 2025:

True! Done

in src/test/fuzz/feefrac.cpp:226 in c4af90f4a8 outdated

 222 | @@ -223,7 +223,7 @@ FUZZ_TARGET(feefrac_mul_div)
 223 |          if (mul64 < std::numeric_limits<int64_t>::max() / 1000 &&
 224 |              mul64 > std::numeric_limits<int64_t>::min() / 1000 &&
 225 |              quot_abs < arith_uint256{std::numeric_limits<int64_t>::max() / 1000}) {
 226 | -            CFeeRate feerate(mul64, (uint32_t)div);
 227 | +            CFeeRate feerate(mul64, (int32_t)div);

ismaelsadeeq commented at 11:21 AM on July 2, 2025:

you can remove the type conversion, I think it is no-op because div is implicitly int32_t

in src/policy/feerate.cpp:22 in 0aaeba9fe7 outdated

  18 | @@ -19,8 +19,8 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, int32_t num_bytes)
  19 |  
  20 |  CAmount CFeeRate::GetFee(int32_t num_bytes) const
  21 |  {
  22 | +    Assume(num_bytes >= 0);

ismaelsadeeq commented at 11:24 AM on July 2, 2025:

why the assume, is it not better to just return the -1? and add a test for it?

Also it was added in first commit and removed here, the two commits should be squashed.

polespinasa commented at 7:26 AM on July 7, 2025:

I don't see in which case a 0 or less can be passed. It should never happen. If it fails on the assume then there's something wrong in another part of the code and we should not treat that as an expected case.

Happy to go back to return -1 if there's a specific case for that.

Eunovo commented at 1:35 PM on July 7, 2025:

+1 on the use of 'Assume' here.

ismaelsadeeq commented at 8:32 AM on August 7, 2025:

Yeah you are right 👍🏾

ismaelsadeeq commented at 11:33 AM on July 2, 2025: member

Code review 0aaeba9fe79df5ed51b3802056f93c0e7857a9fe

I think you should document the current behavior in CFeeRate docstring

Passing any virtual bytes less than or equal to 0 to the constructor will result in 0 fee rate per 0 size.
Passing negative virtual bytes to GetFee will return -1 as the fee.

polespinasa force-pushed on Jul 7, 2025

DrahtBot added the label CI failed on Jul 7, 2025

DrahtBot commented at 8:13 AM on July 7, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45455462527 LLM reason (✨ experimental): The CI failure is due to a trailing whitespace error detected by lint check.

<details><summary>Hints</summary>

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.

</details>

Refactor CFeeRate to use FeeFrac internally

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

d3b8a54a81

polespinasa force-pushed on Jul 7, 2025

polespinasa commented at 9:39 AM on July 7, 2025: contributor

d3b8a54a81209420ef6447dd4581e1b6b8550647 Addressed some of the comments. Mostly nits.

DrahtBot removed the label CI failed on Jul 7, 2025

in src/test/amount_tests.cpp:76 in d3b8a54a81

  72 | @@ -73,18 +73,21 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
  73 |      BOOST_CHECK(CFeeRate(CAmount(-1), 0) == CFeeRate(0));
  74 |      BOOST_CHECK(CFeeRate(CAmount(0), 0) == CFeeRate(0));
  75 |      BOOST_CHECK(CFeeRate(CAmount(1), 0) == CFeeRate(0));
  76 | +    BOOST_CHECK(CFeeRate(CAmount(1), -1000) == CFeeRate(0));

murchandamus commented at 5:39 PM on August 6, 2025:

I was curious whether this test would have passed prior to this change, because the vsize was then expected to be an unsigned integer, and it does.

murchandamus commented at 6:14 PM on August 6, 2025: contributor

code review, lightly tested ACK d3b8a54a81209420ef6447dd4581e1b6b8550647

Passing negative virtual bytes to GetFee will return -1 as the fee.

From experimenting a bit, it seems to me that before this PR, a negative vsize would wrap from max value, i.e. GetFee(<negative>) would return huge amounts (here with a feerate of 1000 s/kvB):

./test/amount_tests.cpp(40): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(-1) == CAmount(-1) has failed [4294967295 != -1]
./test/amount_tests.cpp(41): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(-5) == CAmount(-1) has failed [4294967291 != -1]

The new behavior of throwing an assertion error on negative vsizes seems like an improvement to me.

./policy/feerate.cpp:22 GetFee: Assertion `virtual_bytes >= 0' failed.
unknown location(0): fatal error: in "amount_tests/GetFeeTest": signal: SIGABRT (application abort requested)

DrahtBot requested review from Eunovo on Aug 6, 2025

DrahtBot requested review from theStack on Aug 6, 2025

DrahtBot requested review from ismaelsadeeq on Aug 6, 2025

ismaelsadeeq approved

ismaelsadeeq commented at 8:31 AM on August 7, 2025: member

re-ACK d3b8a54a81209420ef6447dd4581e1b6b8550647 📦

polespinasa requested review from sipa on Aug 7, 2025

polespinasa requested review from davidgumberg on Aug 7, 2025

polespinasa requested review from ajtowns on Aug 7, 2025

in src/policy/feerate.h:83 in d3b8a54a81

  99 | -    friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.nSatoshisPerK); }
 100 | +    friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.m_feerate.fee, f.m_feerate.size); }
 101 | +    friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.m_feerate.fee, f.m_feerate.size); }
 102 |  
 103 | -    SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); }
 104 | +    SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.m_feerate.fee, obj.m_feerate.size); }

theStack commented at 2:52 PM on August 8, 2025:

losely related nit: seems like CFeeRate serialization hasn't been used for more than a decade [1] (other than in the fuzz test) and we could as well just remove it; was shortly looking into that as I was worried that this line could change some file format

[1] AFAICT, the last use was removed in #5159

maflcko commented at 3:16 PM on August 8, 2025:

#22962 ;)

theStack approved

theStack commented at 2:53 PM on August 8, 2025: contributor

Code-review ACK d3b8a54a81209420ef6447dd4581e1b6b8550647

achow101 commented at 12:49 AM on August 9, 2025: member

ACK d3b8a54a81209420ef6447dd4581e1b6b8550647

achow101 merged this on Aug 9, 2025

achow101 closed this on Aug 9, 2025

alexanderwiederin referenced this in commit 8d9cbee66e on Aug 11, 2025

fanquake referenced this in commit 776a163374 on Aug 12, 2025

alexanderwiederin referenced this in commit 30bc73a616 on Aug 12, 2025

Sjors referenced this in commit 3d55222371 on Aug 12, 2025

fanquake referenced this in commit d6887f0cec on Aug 13, 2025

fanquake commented at 8:15 AM on August 13, 2025: member

As pointed out in #33177, this broke the Guix build. Was fixed in #33178.

fanquake referenced this in commit c99f5c5e1b on Aug 15, 2025

stringintech referenced this in commit 71275a1b5e on Aug 17, 2025

fanquake referenced this in commit 89fe999cda on Aug 21, 2025

in src/wallet/test/fuzz/fees.cpp:48 in d3b8a54a81

  44 | @@ -45,8 +45,7 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup)
  45 |      }
  46 |      (void)GetDiscardRate(wallet);
  47 |  
  48 | -    const auto tx_bytes{fuzzed_data_provider.ConsumeIntegral<unsigned int>()};
  49 | -
  50 | +    const auto tx_bytes{fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits<int32_t>::max())};

maflcko commented at 1:20 PM on August 22, 2025:

nit: any reason to exclude negative values? They are documented to behave as-if 0 was passed, so it seems fine to keep the fuzz coverage for it? (https://maflcko.github.io/b-c-cov/fuzz.coverage/src/policy/feerate.cpp.gcov.html)

(There is a unit test covering it, so just a nit)

polespinasa commented at 11:20 PM on August 22, 2025:

Negatives values would make the code crash because of the Assume introduced in GetFee https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/policy/feerate.cpp#L22

$ FUZZ=wallet_fees build_fuzz/bin/fuzz
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2952456953
INFO: Loaded 1 modules   (614477 inline 8-bit counters): 614477 [0x5832503883e0, 0x58325041e42d), 
INFO: Loaded 1 PC tables (614477 PCs): 614477 [0x58325041e430,0x583250d7e900), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
policy/feerate.cpp:22 GetFee: Assertion `virtual_bytes >= 0' failed.
==89225== ERROR: libFuzzer: deadly signal

The addition of that Assertion was recommended here #32750 (comment) and further discussed here #32750 (review) and here #32750#pullrequestreview-3093620724

in src/policy/feerate.cpp:24 in d3b8a54a81

  39 | -        if (nSatoshisPerK < 0) nFee = CAmount(-1);
  40 | -    }
  41 | -
  42 | +    Assume(virtual_bytes >= 0);
  43 | +    if (m_feerate.IsEmpty()) { return CAmount(0);}
  44 | +    CAmount nFee = CAmount(m_feerate.EvaluateFeeUp(virtual_bytes));

maflcko commented at 1:23 PM on August 22, 2025:

nano nit: The c-style cast isn't needed and the narrowing check can be enabled:

    CAmount nFee{m_feerate.EvaluateFeeUp(virtual_bytes)};