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

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
ACK	Eunovo
Concept ACK	musaHaruna

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:

#34405 (wallet: skip APS when no partial spend exists by 8144225309)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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.

DrahtBot added the label Needs rebase on Feb 19, 2026

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:

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

or

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

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

or

"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.

Eunovo commented at 4:45 AM on March 20, 2026:

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.

I don't fully understand this approach, and I think deprecating it as is done in https://github.com/bitcoin/bitcoin/pull/34617/commits/9e53ec10ab6c01c7d219012347e1bd9f0c0325b7#diff-26141d9c7da21eeb4b9e3ffedfaad83212d4710a9e62888f7abea076ca1d0538 is probably better.

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

#include <wallet/spend.h>
#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

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: move StringForFeeReason to block policy esitmator

This commit prepares for the removal of FeeCalculation from wallet code in a future commit.

The FeeCalculation debug log in CreateTransactionInternal is moved into estimateSmartFee
    and replaced with a simpler log that doesn't log the details of feeCalc.

Remove fee reason output from rpc and comment out corresponding functional test.
This will be fixed in future commit.

Co-authored-by: Novo <eunovo9@gmail.com>

a66402a85f

wallet: limit FeeCalculation struct usage to wallet/fees

The rest of the wallet only requires the fee_reason (soon to be changed to fee_source)
from the fee estimation result.

Refactor GetMinimumFeeRate to return the fee_reason along with
the rate and target in a struct for the wallet code to use.

Remove unused block policy estimator includes in the wallet.

Co-authored-by: Novo <eunovo9@gmail.com>

1c55f8f6c3

fees: add FeeSource enum

The current FeeReason enum mixes the source of the fee estimate
and the reason for the fee estimate from the estimator.
The FeeSource enum is added to represent the source of a fee estimate.

The FeeReason enum will be modified to only contain
the reason an estimate is selected by the block policy estimator
in a future commit.

Co-authored-by: Novo <eunovo9@gmail.com>

8872aa8473

wallet: replace FeeReason enum usage with FeeSource enum

The current FeeReason enum mixes the source of the fee estimate
and the reason for the fee estimate from the estimator; it will be
updated to only represent the fee estimate reason in subsequent commit.

This commit refactors the wallet code to use the new
FeeSource enum.

Co-authored-By: Novo <eunovo9@gmail.com>

9e53ec10ab

fees: remove fee sources from FeeReason enum

The FeeReason enum is edited to only contain the reason for an estimate
from estimateSmartFee.

Co-authored-By: Novo <eunovo9@gmail.com>

e1ff0e2571

refactor: use switch in `StringForFeeReason`

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

aa6198a6bf

ismaelsadeeq force-pushed on Mar 19, 2026

ismaelsadeeq commented at 12:09 PM on March 19, 2026: member

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.

Rebased, taken with a few modifications. Thanks for your review and suggestions @Eunovo

35cd7340..aa6198a6

Eunovo approved

Eunovo commented at 4:42 AM on March 20, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/34617/commits/aa6198a6bf51b3f36993d3e0ded9c90aad463660

I wasn't sure about replacing fee_reason with fee_source, but I'm comfortable with deprecating it instead. It may be possible to keep fee_reason as is and just deprecate it, while adding a new fee_source key to the response, but I'm not sure it's worth it, since the fee_source information should be enough for the user.

ismaelsadeeq commented at 4:21 PM on April 7, 2026: member

cc @rkrux @achow101