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 depends on CBlockPolicyEstimator internals through FeeCalculation and the old FeeReason enum returned by estimateSmartFee.

That enum mixed two different concepts:

why the block policy estimator chose a specific estimate, such as the half-target, full-target, double-target, or conservative threshold;
why the wallet selected the fee rate it will use, such as fallback fee, mempool minimum fee, user-specified fee, or minimum required fee.

This coupling makes wallet fee selection harder to reason about and exposes estimator-specific details through wallet code and wallet RPC output. This PR separates those concerns so the estimator owns its internal estimate reasons and the wallet reports only wallet-level fee selection reasons.

Key Changes

Split wallet and estimator fee reasons:
- Added a wallet-facing FeeReason enum in src/util/fees.h with FEE_RATE_ESTIMATOR, MEMPOOL_MIN, USER_SPECIFIED, FALLBACK, and REQUIRED.
- Renamed the estimator reason enum to BlockPolicyEstimateReason and scoped it to estimator outputs: NONE, HALF_ESTIMATE, FULL_ESTIMATE, DOUBLE_ESTIMATE, and CONSERVATIVE.
Stop passing estimator internals through wallet fee selection:
- Replaced the FeeCalculation* output parameter in wallet::GetMinimumFeeRate with a MinimumFeeRateResult struct containing the selected fee rate, wallet fee reason, and returned target.
- Updated wallet, Qt, RPC, interface, and test/fuzz callers to use the wallet fee reason instead of FeeCalculation.
- Updated CreatedTransactionResult to store the wallet FeeReason instead of a full FeeCalculation.
Keep wallet RPC fee_reason at wallet scope:
- Verbose wallet RPC responses still use the existing fee_reason field name for compatibility.
- The field now reports the wallet fee selection reason, not the block policy estimator's detailed threshold reason.
- The help text for the affected wallet RPC responses was updated to describe this behavior.
Move estimator reason stringification into the estimator:
- Moved StringForBlockPolicyEstimateReason out of src/common/messages.cpp and into src/policy/fees/block_policy_estimator.cpp.
- Implemented it with a switch without a default case so missing enum cases can be caught by compiler warnings.
Move detailed estimator logging into estimateSmartFee:
- CBlockPolicyEstimator::estimateSmartFee now always populates a local FeeCalculation, even when the caller does not pass one, so estimator debug logging remains available inside the estimator.
- The detailed estimate debug log now lives in estimateSmartFee, where the estimator data originates.
- Wallet transaction creation now logs only the selected fee, transaction size, and wallet fee reason.

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.

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Approach ACK	musaHaruna
Stale ACK	Eunovo

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:

#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35436 (wallet: Add addHDkey interface by pseudoramdom)
#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.

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

ismaelsadeeq force-pushed on Apr 21, 2026

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

I fixed a silent merge conflict after https://github.com/bitcoin/bitcoin/pull/34382

rkrux commented at 4:58 PM on April 21, 2026: contributor

Re: #34617 (comment)

Thanks for the ping, I'll take a look at the PR. It will be sometime before I am able to get to it though.

DrahtBot added the label Needs rebase on May 20, 2026

ismaelsadeeq force-pushed on May 26, 2026

DrahtBot removed the label Needs rebase on May 26, 2026

DrahtBot added the label CI failed on May 26, 2026

ismaelsadeeq commented at 7:19 AM on May 27, 2026: member

Rebased to fix merge conflict. 456785..5162

The C.I failure here is unrelated to the changes in this PR.

DrahtBot removed the label CI failed on May 28, 2026

ismaelsadeeq force-pushed on Jun 17, 2026

fees: split mixed FeeReason into wallet and block policy reasons

The block policy estimator's FeeReason enum mixed two unrelated concerns:
the threshold that produced an estimateSmartFee output (NONE, HALF_ESTIMATE, ...)
and the reason the wallet ended up selecting a fee rate (FALLBACK, MEMPOOL_MIN,
REQUIRED). Split them so each layer owns the reasons it actually uses:

- Add a wallet-facing FeeReason enum (util/fees.h) with the main reasons
  the wallet selects a fee rate for: FEE_RATE_ESTIMATOR, MEMPOOL_MIN,
  USER_SPECIFIED, FALLBACK, REQUIRED, plus a StringForFeeReason helper.

- Rename the estimator's enum to BlockPolicyEstimateReason and narrow it to
  the reasons the estimator produces: NONE, HALF_ESTIMATE, FULL_ESTIMATE,
  DOUBLE_ESTIMATE, CONSERVATIVE.

- Migrate the wallet (and the QT/RPC/interfaces callers) off FeeCalculation
  and onto the new FeeReason, returned via MinimumFeeRateResult. The rest of
  the wallet no longer needs access to FeeCalculation internals.

The two enums no longer share any values: BlockPolicyEstimateReason is
internal to the estimator, FeeReason is the wallet's public fee reason.

The detailed log in CreateTransactionInternal has been deleted and
replaced with a simple log without estimateSmartFee internals because
it does not belong in that scope. The detailed log will be added to
estimateSmartFee method in the next commit.

cc64dde8ce

fees: move StringForBlockPolicyEstimateReason to block policy estimator

Now that the wallet reports its own FeeReason, StringForBlockPolicyEstimateReason
is only used internally by the block policy estimator. Move it from
common/messages into the estimator, written as a switch so the compiler
warns on unhandled cases.

Also add the detailed FeeCalculation debug log to estimateSmartFee, where
the FeeCalculation data originates, and always populate feeCalc locally so
the log is available even when the caller does not pass one.

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

e50e002e3b

ismaelsadeeq force-pushed on Jun 29, 2026

ismaelsadeeq commented at 11:42 PM on June 29, 2026: member

Forced pushed 9cb269b..e50e002e to match changes with parent PR.

I squash the commits to ditch the intermediate fix-me.

musaHaruna commented at 12:16 PM on July 1, 2026: contributor

Approach ACK

Reviewed at cc64dde.

I like the new approach on keeping the fee_reason RPC field name for compatibility while changing its semantics to report the wallet's fee selection reason rather than the block policy estimator's internal threshold reason. I also like updating the RPC help text to reflect the new behavior.

Nit:

In commit message:

The block policy estimator's FeeReason enum mixed two unrelated concerns:

could be

The block policy estimator's FeeReason enum mixed two unrelated concepts:

to match the wording used in the PR description.

Suggestion (non-blocking):

For the updated RPC help text:

{RPCResult::Type::STR, "fee_reason", "The reason the wallet selected this fee rate (e.g. fee rate estimator, mempool minimum, fallback, or minimum required). It no longer exposes the block policy estimator's detailed threshold internals."}

I wonder if the second sentence could simply be omitted. The first sentence already clearly describes what fee_reason represents, and I think it's an improvement over the previous "The transaction fee reason" description.

Alternatively, if users are expected to obtain the estimator's detailed threshold reason elsewhere, perhaps the documentation could point them toward estimatesmartfee instead. Although, after this change, those detailed estimator reasons seem primarily intended for internal/debug logging:

LogDebug(BCLog::ESTIMATEFEE,
    "... Reason:\"%s\" ...",
    StringForBlockPolicyEstimateReason(feeCalc->reason), ...);

So this is more of a documentation suggestion than a request for change—feel free to ignore if you think the explicit clarification is valuable.