fees: wallet: remove block policy fee estimator internals from wallet

ismaelsadeeq commented at 6:45 pm on February 18, 2026: member

Motivation

The wallet currently knows the block policy estimator’s internals from the response returned by estimateSmartFee. estimateSmartFee logging, which is supposed to be inside the method, is done in the wallet. Also, the source of a wallet transaction fee is saved in the CBlockPolicyEstimator::FeeReason enum. This shows that there is tight coupling between the wallet and CBlockPolicyEstimator, which results in code that is a bit messy to extend and even read. In #34075 the wallet is going to use a response returned via a fee rate estimator manager which does not return all those internals, hence motivating this refactor. This PR refactors the interface between the wallet and the fee estimator to improve encapsulation and separate two distinct concerns: the source of a fee rate (where it came from) and the estimator’s reason (why the algorithm chose it).

Key Changes

Separate FeeSource from FeeReason: Introduced a new FeeSource enum in src/util/fees.h to represent the high-level origin of a fee rate (e.g., PAYTXFEE, FALLBACK, FEE_RATE_ESTIMATOR). The existing FeeReason is now strictly scoped to internal CBlockPolicyEstimator details.
Encapsulate estimateSmartFee Logging: Moved the fee calculation logging logic into CBlockPolicyEstimator. This removes the need for the wallet to manage a FeeCalculation object just to facilitate logging, especially when the caller passes nullptr.
Refactor GetMinimumFeeRate: Updated wallet::GetMinimumFeeRate to return a MinimumFeeRateResult struct. This avoids the use of output parameters and provides a cleaner interface for retrieving the fee rate, source, and target.
Move StringForFeeReason: Moved StringForFeeReason to src/policy/fees/block_policy_estimator.cpp to align with the location of the FeeReason enum. The function was also refactored to use a switch statement without a default case to ensure compile-time coverage of all reasons.
Wallet RPC field deprecation : deprecate the fee_reason field in wallet RPCs ( send and sendmany). This field now correctly returns the fee source, but does not rename the field fee_reason to fee_source; instead it deprecates to maintain backward compatibility for some time before renaming the field in the future.
Additional Logging: Added logging for transaction fee, size, and source in CreateTransactionInternal to improve debugging of fee selection.

DrahtBot renamed this:
~~fees: wallet: remove block policy fee estimator internals from wallet~~
fees: wallet: remove block policy fee estimator internals from wallet
on Feb 18, 2026

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

DrahtBot commented at 6:45 pm on February 18, 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
Concept ACK	musaHaruna, Eunovo

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34838 (interfaces: typed receive-request APIs by MkDev11)
#34405 (wallet: skip APS when no partial spend exists by 8144225309)
#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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

[estimateCombinedFee(confTarget/2, HALF_SUCCESS_PCT, true, &tempResult)] in src/policy/fees/block_policy_estimator.cpp
[estimateCombinedFee(confTarget, SUCCESS_PCT, true, &tempResult)] in src/policy/fees/block_policy_estimator.cpp

2026-03-05 11:41:26

DrahtBot added the label Needs rebase on Feb 19, 2026

refactor: use switch in `StringForFeeReason`

- An `assert(false)` is added after the switch to catch any unhandled
  cases during development and testing.

8d7741ddd5

ismaelsadeeq force-pushed on Feb 19, 2026

DrahtBot removed the label Needs rebase on Feb 19, 2026

in src/policy/fees/block_policy_estimator.h:66 in d1c6403745 outdated

62@@ -63,9 +63,6 @@ enum class FeeReason {
63     FULL_ESTIMATE,
64     DOUBLE_ESTIMATE,
65     CONSERVATIVE,
66-    MEMPOOL_MIN,

Eunovo commented at 6:49 am on February 24, 2026:

https://github.com/bitcoin/bitcoin/pull/34617/commits/d1c640374532b0e78abaaeacb4805e7192c49835:

Removing these from block_policy_estimator seems sensible, because they are not used in estimateSmartFee, and they are not part of the possible FeeReason that can be returned from estimateSmartFee.

in src/qt/sendcoinsdialog.cpp:866 in d1c6403745 outdated

862@@ -862,7 +863,7 @@ void SendCoinsDialog::updateSmartFeeLabel()
863 
864     ui->labelSmartFee->setText(tr("%1/kvB").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), feeRate.GetFeePerK())));
865 
866-    if (reason == FeeReason::FALLBACK) {
867+    if (reason == FeeReason::NONE) { // FIXME: use correct enum

Eunovo commented at 6:53 am on February 24, 2026:

https://github.com/bitcoin/bitcoin/pull/34617/commits/d1c640374532b0e78abaaeacb4805e7192c49835:

Why not use FeeSource::FALLBACK here?

ismaelsadeeq commented at 7:02 am on February 24, 2026:

This is an intermediate commit to simplify review. It is used here 991df9bd182679e1fb1632ea1688042030fd264f

Eunovo commented at 10:42 am on February 27, 2026:

I think it’s fine if you also update WalletImpl::getMinimumFee in interfaces.cpp to use FeeSource in this commit. It doesn’t seem like it requires a lot of changes, and you can ditch the // FIXME comment.

in src/wallet/rpc/spend.cpp:194 in 1b6ee3c07c outdated

190@@ -191,7 +191,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
191     if (verbose) {
192         UniValue entry(UniValue::VOBJ);
193         entry.pushKV("txid", tx->GetHash().GetHex());
194-        entry.pushKV("fee_reason", StringForFeeReason(res->fee_calc.reason));
195+        entry.pushKV("fee_source", StringForFeeSource(res->m_fee_source));

Eunovo commented at 9:36 am on February 24, 2026:

https://github.com/bitcoin/bitcoin/pull/34617/commits/1b6ee3c07c9a0761c1026e77dbc30241b13854be:

There is a sufficient reduction in the amount of information being returned to the user here. The user can no longer see the reason for the fee rate returned from the CBlockPolicyEstimator. This reduces debugging information. See https://github.com/maflcko/bitcoin-core/issues/22#issue-616251578 and https://github.com/bitcoin/bitcoin/pull/10284

ismaelsadeeq commented at 9:56 am on February 24, 2026:

Indeed, and I marked the commit as a breaking change. This is still in line with what @maflcko suggested in the original issue that introduces the field.

The goal of that issue is:

“When the wallet funds a transaction, it needs to add inputs until all output amounts as well as the fee are satisfied. The feerate is either set explicitly by the user or estimated.

To aid debugging, the fee reason should be reported to the user.”

And after this PR, we report that to the user as a fee_source; fee_reason is a misnomer and includes other information from the estimatesmartfee algorithm, which is internal there.

When a user sees that the fee estimator is the fee source, they can rightfully investigate that algorithm if they want further information. And if the goal is debugging, they should have debug mode on and will see the estimatesmartfee reason in the log.

This PR just corrects the hierarchy, simplifies the API and makes it extendable.

Eunovo commented at 10:10 am on February 24, 2026:

Isn’t it possible that users already depend on the information from this RPC in production?

Also, is the exact fee reason from the estimator classified as “internal implementation detail”? It appears to be useful output to me.

EDIT: if it is an internal implementation detail, why do we return it from estimateSmartFee at all?

ismaelsadeeq commented at 10:54 am on February 24, 2026:

Isn’t it possible that users already depend on the information from this RPC in production?

This is indeed a valid concern. But I go ahead to break in this case because

The risk appears very low. As noted in the original rationale for adding the output, it exists to aid debugging
It is only available when verbose is true in these wallet sending RPC’s.

But I am open to other suggestions on how to fix the issue, I see these options

Add a dummy fee_reason with output as NONE as a safety net for people who might match this? (Curious also to know a use-case apart from debugging)?
Maintain complete backward compatibility by not changing the output to the fee source, i.e making it fee_reason but changing and reducing the outputs to only the fee source, marking it as deprecated for removal.
Leave is as is on the basis that the output is a misnomer, and include outputs that should not be there.
Leave the current status quo (note: that is incompatible with #34075 because we will have another fee rate estimator that will return output from mempool based fee rate estimator, which does not have those fee reason outputs)

My preference would be 3 or 2, as 1 dummy output is not good in general and a hacky solution, and 4 is not compatible with my PR approach (I can change the approach to not clean this up and leave things as is with a workaround that basically uses the wallet current conditions (Kitchen sink), but I rather clean this up while I touching this). wdyt?

Also, is the exact fee reason from the estimator classified as “internal implementation detail”? It appears to be useful output to me. EDIT: if it is an internal implementation detail, why do we return it from estimateSmartFee at all?

Indeed fee reason can be a useful output for someone using something like estimaterawfee, which is explicitly a debugging RPC that is the main rationale for those values being returned.

Fun fact: Fee reason is not returned in estimatesmartfee RPC at all, which is the correct behaviour.

Eunovo commented at 9:04 am on February 25, 2026:

You can return the fee source and reason in the output as shown below:

0 "fee_source": "Fee Rate Estimator",
1 "fee_reason": "Conservative Double Target longer horizon",

or

0"fee_reason": "Fee Rate Estimator: Conservative Double Target longer horizon"

ismaelsadeeq commented at 11:35 am on March 5, 2026:

or

0"fee_reason": "Fee Rate Estimator: Conservative Double Target longer horizon"

In the latest push, I implemented option 2, which is similar to your suggestion above and @musaHaruna suggestion.

– Another approach I can think of that avoids the deprecation is to reverse what is currently implemented: have the wallet retain the FeeReason enum, and update the block policy enumerator to something like EstimateFeeReason or EstimateFeeDecision. This completely avoids the deprecation.

musaHaruna commented at 8:47 pm on March 3, 2026: contributor

Concept ACK on separating FeeSource from FeeReason

Maintain complete backward compatibility by not changing the output to the fee source, i.e making it fee_reason but changing and reducing the outputs to only the fee source, marking it as deprecated for removal.

I think option 2 makes sense IMO, if the goal is to avoid breaking changes while giving users who rely on this field time to migrate. During the transition period, the field can remain named fee_reason, while clearly informing users that its output now represents the fee source (i.e., where the fee rate came from) rather than the estimator’s internal reasoning.

I think this approach helps correct the misnomer by removing outputs that should not be exposed, while still maintaining backward compatibility. It also avoids leaking internal estimator details, since estimateSmartFee now logs its internal reasoning directly in this PR, and the wallet no longer handles or exposes those internals. As a result, the estimator logic remains properly encapsulated.

Renamed the fee_reason field to fee_source in wallet RPCs (such as send, walletcreatefundedpsbt).

Not sure if sendand walletcreatefundedpsbt are the correct RPCs being affected. Unless am missing something, because fee_source appears in SendMoney() which is only used by sendtoaddress and sendmany. This seems consistent with the comment in the function: // This function is only used by sendtoaddress and sendmany. So it looks like the RPCs affected might actually be sendtoaddress and sendmany.

in src/wallet/interfaces.cpp:30 in 991df9bd18 outdated

25@@ -26,6 +26,7 @@
26 #include <wallet/load.h>
27 #include <wallet/receive.h>
28 #include <wallet/rpc/wallet.h>
29+#include <wallet/types.h>
30 #include <wallet/spend.h>

musaHaruna commented at 9:01 pm on March 3, 2026:

Nit: can be sorted alphabetically

0#include <wallet/spend.h>
1#include <wallet/types.h>

ismaelsadeeq force-pushed on Mar 5, 2026

DrahtBot added the label CI failed on Mar 5, 2026

ismaelsadeeq commented at 11:25 am on March 5, 2026: member

Forced pushed from 991df9bd182679e1fb1632ea1688042030fd264f to f21af8784ac44045f1f3428746e20ac935cd7340 compare diff

Thanks for the review @Eunovo @musaHaruna

#34617 (review) indeed, fixed.
#34617 (review) switched to option 2 here.
#34617 (review) Fixed the nit
#34617#pullrequestreview-3884966440 updated the PR description to be more accurate.

C.I failure seems unrelated, see https://github.com/bitcoin/bitcoin/issues/34731

refactor: split fee source from estimation reason enum

- The FeeReason enum mixed two distinct concepts: the source of a fee
  rate (e.g fallback, required and fee rate estimator) and the reason of estimateSmartFee
  output (e.g., half-estimate, conservative).

- This commit refactor this enum separating concerns by
  introducing a new `FeeSource` enum.

    - `FeeSource`: Represents the origin of a fee, such as
      `FEE_RATE_ESTIMATOR`, `MEMPOOL_MIN`, `USER_SPECIFIED`
      `FALLBACK`, and `REQUIRED`.

    - `FeeReason`: Now exclusively represents the reason for a fee
      estimateSmartFee output, such as `HALF_ESTIMATE` or `CONSERVATIVE`.

- Functions `GetMinimumFeeRate` and `GetMinimumFee` are updated to
  accept a `FeeSource*` parameter, enabling callers to determine the
  source of the resulting fee rate.

- A functional test in `wallet_basic.py` has been temporarily commented out.
  This test validates the wallet sending rpc return the correct fee source,
  with the current split of the enums, the fee_reason output does not
  contain the expected result.
  This will be fixed in a subsequent commit.

afc54abddb

wallet: rpc: return fee source in fee_reason field.

- The key fee_reason is deprecated and marked for rename to an accurate
  key description in the future. This commit returns the correct value
  to fee_reason but do not update the key name to maintain backward
  compatibility during the deprecation period.

e495e5c2fe

fees: make estimateSmartFee log internal

- Since the FeeCalc argument can be a nullptr and we
  want to log values of the FeeCalc in estimateSmartFee.
  This commit updates estimateSmartFee to create a FeeCalc local
  variable which is used for the log and then copied into the
  user passed FeeCalc argument if it is not a nullptr.

- The omitted Reason field from the log will be returned in a subsequent commit.

8c206e4a12

refactor: fees: move StringForFeeReason to block policy estimator

- This commit also added the Reason field back to to the log
  of estimateSmartFee method.

9180bd33c6

wallet: log transaction fee in `CreateTransactionInternal`

- The transaction size in bytes and the transaction fee source
  are also logged.

2ebd1ceea4

refactor: return struct from GetMinimumFeeRate instead of using out-params

- This commit refactors the `GetMinimumFeeRate` function to return a
  `MinimumFeeRateResult` struct containing the fee rate, fee source, and
  confirmation target. This avoids the use of output parameters, leading to
  cleaner and more readable code.

- The `GetMinimumFee` function is also updated to take the new struct as a
  parameter.

- All call sites have been updated to use the new function signatures.

f21af8784a

ismaelsadeeq force-pushed on Mar 5, 2026

DrahtBot removed the label CI failed on Mar 10, 2026

ismaelsadeeq requested review from musaHaruna on Mar 16, 2026

ismaelsadeeq requested review from Eunovo on Mar 16, 2026

Eunovo commented at 5:41 pm on March 17, 2026: contributor

Concept ACK

I think it’s a bit weird that some intermediate commits add some new function parameters that get removed in later commits. I took a stab at rearranging the commits to see if I could eliminate that; I came up with this https://github.com/Eunovo/bitcoin/tree/2026-03-remove-estimatesmartfee-internals-from-wallet Take a look and feel free to use.

fees: wallet: remove block policy fee estimator internals from wallet #34617

Motivation

Key Changes

Reviews

Conflicts

LLM Linter (✨ experimental)