fees: refactor: separate feerate format from fee estimate mode #34552

ismaelsadeeq commented at 6:21 PM on February 10, 2026: member

Motivation

Part of #34075

The FeeEstimateMode enum was responsible for both selecting the fee estimation algorithm and specifying the fee rate' format.

Changes in this PR:

The FeeEstimateMode enum (UNSET, ECONOMICAL, CONSERVATIVE) is moved to a new util/fees.h header.
A new FeeRateFormat enum (BTC_KVB, SAT_VB) is introduced in policy/feerate.h for feerate formatting.
The CFeeRate::ToString() method is updated to use FeeRateFormat.
All relevant function calls have been updated to use the new FeeRateFormat enum for formatting and FeeEstimateMode for fee estimation mode.

This refactoring separates these unrelated responsibilities to improve code clarity.

DrahtBot added the label TX fees and policy on Feb 10, 2026

DrahtBot commented at 6:22 PM on February 10, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, furszy, musaHaruna, willcl-ark

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ismaelsadeeq commented at 6:24 PM on February 10, 2026: member

cc @l0rinc, @Eunovo, as discussed IRL.

I also split it into two commits as you asked.

in src/common/messages.cpp:1 in ea62e21ec3 outdated

l0rinc commented at 9:04 PM on February 10, 2026:

nit: consider a slightly more descriptive commit message since a few things have happened here:

fees: split feerate format from estimate mode

Introduce `FeeRateFormat` in `policy/feerate.h` and change `CFeeRate::ToString()` to use it for `BTC/kvB` vs `sat/vB` output formatting.

Keep `FeeEstimateMode` focused on fee estimation behavior by removing formatting values from `util/fees.h`.

Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee-rate display concerns from fee-estimation mode selection.

in src/policy/feerate.cpp:9 in eefd9d5427

   5 | @@ -6,6 +6,7 @@
   6 |  #include <consensus/amount.h>
   7 |  #include <policy/feerate.h>
   8 |  #include <tinyformat.h>
   9 | +#include <util/fees.h>

l0rinc commented at 9:05 PM on February 10, 2026:

eefd9d5427aa12ab7dbd0501f3e063b212523bbd I don't think we need this in the first commit

in src/policy/feerate.cpp:30 in ea62e21ec3

  26 | @@ -27,11 +27,11 @@ CAmount CFeeRate::GetFee(int32_t virtual_bytes) const
  27 |      return nFee;
  28 |  }
  29 |  
  30 | -std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
  31 | +std::string CFeeRate::ToString(const FeeRateFormat& feerate_format) const

l0rinc commented at 9:07 PM on February 10, 2026:

the enum name splits the word fee from format:

std::string CFeeRate::ToString(const FeeRateFormat& fee_rate_format) const

or just

std::string CFeeRate::ToString(const FeeRateFormat& format) const

in src/policy/feerate.cpp:35 in ea62e21ec3

  33 |      const CAmount feerate_per_kvb = GetFeePerK();
  34 | -    switch (fee_estimate_mode) {
  35 | -    case FeeEstimateMode::SAT_VB: return strprintf("%d.%03d %s/vB", feerate_per_kvb / 1000, feerate_per_kvb % 1000, CURRENCY_ATOM);
  36 | +    switch (feerate_format) {
  37 | +    case FeeRateFormat::SAT_VB: return strprintf("%d.%03d %s/vB", feerate_per_kvb / 1000, feerate_per_kvb % 1000, CURRENCY_ATOM);
  38 |      default:                      return strprintf("%d.%08d %s/kvB", feerate_per_kvb / COIN, feerate_per_kvb % COIN, CURRENCY_UNIT);

l0rinc commented at 9:08 PM on February 10, 2026:

since we have a dedicated enum, we don't need a default anymore, consider something like:

std::string CFeeRate::ToString(const FeeRateFormat& format) const
{
    const CAmount feerate_per_kvb{GetFeePerK()};
    switch (format) {
    case FeeRateFormat::BTC_KVB: return strprintf("%d.%08d %s/kvB", feerate_per_kvb / COIN, feerate_per_kvb % COIN, CURRENCY_UNIT);
    case FeeRateFormat::SAT_VB:  return strprintf("%d.%03d %s/vB", feerate_per_kvb / 1000, feerate_per_kvb % 1000, CURRENCY_ATOM);
    } // no default case, so the compiler can warn about missing cases
    assert(false);
}

in src/wallet/rpc/spend.cpp:16 in ea62e21ec3

  11 | @@ -12,6 +12,8 @@
  12 |  #include <rpc/rawtransaction_util.h>
  13 |  #include <rpc/util.h>
  14 |  #include <script/script.h>
  15 | +#include <univalue.h>
  16 | +#include <util/fees.h>

l0rinc commented at 9:14 PM on February 10, 2026:

do we need the FeeEstimateMode in this file?

in src/wallet/spend.cpp:20 in ea62e21ec3

  16 | @@ -19,6 +17,7 @@
  17 |  #include <script/signingprovider.h>
  18 |  #include <script/solver.h>
  19 |  #include <util/check.h>
  20 | +#include <util/fees.h>

l0rinc commented at 9:16 PM on February 10, 2026:

we don't seem to need FeeEstimateMode here either after the changes

l0rinc approved

l0rinc commented at 9:20 PM on February 10, 2026: contributor

Change looks mostly good to me, I would prefer unifying the commit messages and removing the default case from CFeeRate::ToString and some imports that may not be needed after the split.

refactor: move-only: move `FeeEstimateMode` enum to `util/fees.h` 922ebf96ed

ismaelsadeeq force-pushed on Feb 11, 2026

ismaelsadeeq commented at 12:43 PM on February 11, 2026: member

Forced pushed from ea62e21ec3f7e733fa89a77cde4003545f426f2f to 501347649cf4f02e0762350fd54a09d1315e14d5 compare diff 5f426f2f..501347

I addressed review comments from @l0rinc, thanks. #34552 (review) Taken with a few modifications. #34552 (review) We don't, removed. #34552 (review) Taken, but did not modify other occurrences in the method. Doing that is beyond the scope of this PR. should be done everywhere. #34552 (review) Taken. #34552 (review) We don't, removed. #34552 (review) Indeed removed.

ismaelsadeeq force-pushed on Feb 11, 2026

DrahtBot added the label CI failed on Feb 11, 2026

DrahtBot commented at 12:49 PM on February 11, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task Alpine (musl): https://github.com/bitcoin/bitcoin/actions/runs/21905411377/job/63244294391</sub> <sub>LLM reason (✨ experimental): Compiler error: feerate.cpp::ToString lacks a return value, with -Werror treating this warning as an error.</sub>

<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 force-pushed on Feb 11, 2026

DrahtBot removed the label CI failed on Feb 11, 2026

l0rinc approved

l0rinc commented at 2:15 PM on February 11, 2026: contributor

ACK 501347649cf4f02e0762350fd54a09d1315e14d5

in src/policy/feerate.cpp:29 in 501347649c

  25 | @@ -26,11 +26,13 @@ CAmount CFeeRate::GetFee(int32_t virtual_bytes) const
  26 |      return nFee;
  27 |  }
  28 |  
  29 | -std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
  30 | +std::string CFeeRate::ToString(const FeeRateFormat& fee_rate_format) const

furszy commented at 3:06 PM on February 11, 2026:

nano nit: usually, enums are passed by value.

l0rinc commented at 3:11 PM on February 11, 2026:

indeed, we should fix that while we're here

ismaelsadeeq commented at 3:49 PM on February 11, 2026:

Done in latest push https://github.com/bitcoin/bitcoin/compare/501347649cf4f02e0762350fd54a09d1315e14d5..c1355493e2c26b613109bfac3dcd898b3acca75a

furszy commented at 3:07 PM on February 11, 2026: member

ACK 501347649cf4f02e0762350fd54a09d1315e14d5

refactor: fees: split fee rate format from fee estimate mode

 - Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
   to use it for `BTC/kvB` vs `sat/vB` output formatting.
 - Handle all enum values, hence remove default case in `CFeeRate::ToString()`
   and `assert(False)` when a `FeeRateFormat` value is not handled.
 - Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
   values from `FeeEstimateMode`.
 - Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
   from fee-estimation mode selection.

c1355493e2

ismaelsadeeq force-pushed on Feb 11, 2026

in src/util/fees.h:17 in 922ebf96ed

  12 | +    CONSERVATIVE, //!< Force estimateSmartFee to use conservative estimates
  13 | +    BTC_KVB,      //!< Use BTC/kvB fee rate unit
  14 | +    SAT_VB,       //!< Use sat/vB fee rate unit
  15 | +};
  16 | +
  17 | +#endif // BITCOIN_UTIL_FEES_H

furszy commented at 3:57 PM on February 11, 2026:

The missing blank line at the end of the file will likely cause the CI lint job to fail

l0rinc commented at 4:18 PM on February 11, 2026:

GitHub is probably drunk: <img width="270" height="41" alt="image" src="https://github.com/user-attachments/assets/d67bc88a-40a8-46cf-a697-e5bceaa81a72" />

ismaelsadeeq commented at 4:18 PM on February 11, 2026:

I don't think so, the C. I was green previously.

furszy commented at 4:24 PM on February 11, 2026:

I don't think so, the C. I was green previously.

~~you removed it in the last push~~ I see, you removed another line. nvm.

l0rinc commented at 4:18 PM on February 11, 2026: contributor

ACK c1355493e2c26b613109bfac3dcd898b3acca75a

DrahtBot requested review from furszy on Feb 11, 2026

furszy commented at 4:26 PM on February 11, 2026: member

utACK c1355493e2c26b613109bfac3dcd898b3acca75a

musaHaruna commented at 2:07 PM on February 12, 2026: contributor

ACK c135549 — reviewed in the context of PR 34075

willcl-ark approved

willcl-ark commented at 10:54 AM on February 16, 2026: member

ACK c1355493e2c26b613109bfac3dcd898b3acca75a

Seperating unrelated concerns (feerate and formatting) out makes sense. No stale FeeEstimateMode::BTC_KVB/SAT_VB references remaining.

l0rinc commented at 9:32 AM on February 17, 2026: contributor

rfm?

sedited merged this on Feb 17, 2026

sedited closed this on Feb 17, 2026

ismaelsadeeq deleted the branch on Feb 17, 2026

in src/common/messages.cpp:70 in c1355493e2

  67 | @@ -68,7 +68,6 @@ std::string FeeModeInfo(const std::pair<std::string, FeeEstimateMode>& mode, std
  68 |                     "less responsive to short-term drops in the prevailing fee market. This mode\n"
  69 |                     "potentially returns a higher fee rate estimate.\n", mode.first);
  70 |          default:

maflcko commented at 3:22 PM on February 17, 2026:

nit: Please don't use a default case. This will prevent the compiler from checking. Please use the documented approach as per the dev notes:

} // no default case, so the compiler can warn about missing cases
    assert(false);

l0rinc commented at 3:25 PM on February 17, 2026:

This wasn't added in this PR, it's why I didn't comment on it. Would it make sense to add a dedicated PR for fixing these?

maflcko commented at 3:41 PM on February 17, 2026:

This should be the only case in the codebase, no? But I am happy to review a pull fixing this and all other cases.

ismaelsadeeq commented at 4:23 PM on February 17, 2026:

@maflcko this is not a change added in this PR I am happy to review a patch fixing it as well.

l0rinc commented at 4:27 PM on February 17, 2026:

There are a few more cases like this, I'll add a PR for it

maflcko commented at 8:06 PM on February 26, 2026:

Done in #34684. I only found two other cases, besides the one here. In theory a clang-tidy rule can be written, but this may not be worth it with so few cases.

sedited referenced this in commit 925759d172 on Feb 23, 2026

sedited referenced this in commit 8640523843 on Feb 23, 2026