fees: Introduce Mempool Based Fee Estimation to reduce overestimation #34075

ismaelsadeeq commented at 11:53 AM on December 15, 2025: member

This PR is another attempt to fix #27995 using a better approach.

For background and motivation, see #27995 and the discussion in the Delving Bitcoin post Mempool Based Fee Estimation on Bitcoin Core.

This PR is currently limited to using the mempool only to lower what is recommended by the Block Policy Estimator. Accurate and safe fee estimation using the mempool is challenging. There are open questions about how to prevent mempool games that are theoretically possible for miners (a variant of the Finney attack).

This is one of the reasons I opted to use the mempool only to lower the Block Policy Estimator, which itself is not gameable, and therefore this mechanism is not susceptible to this attack.

The underlying assumption here is that, with the current tools and work done to make RBF and CPFP feasible and guarantee (TRUC transaction relay, ephemeral anchors, cluster size 2 package RBF), it is safer to underestimate rather than overestimate. We now assume it is relatively easy to fee-bump later if a transaction does not confirm, whereas once a fee is overestimated there is no way to recover from that.

Another open question when using the mempool for fee estimation is how to account for incoming transaction inflow. Bitcoin Augur does this by using past inflow plus a constant expected inflow to predict future inflow. I find this unconvincing for fee estimation and potentially prone to more overestimation, as past conditions are not always representative of the future. See my review of the Augur fee rate estimator and open questions.

This PR uses a much simpler approach based on current user behavior, similar to the widely used method employed by mempool.space: looking at the top block of the mempool and selecting a percentile feerate depending on whether the user is economical or conservative.

Empirical data from both myself and Clara Shikhelman shows that the 75th percentile feerate for economical users and the 50th percentile feerate for conservative users provide positive confirmation guarantees, hence this is what is used in this PR.

Parallel research by Rene Pickhardt and his student suggests that using the average fee per byte of the block template performs well.

All of these are constants that can be adjusted, there is parallel work exploring these constants and running benchmarks across fee estimators to find a sweet spot.

See also work in LND, the LND Budget Sweeper, which applies this idea successfully. Their approach is to estimate fees initially with bitcoind, then increment gradually as the confirmation deadline approaches, using a fixed fee budget.

Historical data indicates that by doing only the approach in this PR, we are able to reduce overestimation quite significantly (~29%).

This is particularly useful in scenarios where the Block Policy Estimator recommends a high feerate while the mempool is empty. <img width="1800" height="1090" alt="56f3ba26c0184521c42bb82ec9d8c9f2224d4f8e" src="https://github.com/user-attachments/assets/c035c40c-8ece-42a7-b290-d29f1ac9bf4d" />

As seen in the image above, there is only one remaining unfixed case: when there is a sudden inflow of transactions and the feerate rises, the Block Policy Estimator takes time to reflect this. In that case, users will continue to see a low feerate estimate until it slowly updates. From the historical data linked above, this occurs about ~26% of the time.

Overall, we observe a 73% success rate with 0% overestimation, and 26% underestimation with this approach.

See https://bitcoincorefeerate.com/stats for recent running stats that have almost identical data.

This PR also includes refactors that enable this work. Rather than splitting the PR and implementing changes incrementally, I opted for an end-to-end implementation:

1. Refactors

Split the mixed fee reason enum into separate wallet and block policy concepts. The wallet now has a FeeReason enum for why the wallet selected a fee rate (FEE_RATE_ESTIMATOR, MEMPOOL_MIN, USER_SPECIFIED, FALLBACK, REQUIRED), while the Block Policy Estimator uses BlockPolicyEstimateReason for its internal threshold details.
Move StringForBlockPolicyEstimateReason to the Block Policy Estimator code, keeping the estimator-specific strings with the estimator.
Move detailed Block Policy Estimator logging out of wallet transaction creation and into the estimator path. Wallet transaction creation now logs the selected fee and wallet fee reason instead of leaking estimator internals.
Keep the wallet RPC fee_reason field name for compatibility, but update its meaning to report the wallet fee reason instead of the Block Policy Estimator's internal threshold reason.
Rename policy estimator tests and files to block-policy-specific names where appropriate.
Update Block Policy Estimator unit tests to be independent of the mempool and validation interface.

2. Introduce Mempool-Based Fee Estimator and Fee Estimator Manager

Introduce FeeRateEstimatorResult as the response type, containing estimator metadata and avoiding new out-parameters.
Add FeeRateEstimatorType to identify the estimator that produced a result.
Add FeeRateEstimatorManager, responsible for owning the Block Policy Estimator and Mempool Fee Rate Estimator.
Update the node context to store a unique_ptr to FeeRateEstimatorManager instead of CBlockPolicyEstimator.
Update CBlockPolicyEstimator to no longer subscribe directly to the validation interface; instead, FeeRateEstimatorManager subscribes and forwards relevant notifications.
Add a mempool fee estimator that generates a block template when called, calculates a percentile feerate, and returns the 75th percentile for economical mode or the 50th percentile for conservative mode.
When the mempool estimator is used and the selected estimate is below the node's fee floor, estimatesmartfee still returns at least the max of mempoolminfee and minrelaytxfee.
Add caching to the mempool estimator so that new estimates are generated only every 7 seconds, assuming enough transactions have propagated to make a meaningful difference. This heuristic will likely be replaced by requesting block templates via the general-purpose block template cache proposed here: #33389
Update MempoolTransactionsRemovedForBlock to receive the connected block transactions as well as the transactions removed from the mempool.
Track the weight of block transactions and mempool transactions removed due to block connection after each block connection. This data is tracked for the last 6 mined blocks. A mempool feerate estimate is returned only when the average ratio of mempool transaction weight removed due to block connection to block transaction weight is greater than 75%. This heuristic provides rough confidence that the node's mempool matches that of the majority of the hashrate. The 75% threshold is arbitrary and can be adjusted.

There is a caveat when transactions in the local mempool are consistently not mined by the network, as described in #27995 (e.g. due to filtering). Accounting for these transactions during fee estimation is not necessary, as they should be evicted from the mempool itself (see #33510). Handling this again within fee estimation would be redundant.

Persist statistics for the 6 most recent mined blocks to fees/mempool_policy_estimates.dat during periodic flushes and shutdown, so this data is available after restarts.
Move Block Policy Estimator data from fee_estimates.dat to fees/block_policy_estimates.dat, migrating the legacy file during startup when needed.
Add block_policy_only to the estimatesmartfee options object. When true, estimatesmartfee uses only the Block Policy Estimator. By default it is false, so the manager may use the Mempool Fee Rate Estimator when it returns a lower valid estimate.
Add verbosity to the estimatesmartfee options object. With verbosity >= 2 and block_policy_only=false, the RPC returns recent mempool health statistics.
Expose the selected fee rate estimator in estimatesmartfee results when block_policy_only=false.
Add unit, functional, and fuzz test coverage for the new estimator behavior, persistence, RPC options, and estimator I/O.

<details> <summary>see example output</summary>

bitcoin-cli estimatesmartfee 1 economical '{"verbosity": 2, "block_policy_only": false}'

{
  "feerate": 0.00002133,
  "estimator": "Mempool Fee Rate Estimator",
  "blocks": 2,
  "mempool_health_statistics": [
    {
      "block_height": 927953,
      "block_weight": 3991729,
      "mempool_txs_weight": 3942409
    },
    ...
  ]
}

</details>

DrahtBot added the label TX fees and policy on Dec 15, 2025

DrahtBot commented at 11:53 AM on December 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/34075.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sedited, davidgumberg, polespinasa
Stale ACK	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:

#35587 (Remove boost as a unit test runner by rustaceanrob)
#35581 (node: add block template manager and track waitNext fee inflow by ismaelsadeeq)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35474 (node: move index ownership to NodeContext by w0xlt)
#35436 (wallet: Add addHDkey interface by pseudoramdom)
#35105 (Refactor: Updated TransactionError to use util::Expected and removed TransactionError:OK by kevkevinpal)
#34803 (mempool: asynchronous mempool fee rate diagram updates via validation interface by ismaelsadeeq)
#34565 (refactor: extract BlockDownloadManager from PeerManagerImpl by w0xlt)
#34405 (wallet: skip APS when no partial spend exists by 8144225309)
#33741 (rpc: Optionally print feerates in sat/vb by polespinasa)
#33637 (refactor: optimize block index comparisons (1.4-6.8x faster) by l0rinc)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
#32427 (kernel: Replace leveldb-based BlockTreeDB with WAL and .dat file based store by sedited)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#28690 (build: Introduce internal kernel library by sedited)
#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.

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):

self.broadcast_and_maybe_mine(self.nodes[1], high_feerate, TXS_COUNT, 12, self.nodes[2]) in test/functional/feature_fee_estimation.py
self.broadcast_and_maybe_mine(self.nodes[1], low_feerate, TXS_COUNT, 4, self.nodes[2]) in test/functional/feature_fee_estimation.py
self.broadcast_and_maybe_mine(self.nodes[1], feerate_1_s_per_vb, TXS_COUNT, 1, self.nodes[2]) in test/functional/feature_fee_estimation.py
self.broadcast_and_maybe_mine(self.nodes[1], high_feerate, TXS_COUNT, 6, miner) in test/functional/feature_fee_estimation.py
AddRemovedBlock(mempool_estimator, 0, 0, height) in src/test/mempool_fee_estimator_tests.cpp

2026-07-01 15:30:45

DrahtBot added the label CI failed on Dec 15, 2025

ismaelsadeeq force-pushed on Dec 15, 2025

DrahtBot removed the label CI failed on Dec 15, 2025

gmaxwell commented at 11:01 PM on December 15, 2025: contributor

I think this is a good idea, especially if it there was a 'very conservative' that just didn't use this.

Accounting for these transactions during fee estimation is not necessary, as they should be evicted from the mempool itself

I don't agree here-- rather time based expiration should probably be eliminated over time. If there really is a censored transaction then we want nodes to work on getting it mined, and not just give up if the censors manage to control the blocks for a long time.

Also two weeks is already long time to allow a non-minable transaction to distort fee estimates. And it's a short time for low rate confirmation-- there have been times in history when transactions weren't confirmed for months after their initial announcement (though those were also at time so far where size based expiration would have eliminated them).

So I suggest instead that future work (not this PR) could improve the estimator by excluding from considerations transactions that should have been mined according to our template but weren't for more than a threshold number of blocks, perhaps 6 or something like that.

Such code could also be used to log/warn the user that there is either ongoing censorship or that their node's policy appears to be less restrictive than the network.

But in any case it doesn't need to be done immediate to make use of this kind of estimator.

Overall, we observe a 73% success rate with 0% overestimation, and 26% underestimation with this approach.

One thing to keep in mind is that if this approach is widely used the success rate will probably change. It might make sense to initially deploy with more conservative settings (/defaults) and slowly increase them to avoid causing an abrupt change.

DrahtBot added the label Needs rebase on Dec 16, 2025

ismaelsadeeq force-pushed on Dec 16, 2025

ismaelsadeeq commented at 3:39 PM on December 16, 2025: member

I don't agree here; rather, time-based expiration should probably be eliminated over time. If there really is a censored transaction, then we want nodes to continue working to get it mined, not just give up if censors manage to control blocks for a long time.

Indeed. In the context of censorship, we would want to keep such transactions if they are incentive compatible. However, there may also be legitimate reasons miners deliberately exclude them for example, a legacy node that is unaware of a soft fork that invalidates a certain spend (e.g., post-quantum).

Hence as you also mentioned, I would like to see the direction of #33510, leaning toward deferring this improvement to future work.

Especially if there were a “very conservative” option that simply didn’t use this. One thing to keep in mind is that if this approach is widely used, the success rate will probably change. It might make sense to initially deploy with more conservative settings (or defaults) and slowly increase them to avoid causing an abrupt change.

I considered this and even had a branch that added an option to estimatesmartfee to force block-policy-only behavior.

However, the users this targets are those running bitcoind with default settings and trusting developers to make sane default choices. I’ve seen this scenario before with previous conservative defaults: complaints and issues accumulated over time, eventually leading us to switch block policy the default to economical, which provided a significant reduction in overestimation and better accuracy.

FWIW, I also did some research over a short block interval to get a sense of what Bitcoin core wallet users uses for feerate estimation, and I find that the majority of Bitcoin Core wallet users (that I was able to fingerprints) are not using the block-policy estimator, we’ve seen self-hosted users 1 2 switch approaches in the past, I’ve also personally spoken to large Bitcoin services that rely on third party but want this feature.
It seems to me enabling this by default feels like the better direction, IMHO.

DrahtBot removed the label Needs rebase on Dec 16, 2025

gmaxwell commented at 2:57 PM on December 17, 2025: contributor

I think enabling it by default probably makes sense, that particular sub comment was more about having a ready way to choose not to use it when transacting.

As far as the initial defaults I think there I meant that that it shouldn't target too close to the bottom of the mempool as even if that currently gets high success that may not be true after wide deployment. Though on reflection I'm not sure that thats true as this only lowers feerates, so other people using this should only increase your odds of success while using it.

DrahtBot added the label Needs rebase on Dec 18, 2025

ismaelsadeeq force-pushed on Dec 22, 2025

DrahtBot removed the label Needs rebase on Dec 22, 2025

ismaelsadeeq commented at 4:40 PM on January 8, 2026: member

@brunoerg friendly ping for a mutation score and un-killed mutants 🔪

I will fix the issues brought up by @DrahtBot soon.

brunoerg commented at 5:00 PM on January 8, 2026: contributor

@brunoerg friendly ping for a mutation score and un-killed mutants 🔪

I will fix the issues brought up by @DrahtBot soon.

I'm running it.

in src/policy/fees/mempool_estimator.cpp:147 in fee261249f outdated

 142 | +    if (prev_mined_blocks.back().m_height != current_height) {
 143 | +        LogDebug(BCLog::ESTIMATEFEE, "%s: Most Recent BlockData is not the chain tip", FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()));
 144 | +    }
 145 | +    if (!total_block_txs_weight) return false;
 146 | +    auto average_percentile = total_removed_txs_weight / total_block_txs_weight;
 147 | +    return average_percentile >= HEALTHY_BLOCK_PERCENTILE;

brunoerg commented at 9:42 PM on January 9, 2026:

Unkilled mutant:

diff --git a/src/policy/fees/mempool_estimator.cpp b/muts-pr-34075-mempool_estimator-cpp/mempool_estimator.mutant.51.cpp
index 7c41644103..c2c9dbd49c 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/muts-pr-34075-mempool_estimator-cpp/mempool_estimator.mutant.51.cpp
@@ -144,7 +144,7 @@ bool MemPoolFeeRateEstimator::isMempoolHealthy(size_t current_height) const
     }
     if (!total_block_txs_weight) return false;
     auto average_percentile = total_removed_txs_weight / total_block_txs_weight;
-    return average_percentile >= HEALTHY_BLOCK_PERCENTILE;
+    return average_percentile > HEALTHY_BLOCK_PERCENTILE;
 }

ismaelsadeeq commented at 5:39 PM on January 11, 2026:

This requires making the average_percentile exactly .75; the current unit test is based on approximation, thats why this is un-killed.

brunoerg commented at 1:19 PM on January 12, 2026:

Good to know, thank you for the feedback.

ismaelsadeeq commented at 2:04 PM on January 12, 2026:

I am considering adding a fuzz test which can easily provide such an arrangement, such that the percentile is exactly at .75, but this PR is currently quite large in terms of diff.

I will leave that as an idea for a follow-up to avoid further expanding the scope.

Thanks for the mutants.

in src/policy/fees/mempool_estimator.cpp:145 in fee261249f outdated

 140 | +        total_removed_txs_weight += current.m_removed_block_txs_weight;
 141 | +    }
 142 | +    if (prev_mined_blocks.back().m_height != current_height) {
 143 | +        LogDebug(BCLog::ESTIMATEFEE, "%s: Most Recent BlockData is not the chain tip", FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()));
 144 | +    }
 145 | +    if (!total_block_txs_weight) return false;

brunoerg commented at 9:42 PM on January 9, 2026:

Unkilled mutant:

diff --git a/src/policy/fees/mempool_estimator.cpp b/muts-pr-34075-mempool_estimator-cpp/mempool_estimator.mutant.48.cpp
index 7c41644103..0b808bcb35 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/muts-pr-34075-mempool_estimator-cpp/mempool_estimator.mutant.48.cpp
@@ -142,7 +142,7 @@ bool MemPoolFeeRateEstimator::isMempoolHealthy(size_t current_height) const
     if (prev_mined_blocks.back().m_height != current_height) {
         LogDebug(BCLog::ESTIMATEFEE, "%s: Most Recent BlockData is not the chain tip", FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()));
     }
-    if (!total_block_txs_weight) return false;
+
     auto average_percentile = total_removed_txs_weight / total_block_txs_weight;
     return average_percentile >= HEALTHY_BLOCK_PERCENTILE;
 }
@@ -196,4 +196,4 @@ FeeRateEstimatorResult MemPoolFeeRateEstimator::EstimateFeeRate(int target, bool
     result.feerate = conservative ? percentiles.p50 : percentiles.p75;
     result.returned_target = MEMPOOL_FEE_ESTIMATOR_MAX_TARGET;
     return result;

ismaelsadeeq commented at 5:37 PM on January 11, 2026:

Killed thanks

in src/policy/fees/mempool_estimator.cpp:156 in fee261249f

 151 | +{
 152 | +    LOCK(cs);
 153 | +    FeeRateEstimatorResult result;
 154 | +    result.feerate_estimator = GetFeeRateEstimatorType();
 155 | +    auto activeTip = WITH_LOCK(cs_main, return m_chainstate->m_chainman.ActiveTip());
 156 | +    if (!activeTip) {

brunoerg commented at 9:45 PM on January 9, 2026:

Unkilled mutant (corecheck shows this verification has no test coverage at all):

diff --git a/src/policy/fees/mempool_estimator.cpp b/muts-pr-34075-mempool_estimator-cpp/mempool_estimator.mutant.53.cpp
index 7c41644103..17a5c28093 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/muts-pr-34075-mempool_estimator-cpp/mempool_estimator.mutant.53.cpp
@@ -153,7 +153,7 @@ FeeRateEstimatorResult MemPoolFeeRateEstimator::EstimateFeeRate(int target, bool
     FeeRateEstimatorResult result;
     result.feerate_estimator = GetFeeRateEstimatorType();
     auto activeTip = WITH_LOCK(cs_main, return m_chainstate->m_chainman.ActiveTip());
-    if (!activeTip) {
+    if (1==0) {
         result.errors.emplace_back(strprintf("%s: No active chainstate available", FeeRateEstimatorTypeToString(result.feerate_estimator)));
         return result;
     }

ismaelsadeeq commented at 5:36 PM on January 11, 2026:

This is gone now. Thanks

ismaelsadeeq force-pushed on Jan 11, 2026

DrahtBot added the label CI failed on Jan 11, 2026

DrahtBot commented at 6:53 PM on January 11, 2026: contributor

🚧 At least one of the CI tasks failed. Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/20899189424/job/60042443886 LLM reason (✨ experimental): Compilation failed due to a dangling reference error in mempool_estimator.cpp, treated as an error by -Werror.

<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 Jan 12, 2026

DrahtBot removed the label CI failed on Jan 12, 2026

musaHaruna commented at 6:22 AM on January 16, 2026: contributor

This is very impressive and a nice improvement. This is quite a big PR so I've spent some time trying to understand what the PR is doing conceptually, why the design choices were made, and what trade-offs are being accepted, before diving deeply into the code. I’m still new, so please treat the points below as learning-oriented feedback rather than authoritative critique

High-level understanding/summary

From my understanding, this PR Introduces a mempool-based fee estimator that supplements (but never replaces) the existing Block Policy Estimator i.e:

Final fee = min(Block Policy Estimator, mempool-based estimate)

This means: Fees are never increased above the Block Policy Estimator using mempool data. Mempool data can only lower the recommendation when conditions look favorable. If the local mempool is deemed unreliable (via the “mempool health” heuristic), the system falls back entirely to the Block Policy Estimator.

Threat model, assumptions, and mitigations

Miner manipulation (Finney-like attacks): A miner could try to seed the mempool with fake high-fee transactions or withhold a block to raise mempool fees before releasing it (a variant of a Finney attack). However, because this PR uses min(Block Policy Estimator, mempool_estimate), an attacker cannot force the recommended fee above the safe Block Policy Estimator value (which is based on confirmed blocks). In effect, miners can only induce under-bidding, not over-bidding. As a result, the worst-case is that transactions are slower to confirm (underestimate), but never require overpayment. Underpayment is considered safer since transactions can be fee bumped later if needed. So the assumption here is “it is relatively easy to fee-bump later if a transaction does not confirm, whereas once a fee is overestimated there is no way to recover from that”

Mempool Representativeness & Filtering Issues: Local mempool divergence (e.g. due to policy differences or connectivity) could otherwise spoil the estimator. The PR addresses this through the “mempool health” (sanity) check: it tracks, for each of the last 6 blocks, how much weight of the block’s transactions was present in our mempool when the block arrived. If the average is below 75%, we assume our mempool isn’t representative (e.g. we missed many tx, or we hold tx miners wouldn’t mine) therefore we will not use the mempool estimator, falling back to Block Policy Estimator.

To be more elaborate IIUC, based on the mempool health, choose estimator:

If mempool is healthy: Bitcoin Core may use the mempool estimator by simulating the next block and computing a percentile fee (e.g., 50th or 75th percentile). Then it takes:

final_fee = min(Block Policy Estimator, mempool fee)

If mempool is not healthy: Bitcoin Core skips the mempool estimator entirely and just uses the Block Policy Estimator

Mempool health check make sure our mempool ≈ to the miners’ mempool. If my understanding is correct.

So I’m guessing this will be dynamic i.e always performing mempool health checks before choosing either to use Block Policy Estimator or mempool Estimator depending on the above conditions.

Question: Are we treating the 75% mempool-health threshold as a statistically meaningful guarantee of fee-estimate correctness, or merely as a coarse guardrail — and do we have empirical evidence about false positives where the guardrail passes but the estimate is still misleading?

Adversarial flooding of mempool: The assumption here is that malicious peer could flood the local mempool with many very low-fee or very high-fee transactions. If low-fee spam dominates the top of the mempool histogram, the chosen percentile fee rate drops sharply, causing an underestimate. This is tolerated (the user may pay less but risk delay), and in practice the mempool “health” checks will likely disable mempool-based estimates if the mempool is dominated by attacker traffic. Flooding with high-fee spam cannot raise the fee estimate beyond Block Policy Estimator, limiting damage.

Choice of Percentiles and Other Heuristics: The PR chooses fixed fee percentiles for the mempool-based estimator based on empirical testing. In economical mode, it returns the 75th percentile fee from a simulated block template built from the current mempool, while conservative mode returns the 50th percentile. The percentile represents the fee rate at which that fraction of the block’s total weight would be filled when transactions are ordered by fee rate. These choices aim to balance cost and reliability: higher percentiles lower fees but increase delay risk, while lower percentiles raise fees and improve confirmation odds. Mainnet testing (Feb–Mar 2024) showed almost no overpayments and about 26% underpayments, compared to roughly 29% overpayments when using the historical Block Policy Estimator alone.

Beyond percentiles, the PR introduces two additional heuristics: a 7-second cache and a 75% mempool weight threshold. The cache avoids rebuilding block templates too frequently, based on the assumption that mempool contents do not change meaningfully within a few seconds. The weight threshold checks whether the local mempool is representative by requiring that, on average, at least 75% of the transaction weight in recent blocks was already present in the node’s mempool before mining. If this condition is not met, the estimator ignores the mempool and falls back to Block Policy Estimator. These constants are explicitly acknowledged as tunable and somewhat arbitrary. Overall, the heuristics are intended as a practical starting point that improves fee accuracy without compromising safety, while leaving room for refinement.

Handling Sudden Inflows (“Unfixed Case”): A key concern is when the mempool suddenly fills (fee demand surges) after the estimate was computed but before the transaction is mined. In this case the mempool-based estimate was too low and Block Policy Estimator will catch up only gradually via future blocks. Historical data suggests this happens roughly 26% of the time, accounting for most underestimation cases. In such cases users may need an RBF bump once the fee crisis becomes apparent.

Mempool health check suggestions

The PR already exposes detailed mempool_health_statistics via estimatesmartfee (verbosity > 2), which is a great foundation. However, I think node operators and wallet developers currently lack guidance on how to interpret and act on this data. I suggest the following improvements and documentation clarifications:

Clarify how mempool health is interpreted Document that each entry shows how much of a mined block’s weight was already present in the local mempool. Explain that the estimator averages the ratios over recent blocks (typically 6), and considers the mempool healthy if the average ≥ 75%.

Explicitly state that if the threshold is not met (unless my understanding about this is incorrect), Bitcoin Core falls back to the Block Policy Estimator. Consider exposing: "mempool_healthy": true|false I think this avoids forcing node operators and tooling to recompute the average manually and makes monitoring simpler.

Improve logs when switching estimators: I don’t know whether this already exists. But I think adding explicit log lines when mempool estimator is enabled or disabled due to health and which estimator was chosen for a given estimatesmartfee call will also be beneficial by making debugging easier when a node suddenly stops using mempool estimates. Examples:

INFO: mempool health avg=0.94 ≥ 0.75 — using mempool estimator
WARN: mempool health avg=0.62 < 0.75 — falling back to Block Policy Estimator

Suggest sanity warnings / metrics Operators monitoring nodes could benefit from: Alerts when mempool health drops below threshold for a long duration A gauge or counter for mempool health ratio over time I think this helps detect network connectivity issues or local policy divergences before they impact fee estimates.

Document interpretation & edge cases Finally I think adding a brief section in the PR or documentation covering: What does it mean when mempool health is low? Should users be concerned if the mempool estimator is frequently disabled? If yes, then when should they be concerned?

I will be reviewing the code very soon.

ismaelsadeeq commented at 2:27 PM on January 19, 2026: member

Thanks for your review @musaHaruna

In effect, miners can only induce under-bidding, not over-bidding. As a result, the worst-case is that transactions are slower to confirm (underestimate), but never require overpayment.

I don't think that is an accurate claim; when there are high-fee transactions available in the mempool, that would directly affect the outcome. There are two scenarios and can think of in that case:

They publish low-fee transactions, and there are high-fee transactions present. In this case, the estimate would be higher than the miners' low-fee transactions, and the final result would be min(mempool, block policy).
They publish low-fee transactions that result in the mempool being lower than the block policy, then you use the mempool, but that's not under-bidding; the mempool is empty anyway, and you would have bid higher with the block policy.

Are we treating the 75% mempool-health threshold as a statistically meaningful guarantee of fee-estimate correctness, or merely as a coarse guardrail — and do we have empirical evidence about false positives where the guardrail passes but the estimate is still misleading?

That's a good question. The 75% is a reasonable constant that is chosen. A 100% match is nearly impossible, and most of the time, when a node has been up for a while, it achieves 95% or above based on empirical evidence. However, it's set much lower than that to avoid making the requirement too strict; for example, a node that starts fresh or a node that restarts after a while should not have to spend a long time to be able to make a mempool-based estimate.

For other questions, you answered yourself already. On suggestions for more debug information, let me know whether you have explicit code suggestions inline on where more info is needed. I believe the current log and info are sufficient. We log after each estimate is made by an estimator, and we also log the final estimate.

musaHaruna commented at 5:07 PM on January 20, 2026: contributor

The PR looks conceptually sound, but I’ll wait for feedback from a more experienced contributor before confirming. In the meantime I have reviewed the first 28 of 29 commits.

fees: separate feerate format from fee estimate mode (44f6548) The commit is separating fee rate format from fee estimate mode (because they are logically independent), by creating separate enum FeeRateFormat. And moving FeeEstimationMode to src/util/fees.h‎. Which makes sense because FeeEstimationMode describes more of a choice, and not a policy rule. Also in the commit's that follows, src/util/fees.h‎ houses fee related utilities so moving FeeEstimationMode is justified I think .
fees: make estimateSmartFee log internal (dfdc26e) Confirmed that logs are now emitted by CBlockPolicyEstimator::estimateSmartFee by running ./build/test/functional/feature_fee_estimation.py --tmpdir $(pwd)/tmp --nocleanup and checking the debug.log file of node0 and confirmed: 2026-01-20T15:22:16.608377Z [httpworker.0] [policy/fees/block_policy_estimator.cpp:976] [estimateSmartFee] [estimatefee] estimateSmartFee Selected feerate :129421 Tgt:2 (requested 1) Reason:"Target 85% Threshold" Decay 0.96200: Estimation: (125239 - 131501) 91.79% 16.2/(17.7 0 mem 0.0 out) Fail: (108186 - 125239) 83.05% 22.7/(27.3 0 mem 0.0 out) I think the commit message can also benefit from adding the new changes of creating a temporary local copy FeeCalculation temp_fee_calc; object that CBlockPolicyEstimator::estimateSmartFee function builds step by step, only at the end (or on failure) does it copy it to *feeCalc, to avoid partially filled data inconsistent output (if that's the intent).
fees: seperate fee estimate mode from fee source (dfdc26e) The commit's separates fee estimation mode from fee estimate source (because they are semantically different) by creating a separate function StringForFeeSource that holds fee estimation sources. This change makes sense. I also noticed commit is also returning the user input confirmation target in GetMinimumFeeRate(const CWallet& wallet, const CCoinControl& coin_control, FeeSource* fee_source, int* returned_target); and CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, FeeSource* fee_source, int* returned_target); which was somehow not captured before the change in this commit. If that's the case, I think incorporating this change in the commit message could also make sense.
fees: move StringForReason to block policy estimator (dd9b1b6) Moves the function StringForFeeReason from src/common/messages.cpp to src/policy/fees/block_policy_estimator.cpp. Which makes sense seeing that it holds fee estimate enums for block policy estimator. Confirmed fee reason is now been logged: 2026-01-20T16:13:41.094109Z [httpworker.0] [policy/fees/block_policy_estimator.cpp:976] [estimateSmartFee] [estimatefee] estimateSmartFee Selected feerate :153904 Tgt:2 (requested 1) Reason:"Target 85% Threshold" Decay 0.96200: Estimation: (152229 - 176224) 94.48% 26.0/(27.5 0 mem 0.0 out) Fail: (131501 - 152229) 81.39% 11.8/(14.5 0 mem 0.0 out)
wallet: log transaction fee in CreateTransactionInternal (3a8931c) Confirmed transaction fee is now been logged, coming from CreateTransactionInternal confirmed this by running ./build/test/functional/wallet_basic.py --tmpdir $(pwd)/tmp --nocleanup checked the debug.log file of node2: 2026-01-20T16:31:34.430738Z [httpworker.0] [wallet/wallet.h:938] [WalletLogPrintf] [default_wallet] Fee Calculation: Fee:2820 Bytes:141 Source: Fallback fee
test: rename policyestimator_tests to blockpolicyestimator_tests (dd9b1b6) Rename makes sense, after scheming through the test file I noticed that the test is mainly testing the block policy estimator functionality, coming commit d117b09, "make blockpolicyestimator_tests independent of mempool", puts more emphasis on my first point.
test: rename policy estimator to block policy estimator (787b436) Rename makes sense, after a brief scheme through the file, I noticed most of the code are block policy estimator codes.
test: make blockpolicyestimator_tests independent of mempool (d117b09) Commit makes test independent of the mempool, by bypassing node machinery and talking to the estimator directly without using CTxMemPool, validation signals,locks,TryAddToMempool, removeForBlock. Instead, It manually constructs NewMempoolTransactionInfo,RemovedMempoolTransactionInfo and calls feeEst.processTransaction(...),feeEst.processBlock(...). Testing the block policy estimator directly without mempool overhead. I wanted to suggest a refactor of the test because I find it a bit difficult to reason about, but I think that will be out of scope for the PR.
fees: add abstract fee estimator (6e83c82) Adds FeeRateEstimator abstract base class for fee rate estimators, which derived classes (CBlockPolicyEstimator, MemPoolFeeRateEstimator) must provide concrete implementation of the virtual methods (EstimateFeeRate() which returns estimated fee rate for a package to confirm within the given target and MaximumTarget() which returns the maximum supported confirmation target). Adds FeeRateEstimatorType a struct used for identifying fee estimators e.g BLOCK POLICY or MEMPOOL POLICY. Adds FeeRateEstimatorResult struct represents the response returned by a fee rate estimator. Response includes: feerate, current_block_height, return_target (target at which the package is likely to confirm within), error and feerate_estimator (identifies which fee rate estimator is providing this feerate estimate).
fees: make block_policy_estimator a FeeRateEstimator (273597) CBlockPolicyEstimator class implement the FeeRateEstimator interface (EstimateFeeRate() and MaximumTarget())
add FeeRateEstimatorManager class (a1c6cc1) Introduces FeeRateEstimatorManager class which acts as a controller for different fee rate estimators. Question: Is there a specific reason for naming the files estimator_man.cpp/h instead of estimator_manager.cpp/h. I think the former is more elaborate and derives home the intent of the file more clearly.
fees: add typed accessor for FeeRateEstimator (838f627) Add GetFeeRateEstimator() a templated method which returns a pointer to a fee rate estimator given an estimator type.
fees: replace CBlockPolicyEstimator with FeeEstimatorManager(8f9aba8) Initializing FeeEstimatorManager in the node instead directly using CBlockPolicyEstimator by replacing CBlockPolicyEstimator with FeeEstimatorManager i.e making use of CBlockPolicyEstimator through FeeEstimatorManager which manages it and other estimators. Now fee rate estimation requests is through FeeEstimatorManager which holds a map of all registered estimators to their unique pointers (std::unordered_map<FeeRateEstimatorType, std::unique_ptr<FeeRateEstimator>> estimators;). Chain interface now returns FeeRateEstimatorResult through which fee rate can be accessed instead. FeeEstimatorManager also inherits from CValidationInterface overidding TransactionAddedToMempool, TransactionRemovedFromMempool, MempoolTransactionsRemovedForBlock (I think it could also be good if this also captured in the commit message).
add CalculatePercentiles function (2b30932) Adds CalculatePercentileswhich calculates the percentile fee rates from a given block template chunks assumes that the chunk fee rates in the input vector are sorted in descending order based on mining score priority and returns Percentiles object containing the calculated percentile (25th,50th, 75th and 95th) fee rates. Left a question and suggestion below.
fees: add MemPoolFeeRateEstimator class (bf69cd1) Adds MemPoolFeeRateEstimator class that inherits from FeeRateEstimator and implements all its virtual method, is a FeeRateEstimator type that uses Bitcoin Core's block-building algorithm to generate the block template that will likely be mined from unconfirmed transactions in the mempool. It calculates its fee rate through EstimateFeeRate() by calculating percentile fee rates from the selected packages of the template: the 75th percentile fee rate is used as the economical fee rate estimate, and the 50th fee rate percentile as the conservative estimates. Left a question below.
fees: add FeeRateEstimatorTypeToString method (76a1d22) Function returns string "Block Policy Estimator" or "Mempool Fee Rate Estimator" depending on the FeeRateEstimatorType. Call the function in logs of CBlockPolicyEstimator::estimateSmartFee and MemPoolFeeRateEstimator::EstimateFeeRate respectively.
test: add mempool fee estimator unit test (73109e7) Did a quick run through review of the test and ran it. It looks good for now. I will look at it again later to see if I can find something.
fees: add caching to MemPoolFeeRateEstimator (c49879d) Adds MemPoolFeeRateEstimatorCache which holds a cache of recent fee rate estimates. A fresh fee rate is only provided if the cached value is older than CACHE_LIFE which is 7secs adds IsStale() which returns true if cache is older that 7secs, GetCachedEstimate() returns the cached Percentiles fee rate if available and not stale, GetChainTipHash() returns the chain tip hash that the cache corresponds to and Update() which updates the cache with a new estimates and chain tip hash.
fees: return mempool estimates when it's lower than block policy (f62fbee) This commit implements final_fee = min(block policy estimator, mempool fee estimator) i.e use the mempool only to lower the Block Policy Estimator fee. It acheive this through FeeRateEstimatorManager::GetFeeRateEstimate() by looping through std::unordered_map<FeeRateEstimatorType, std::unique_ptr<FeeRateEstimator>> estimators;and picking the min between selected_estimate = std::min(selected_estimate, current_estimate) which is equivalent to min(block policy estimator, mempool fee estimator) since there are only two fee rate estimator now. The logic is future proof i.e it can accommodate more fee rate estimators without causing breaking changes as long as it's picking and using one fee rate estimator (minimum between the selected and current fee estimator). But I think the current logic may fail for hybrid fee estimation.
rpc:fees: update estimatesmartfee to return errors whether it succeeds or not (a105a89) Adds an error response to estimatesmartfee also removes unnecessary else clause, so that estimatesmartfee will always return an error field whether error is empty or not.
test: ensure estimatesmartfee returns mempool fee rate estimate (68c7338) Add's test to ensure estmatessmartfee fee rpc returns mempool fee rate estimates. As a small suggestion, this test currently generates transactions at a single fixed feerate, which creates a very clean signal for the estimator. From my understanding, the estimator typically operates over a distribution of feerates rather than one value. It might be possible to improve coverage (while keeping the test deterministic) by broadcasting transactions across a few fixed feerate buckets with different counts before mining blocks. This could help exercise estimator behavior under mixed but controlled conditions, without changing the intent of the test or introducing additional complexity. Also left some few code suggestions below.
validation: add block transactions to MempoolTransactionsRemovedForBlock (43b66e) Adds adds block transaction which will be used in a later commit where block_txs is used to compute the actual non-coinbase weight of the mined block, so the fee estimator can understand real block fullness independently of which transactions happened to be removed from the local mempool
miner: remove unused node/context.h import (4ff35c) Removes unused node/context.h import in in src/node/miner.cpp
fees: only return mempool fee rate estimate when mempool is healthy (b2e711f) This commit ensures that the users mempool has seen at least 75% of the last 6 blocks transactions by using MempoolTxsRemovedForBlock() to keep track of the last N blocks and calculates the total weight of transactions mined in the block and total weight of mempool transactions that were removed This information is later used in IsMempoolHealthy() to check if the mempool is “healthy.” IsMempoolHealthy() gets the current chain tip then calls isMempoolHealthy() to determine health. This function is what external code would call to ask: “Is the mempool healthy right now?”. isMempoolHealthy() is where the core logic resides: it calculates total weights over last N blocks and returns true/false based on the fraction of mempool txs that were mined.
fees: return mempool health stats from feerate estimator manager (68ae3af) Returns health stats through GetPrevBlockData() which returns previous block data stats i.e BlockData struct which contains m_height m_removed_block_txs_weight m_block_weight.
fees: rename and move fee_estimates.dat to fees/block_policy_estimates.dat (3227dc) Commit message is self explained.
fees: add EncodedBlockDataFormatter struct (5c0ec49) Adds a helper EncodedBlockDataFormatter that converts a BlockData object to a stream of bytes for storage or transmission (serialization) and reads it back from a stream to restore the object (deserialization), handling doubles safely with encoding/decoding.
fees: persist mempool estimator statistics (42dfb54) Persist mempool estimator statistics. The commit also enables estimatesmartfee to return the stats when verbosity level is greater than 1. E.G

bitcoin-cli estimatesmartfee 1 "conservative" 2
{
 "feerate": 0.00002133,
 "errors": [],
 "blocks": 2,
 "mempool_health_statistics": [
   {
     "block_height": 927953,
     "block_weight": 3991729,
     "mempool_txs_weight": 3942409,
     "ratio": 0.987644451815241
   },
   ...
 ]
}

fees: persist mempool estimator statistics (42dfb54) I don't know much about fuzzing so I did not review this commit. But I will come back to it later.

in src/wallet/fees.cpp:13 in 76b36fe917

musaHaruna commented at 6:00 PM on January 20, 2026:

In "fees: seperate fee estimate mode from fee source" 659708 Non blocking: I think it can be rearranged like below

#include <wallet/coincontrol.h>
#include <wallet/fees.h>
#include <wallet/wallet.h>

namespace wallet {
CAmount GetRequiredFee(const CWallet& wallet, unsigned int nTxBytes)

musaHaruna commented at 6:27 PM on January 22, 2026:

In "fees: make estimateSmartFee log internal (44f6548)"

Nit, Non blocking suggestion: I think output param FeeCalculation *feeCalc should be at the end of function declaration and definination. Read it somewhere in developer-notes.

polespinasa commented at 10:13 AM on June 25, 2026:

in 23d0e3e24780a065cec3bc9f688318174195dae4 rpc: add block_policy_only param to estimatesmartfee

nit: Maybe worth adding a more "complex" CLI example?

ismaelsadeeq commented at 11:04 PM on June 29, 2026:

Not sure what you mean here, can u add a code suggestion?

in src/util/fees.cpp:16 in 2b30932ca6 outdated

  11 | +    if (chunk_feerates.empty()) return Percentiles{};
  12 | +    int32_t accumulated_weight{0};
  13 | +    const int32_t p25_weight = 0.25 * total_weight;
  14 | +    const int32_t p50_weight = 0.50 * total_weight;
  15 | +    const int32_t p75_weight = 0.75 * total_weight;
  16 | +    const int32_t p95_weight = 0.95 * total_weight;

musaHaruna commented at 6:29 PM on January 22, 2026:

In "add CalculatePercentiles function (2b30932)"

Is there a reason why you are not explicity doing interger arithmetic (const int32_t p25_weight = (total_weight * 25) / 100;)? Because I think doing integer arithmetic is more predictable because it produces the exact cutoff you expect and for clarity because someone reading the code immediately sees “25% of total” is done using integer math.

in src/util/fees.cpp:18 in 2b30932ca6

  13 | +    const int32_t p25_weight = 0.25 * total_weight;
  14 | +    const int32_t p50_weight = 0.50 * total_weight;
  15 | +    const int32_t p75_weight = 0.75 * total_weight;
  16 | +    const int32_t p95_weight = 0.95 * total_weight;
  17 | +    Percentiles percentiles{};
  18 | +

musaHaruna commented at 6:33 PM on January 22, 2026:

In "add CalculatePercentiles function (2b30932)"

Nit Non blocking suggestion: I think Assume(std::is_sorted(chunk_feerates.begin(),chunk_feerates.end(),std::greater<FeePerVSize>())); can be added since the function assumes that the chunk fee rates in the input vector are sorted in descending order.

in src/policy/fees/mempool_estimator.cpp:25 in bf69cd122d

  20 | +    int current_height = WITH_LOCK(cs_main, return m_chainstate->m_chain.Height());
  21 | +    Assume(current_height > -1);
  22 | +    result.current_block_height = static_cast<unsigned int>(current_height);
  23 | +    node::BlockAssembler::Options options;
  24 | +    options.test_block_validity = false;
  25 | +    node::BlockAssembler assembler(*m_chainstate, m_mempool, options);

musaHaruna commented at 6:36 PM on January 22, 2026:

In "fees: add MemPoolFeeRateEstimator class (bf69cd1)"

Is there a reason for not using brace initialization (node::BlockAssembler assembler{*m_chainstate, m_mempool, options}) here?

musaHaruna commented at 6:41 PM on January 22, 2026: contributor

Thanks @ismaelsadeeq for clarifying this:

I don't think that is an accurate claim; when there are high-fee transactions available in the mempool, that would directly affect the outcome. There are two scenarios and can think of in that case:

They publish low-fee transactions, and there are high-fee transactions present. In this case, the estimate would be higher than the miners' low-fee transactions, and the final result would be min(mempool, block policy). They publish low-fee transactions that result in the mempool being lower than the block policy, then you use the mempool, but that's not under-bidding; the mempool is empty anyway, and you would have bid higher with the block policy.

I will get back to you on this:

On suggestions for more debug information, let me know whether you have explicit code suggestions inline on where more info is needed.

Once am done reviewing the rest of the commits

in src/common/messages.cpp:44 in 6597083f9d outdated

  40 | @@ -45,6 +41,22 @@ std::string StringForFeeReason(FeeReason reason)
  41 |      return reason_string->second;
  42 |  }
  43 |  
  44 | +std::string StringForFeeSource(FeeSource source)

rustaceanrob commented at 3:16 PM on January 27, 2026:

Why not use a switch?"

switch (source) {
case FeeSource::FEE_RATE_ESTIMATOR: return "Fee Rate Estimator";
case FeeSource::MEMPOOL_MIN: return "Mempool Min Fee";
case FeeSource::PAYTXFEE: return "PayTx Fee";
case FeeSource::FALLBACK: return "Fallback Fee";
case FeeSource::REQUIRED: return "Minimum Required Fee";
default: return "Unknown fee source";
}

in src/policy/fees/block_policy_estimator.cpp:978 in 76b36fe917 outdated

 990 | +
 991 | +    result.current_block_height = fee_calc.best_height;
 992 | +    result.returned_target = fee_calc.returnedTarget;
 993 | +
 994 | +    if (feerate == CFeeRate(0)) {
 995 | +        result.errors.emplace_back("Insufficient data or no feerate found");

musaHaruna commented at 9:52 AM on January 28, 2026:

I think we can return the estimator type here as well

        result.errors.emplace_back(strprintf("%s: Insufficient data or no feerate found", FeeRateEstimatorTypeToString(result.feerate_estimator)));

musaHaruna commented at 10:31 AM on January 28, 2026: contributor

In test: ensure estimatesmartfee returns mempool fee rate estimate (68c7338)

I think we can make mempool_estimator_error a global variable MEMPOOL_ESTIMATOR_ERROR similar to BLOCK_POLICY_ESTIMATOR_ERROR. Also add the fee rate estimator type to BLOCK_POLICY_ESTIMATOR_ERROR

See below suggestion:

MAX_FILE_AGE = 60
SECONDS_PER_HOUR = 60 * 60
MEMPOOL_FEE_ESTIMATOR_CACHE_LIFE = 7 # Seconds
BLOCK_POLICY_ESTIMATOR_ERROR = "Block Policy Estimator: Insufficient data or no feerate found"
MEMPOOL_ESTIMATOR_ERROR = "Mempool Fee Rate Estimator: Unable to provide a fee rate due to insufficient data"
BLOCK_POLICY_ESTIMATOR_FILE_PATH = "fees/block_policy_estimates.dat"

...

high_feerate, tx_count = Decimal("0.004"), 24
self.broadcast_and_mine_blocks(high_feerate, blocks=6, txs=tx_count)
estimate_from_block_policy = node0.estimatesmartfee(1)
verify_estimate_response(estimate_from_block_policy, high_feerate, [MEMPOOL_ESTIMATOR_ERROR])

in src/policy/fees/estimator_man.h:16 in 43b66e1e9d

  12 | @@ -13,6 +13,7 @@
  13 |  #include <chrono>
  14 |  #include <memory>
  15 |  #include <unordered_map>
  16 | +#include <vector>

musaHaruna commented at 10:36 AM on January 28, 2026:

In validation: add block transactions to MempoolTransactionsRemovedForBlock (43b66e) I removed #include <vector> and compiled successful, not sure if it's neccessary?

in src/policy/fees/mempool_estimator.h:26 in b2e711f885

  21 |  
  22 | +struct BlockData;
  23 | +struct RemovedMempoolTransactionInfo;
  24 | +
  25 | +enum class ChainStateRole;
  26 | +enum class MempoolRemovalReason;

musaHaruna commented at 10:49 AM on January 28, 2026:

In fees: only return mempool fee rate estimate when mempool is healthy (b2e711f)

Also removed the enum declarations:

enum class ChainStateRole;
enum class MempoolRemovalReason;

and compiled successfully. Not sure if they are being used somewhere in a way am not aware of.

in src/policy/fees/mempool_estimator.cpp:81 in b2e711f885 outdated

  76 | +    }
  77 | +    if (prev_mined_blocks.back().m_height != current_height) {
  78 | +        LogDebug(BCLog::ESTIMATEFEE, "%s: Most Recent BlockData is not the chain tip", FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()));
  79 | +    }
  80 | +    if (!total_block_txs_weight) return false;
  81 | +    auto average_percentile = total_removed_txs_weight / total_block_txs_weight;

musaHaruna commented at 11:06 AM on January 28, 2026:

On suggestions for more debug information, let me know whether you have explicit code suggestions inline on where more info is needed

Could add debug info here e.g

LogDebug(BCLog::ESTIMATEFEE,
         "%s: mempool health avg=%.2f threshold=%.2f (%s)",
     FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()),
         average_percentile,
         HEALTHY_BLOCK_PERCENTILE,
      average_percentile >= HEALTHY_BLOCK_PERCENTILE ? "healthy" : "unhealthy, falls back to block policy estimator");

Also updated my review list

ismaelsadeeq force-pushed on Jan 28, 2026

ismaelsadeeq commented at 4:06 PM on January 28, 2026: member

Thanks for your review @musaHaruna and @rustaceanrob I forced pushed from 76b36fe917ec61cd390734278f478b82c2409703 to 761ce13e7c56867f2cf16b475dcc4c6fb67c5d47 compare diff

I did a few changes.

Pass chainstate manager to mempool estimator in case the active chainstate changes e.g due to assumeutxo.

I think the commit message can also benefit from adding the new changes of creating a temporary local copy FeeCalculation temp_fee_calc; object that CBlockPolicyEstimator::estimateSmartFee function builds step by step, only at > the end (or on failure) does it copy it to *feeCalc, to avoid partially filled data inconsistent output (if that's the intent).

I also noticed commit is also returning the user input confirmation target in GetMinimumFeeRate(const CWallet& wallet, const CCoinControl& coin_control, FeeSource* fee_source, int* returned_target); and CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, FeeSource* fee_source, int* returned_target); which was somehow not captured before the change in this commit. If that's the case, I think incorporating this change in the commit message could also make sense.

(I think it could also be good if this also captured in the commit message).

I am not a huge fan of overly verbose commit messages that do not explain logic or rationale, just reinstating what is obvious in code, but they don't hurt, so I added as requested in some cases.

Question: Is there a specific reason for naming the files estimator_man.cpp/h instead of estimator_manager.cpp/h. I think the former is more elaborate and derives home the intent of the file more clearly.

No, I just follow the approach of the codebase, you can see with other managers, man suffix, is shorter and obvious.

#34075 (review) Non blocking: I think it can be rearranged like below

Done

#34075 (review) Nit, Non blocking suggestion: I think output param FeeCalculation *feeCalc should be at the end of function declaration and definination. Read it somewhere in developer-notes.

Rearranging the parameter is out of the scope imo. So I leave as is.

#34075 (review) Is there a reason why you are not explicity doing interger arithmetic (const int32_t p25_weight = (total_weight * 25) / 100;)? Because I think doing integer arithmetic is more predictable because it produces the exact cutoff you expect and for clarity because someone reading the code immediately sees “25% of total” is done using integer math.

Yes, multiplication is faster than (dividing and multiplying), although in this case it is negligible. Moreover this values are fixed, and the block size is also a constant; there is no gain in using integer arithmetic here.

#34075 (review) Nit Non blocking suggestion: I think Assume(std::is_sorted(chunk_feerates.begin(),chunk_feerates.end(),std::greater<FeePerVSize>())); can be added since the function assumes that the chunk fee rates in the input vector are sorted in descending order.

Good suggestion, taken.

#34075 (review) Is there a reason for not using brace initialization (node::BlockAssembler assembler{*m_chainstate, m_mempool, options}) here?

No reason, taken.

#34075 (review) Why not use a switch?"

switch language feature is better indeed, taken.

#34075 (review) I think we can return the estimator type here as well

This is a breaking change, and Bitcoin Core clients most likely match this, so I will leave it as is; in order not break them using this PR.

#34075#pullrequestreview-3715928857 I think we can make mempool_estimator_error a global variable MEMPOOL_ESTIMATOR_ERROR similar to BLOCK_POLICY_ESTIMATOR_ERROR. Also add the fee rate estimator type to BLOCK_POLICY_ESTIMATOR_ERROR

No, my rule of thumb is to define a global if the constant is used in multiple subtests. In this case, it is not. So, leaving as is.

#34075 (review) I removed #include <vector> and compiled successful, not sure if it's neccessary?

It is not indeed, removed.

#34075 (review) Also removed the enum declarations: enum class ChainStateRole; enum class MempoolRemovalReason; and compiled successfully. Not sure if they are being used somewhere in a way am not aware of.

These are stale forward declarations from a previous version of the PR, you are right indeed; they are not needed. Removed.

#34075 (review) Could add debug info here e.g ...

Taken with some modifications.

</details>

DrahtBot added the label CI failed on Jan 28, 2026

DrahtBot commented at 7:17 PM on January 28, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21444811601/job/61757678279 LLM reason (✨ experimental): clang-tidy failure: const-qualification of a function parameter in a declaration (CalculatePercentiles) causing CI to fail.

<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 3, 2026

ismaelsadeeq commented at 3:13 PM on February 3, 2026: member

Forced pushed from 761ce13e7c56867f2cf16b475dcc4c6fb67c5d47 to 522406cdf593c1956403bf4aa7cb51e1a25ea241

c6fb67c5d47..522406cdf The Assume suggested in #34075 (review) caught an edge case. Rounding up when converting from weight to vsize may cause slight misorder. I fixed that by sorting before calling CalculatePercentile

DrahtBot removed the label CI failed on Feb 3, 2026

DrahtBot added the label Needs rebase on Feb 3, 2026

ismaelsadeeq force-pushed on Feb 4, 2026

ismaelsadeeq commented at 10:44 AM on February 4, 2026: member

Rebased to fix merge conflict with #32748 diff 30850f3..f5af1117

DrahtBot removed the label Needs rebase on Feb 4, 2026

in src/rpc/fees.cpp:115 in f5af11176a

 116 | +                for (auto block_it = blocks_data.rbegin(); block_it != blocks_data.rend(); ++block_it) {
 117 | +                    UniValue block_stat(UniValue::VOBJ);
 118 | +                    block_stat.pushKV("block_height", block_it->m_height);
 119 | +                    block_stat.pushKV("block_weight", block_it->m_block_weight);
 120 | +                    block_stat.pushKV("mempool_txs_weight", block_it->m_removed_block_txs_weight);
 121 | +                    block_stat.pushKV("ratio", block_it->m_removed_block_txs_weight / block_it->m_block_weight);

l0rinc commented at 8:56 PM on February 9, 2026:

given that m_block_weight has a default of 0, what is the expected behavior here in that case?

in src/policy/fees/estimator_man.h:45 in f5af11176a

  40 | +     * Return a pointer to a fee rate estimator given an estimator type.
  41 | +     */
  42 | +    template <class T>
  43 | +    T* GetFeeRateEstimator(FeeRateEstimatorType estimator_type)
  44 | +    {
  45 | +        FeeRateEstimator* estimator_ptr = estimators.find(estimator_type)->second.get();

l0rinc commented at 8:58 PM on February 9, 2026:

is it guaranteed that the mempool estimator will always be present?

in src/test/fuzz/policy_estimator_io.cpp:43 in f5af11176a

  45 |      }
  46 | -    (void)fuzzed_auto_file.fclose();
  47 | +    (void)fuzzed_auto_file_block_policy.fclose();
  48 | +    static MemPoolFeeRateEstimator mempool_feerate_estimator{MempoolPolicyFeeestPath(*g_setup->m_node.args), g_setup->m_node.mempool.get(), g_setup->m_node.chainman.get()};
  49 | +    if (mempool_feerate_estimator.Read(fuzzed_auto_file_mempool_policy)) {
  50 | +        mempool_feerate_estimator.Write(fuzzed_auto_file_block_policy);

l0rinc commented at 9:00 PM on February 9, 2026:

        mempool_feerate_estimator.Write(fuzzed_auto_file_mempool_policy);

in src/policy/fees/mempool_estimator.cpp:108 in f5af11176a

 103 | +        LogWarning("%s: Unable to write data to %s (non-fatal)", FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()), fs::PathToString(m_mempool_estimates_file_path));
 104 | +        return false;
 105 | +    }
 106 | +    if (file.fclose() != 0) {
 107 | +        LogError("Failed to close mempool fee estimates file %s: %s. Continuing anyway.", fs::PathToString(m_mempool_estimates_file_path), SysErrorString(errno));
 108 | +        return false;;

l0rinc commented at 9:01 PM on February 9, 2026:

        return false;

in src/policy/fees/mempool_estimator.cpp:55 in f5af11176a

  50 | +    if (file.IsNull()) {
  51 | +        LogDebug(BCLog::ESTIMATEFEE, " %s: %s does not exist. Continue anyway", FeeRateEstimatorTypeToString(GetFeeRateEstimatorType()), fs::PathToString(m_mempool_estimates_file_path));
  52 | +        return;
  53 | +    }
  54 | +    if(Read(file)) {
  55 | +        LogDebug(BCLog::ESTIMATEFEE, "%s successfully read from read.", fs::PathToString(m_mempool_estimates_file_path));

l0rinc commented at 9:01 PM on February 9, 2026:

read from read?

in test/functional/feature_fee_estimation.py:352 in f5af11176a

 362 | -        with self.nodes[0].assert_debug_log(expected_msgs=expected_messages, timeout=1):
 363 | -            # Mock the scheduler for an hour to flush fee estimates to fee_estimates.dat
 364 | +        block_policy_estimator_message = f"Flushed fee estimates to {block_policy_fees_dat}."
 365 | +        mempool_policy_estimator_message = f"{mempool_estimator_name_str}: estimates flushed to {mempool_policy_fee_dat}."
 366 | +        with self.nodes[0].assert_debug_log(expected_msgs=[block_policy_estimator_message, mempool_policy_estimator_message], timeout=1):
 367 | +            # Mock the scheduler for an hour to flush fee estimates to block_policy_estimates.dat nd mempool_policy_estimates.dat

l0rinc commented at 9:02 PM on February 9, 2026:

            # Mock the scheduler for an hour to flush fee estimates to block_policy_estimates.dat and mempool_policy_estimates.dat

l0rinc commented at 2:31 PM on February 10, 2026: contributor

Posting a few simple notes during our pairing session.

ismaelsadeeq force-pushed on Feb 10, 2026

DrahtBot added the label Needs rebase on Feb 10, 2026

ismaelsadeeq force-pushed on Feb 10, 2026

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

Forced pushed from f5af11176acaacd5a5039b37289a021c44469d8a to 03659c71071f6cb447ddd6999a6b8376aca4b575 after review from @l0rinc and rebased to fix merge conflict a78a1a805..03659

#34075 (review) indeed can be 0, good catch, fixed.
#34075 (review) No, I fixed the issue by returning nullptr, also each use has to check for nullness, I ensured that all do, and when not, I add an Assume to be explicit that we don't expect it to be nullptr.
#34075 (review) Fixed, thanks.
#34075 (review) Fixed, thanks.
#34075 (review) Fixed, thanks.
#34075 (review) Fixed, thanks. I have also split fe047c4 into a separate PR #34552

The windows failure here seems unrelated: https://github.com/bitcoin/bitcoin/actions/runs/21875404684/job/63142731521?pr=34075. Should it be retriggered (@hebasto @willcl-ark)?

DrahtBot added the label CI failed on Feb 10, 2026

DrahtBot removed the label Needs rebase on Feb 10, 2026

DrahtBot closed this on Feb 12, 2026

DrahtBot reopened this on Feb 12, 2026

DrahtBot removed the label CI failed on Feb 12, 2026

sedited referenced this in commit a7c29df0e5 on Feb 17, 2026

DrahtBot added the label Needs rebase on Feb 17, 2026

ismaelsadeeq force-pushed on Feb 18, 2026

DrahtBot removed the label Needs rebase on Feb 18, 2026

DrahtBot added the label CI failed on Feb 18, 2026

DrahtBot added the label Needs rebase on Feb 19, 2026

ismaelsadeeq force-pushed on Feb 26, 2026

DrahtBot removed the label Needs rebase on Feb 26, 2026

ismaelsadeeq force-pushed on Feb 26, 2026

DrahtBot removed the label CI failed on Feb 26, 2026

ismaelsadeeq commented at 11:50 AM on March 5, 2026: member

See bitcoincorefeerate.com/stat for a recent running benchmark that measures the accuracy of the fee rate estimates being returned by this PR

sedited commented at 8:18 AM on March 14, 2026: contributor

Concept ACK

People have been discussing and asking for this for a long time already, cool to see this actually implemented!

for a recent running benchmark that measures the accuracy of the fee rate estimates being returned by this PR

Do you already have any data on how this accuracy compares to some of the external services, like bitcoin augur or mempool.space?

ismaelsadeeq commented at 7:31 AM on March 16, 2026: member

Do you already have any data on how this accuracy compares to some of the external services, like bitcoin augur or mempool.space?

Previous benchmarks were done relative to our current fee rate estimator (Block Policy). This work aims to improve Block Policy fee rate estimator by reducing overestimation when the mempool is empty.

mempool.space is fully mempool-based and can be more accurate than this during high mempool activity. We choose to be conservative during high mempool activity and use a block policy estimator to avoid being susceptible to mempool games.

Block policy estimator has another advantage that mempool.space does not have, which is fee rate estimates up to 1008 blocks with a high probability of success within that range, for people who want to pay lower fees at the cost of waiting, and this is actively used by lightning implementations e.g LND budget sweeper.

Bitcoin Augur was benchmarked by the Block engineers, but against a stale version of Core v27.2, and significant changes were made to our fee estimator after that version, so it is not a good representation. Their result is: https://delvingbitcoin.org/t/augur-block-s-open-source-bitcoin-fee-estimation-library/1848

Provider	Miss Rate	Avg Overestimate	Total Difference
Augur	14.1%	15.9%	13.6%
Blockstream (Core v27.2)	18.7%	44.2%	35.9%

The data and tool were open source, and I reproduced these numbers locally (Assuming the data points are correct :)) Overestimation is still a notable concern here.

I think for now, this is a good direction to go. This PR made a series of refactors and introduced a fee estimation manager and fee estimator abstract class that should ideally make it easy to add other strategies when they are studied well enough and have rough conceptual agreement.

Running benchmarks for all available fee estimators can be beneficial. I have an issue on bitcoin-core-fees

DrahtBot added the label Needs rebase on Mar 19, 2026

ismaelsadeeq force-pushed on Mar 19, 2026

DrahtBot removed the label Needs rebase on Mar 19, 2026

DrahtBot added the label Needs rebase on Mar 19, 2026

ismaelsadeeq force-pushed on Apr 21, 2026

DrahtBot added the label CI failed on Apr 21, 2026

DrahtBot commented at 9:12 PM on April 21, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/24742042211/job/72384009407 LLM reason (✨ experimental): CI failed because IWYU detected missing/incorrect include dependencies and forced a failure (“Failure generated from IWYU”).

<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 Apr 22, 2026

DrahtBot removed the label CI failed on Apr 22, 2026

DrahtBot removed the label Needs rebase on Apr 23, 2026

ismaelsadeeq force-pushed on Apr 26, 2026

DrahtBot added the label CI failed on Apr 26, 2026

DrahtBot commented at 1:35 PM on April 26, 2026: contributor

🚧 At least one of the CI tasks failed. Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/24956651699/job/73076006747 LLM reason (✨ experimental): CI failed due to a C++ build error in the fuzz target: -Werror=overloaded-virtual reports FeeRateEstimatorManager::GetFeeRateEstimate(...) is hidden by an override, causing compilation to stop.

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

DrahtBot removed the label CI failed on Apr 26, 2026

ismaelsadeeq commented at 5:53 PM on April 26, 2026: member

I simplified this a lot in the recent push.

I squashed the commits.
I removed the estimator abstraction (seems to be overkill and not giving us a major gain, honestly, because the manager selection still has to cast to get a specific estimator dynamically; saving a unique pointer to the estimators in the manager is more ideal), but still retained the manager.
I added an option to have block_policy_only mode. I also made block policy only mode the default now, and the wallet does not use this feature yet. The node interface exposes the block policy estimates, but via the estimator. I am thinking that after about one RC, we can make this the default. Curious to know what other people think. The change is a two-liner and a few test changes.

in src/policy/fees/mempool_estimator.cpp:87 in a9df2054c9 outdated

  82 | +            return false;
  83 | +        }
  84 | +        // VectorFormatter requires reserve(), which std::deque lacks; use a vector as intermediate.
  85 | +        std::vector<MinedBlockStats> v;
  86 | +        file >> Using<VectorFormatter<EncodedBlockDataFormatter>>(v);
  87 | +        prev_mined_blocks.assign(v.begin(), v.end());

willcl-ark commented at 11:06 AM on April 28, 2026:

In a9df2054c9057450fb83bb054a459d617218a438

Should this bound v to NUMBER_OF_BLOCKS before assigning?

MempoolTxsRemovedForBlock() keeps the deque bounded only when prev_mined_blocks.size() == NUMBER_OF_BLOCKS; if a persisted file contains more entries than that, the deque stays oversized and future block additions won’t trim it. isMempoolHealthy() would then evaluate more than the intended recent 6 blocks.

Could truncate to the last NUMBER_OF_BLOCKS entries here, or reject oversized input?

in src/rpc/fees.cpp:101 in 90f09bc239

 101 |              }
 102 | +
 103 | +            for (auto& error : estimate.errors) {
 104 | +                errors.push_back(error);
 105 | +            }
 106 | +            result.pushKV("errors", std::move(errors));

willcl-ark commented at 11:10 AM on April 28, 2026:

In 90f09bc239c091fefe27400a3374fcc3d19a8f96

I think we should update RPCResult here too then, currently it is:

{RPCResult::Type::ARR, "errors", /*optional=*/true,

but now we will always return an errors field.

in test/functional/feature_fee_estimation.py:406 in a9df2054c9

 399 | @@ -385,12 +400,19 @@ def test_estimate_dat_is_flushed_periodically(self):
 400 |          assert_not_equal(block_policy_fee_dat_current_content, block_policy_dat_initial_content)
 401 |          block_policy_dat_initial_content = block_policy_fee_dat_current_content
 402 |  
 403 | +        mempool_policy_fee_dat_current_content = open(mempool_policy_fee_dat, "rb").read()
 404 | +        assert_not_equal(mempool_policy_fee_dat_current_content, mempool_policy_dat_initial_content)
 405 | +
 406 | +        mempool_policy_fee_dat_current_content = mempool_policy_fee_dat_current_content

willcl-ark commented at 11:26 AM on April 28, 2026:

In a9df2054c9057450fb83bb054a459d617218a438:

Is this meant to update mempool_policy_dat_initial_content?

As written this self-assignment is a no-op, so the later restart assertion still compares against the original mempool estimator file contents rather than the contents after the latest flush.

willcl-ark changes_requested

willcl-ark commented at 11:28 AM on April 28, 2026: member

looking pretty nice here now :)

I will do some more testing, and leave this running on a box so I can compare more estimates myself.

A few ~ nits/questions I had re-reading through this this morning below.

ismaelsadeeq force-pushed on Apr 30, 2026

DrahtBot added the label CI failed on Apr 30, 2026

ismaelsadeeq commented at 12:00 PM on April 30, 2026: member

Forced pushed from 858bcd6f2b360c6b02aa41a5633967e122d08da5 to 632904f3c65dd745830c3536e277fe2723a2b774 compare diff

#34075 (review) Good idea, I take the suggestion of halting when the vector size exceeds the maximum.
#34075 (review) Fixed, but differently, I modified only to return an error when it exists, not all the time, even on an empty error. So no need to adjust the help string now.
#34075 (review) Good catch fixed here.

I will do some more testing, and leave this running on a box so I can compare more estimates myself.

Nice, happy to see how it compares to https://bitcoincorefeerate.com/stats

DrahtBot removed the label CI failed on Apr 30, 2026

davidgumberg commented at 12:23 AM on May 12, 2026: contributor

Concept ACK,

From looking at the metrics in your description and on https://bitcoincorefeerate.com/stats this approach is much more accurate than the current fee estimation in Bitcoin Core, and as pointed out, erring on the side of underestimation is reasonable given that transactions can be RBF'ed. I will review this soon.

DrahtBot added the label Needs rebase on May 20, 2026

ismaelsadeeq force-pushed on May 27, 2026

DrahtBot added the label CI failed on May 27, 2026

DrahtBot commented at 2:57 PM on May 27, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/26514463790/job/78087585423 LLM reason (✨ experimental): (empty)

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

DrahtBot removed the label Needs rebase on May 27, 2026

ismaelsadeeq force-pushed on May 28, 2026

willcl-ark commented at 2:07 PM on May 28, 2026: member

Thanks for rebasing, I'd like to see this get in!

One major thing first, and I could be forgetting something already-discussed; if so sorry.

It looks like we hardcode the mempool-based estimator to 2 blocks (MEMPOOL_FEE_ESTIMATOR_MAX_TARGET) in alll cases. I had been somewhat working (in my head) under the assumption that was the maximum, but that we could also do 1 block too. Is this incorrect?

IIUC current branch will do:

https://github.com/bitcoin/bitcoin/blob/ff06a9ded6e6bde8f5c0359e9b8e0b09133e462b/src/policy/fees/estimator_man.cpp#L35-L37

which, if called foir a one block estimate, seems likely to return a (lower) 2 block, mempool-based estimate. Which might not be desirable?

ismaelsadeeq force-pushed on May 28, 2026

ismaelsadeeq commented at 3:43 PM on May 28, 2026: member

but that we could also do 1 block too. Is this incorrect?

It will do one.

which, if called for a one block estimate, seems likely to return a (lower) 2 block, mempool-based estimate. Which might not be desirable?

In the block policy and mempool estimator, 1/2 block targets are basically identical. The current approach is to not enforce a maximum in the mempool estimator, it is explicit from the class docstring that that's the only target the mempool estimator will serve (2).

The MEMPOOL_FEE_ESTIMATOR_MAX_TARGET identical to the block policy estimator's "blocks" returned output, which basically implies the target the estimate was meant for. All 1-conf target inputs are collapsed to 2. For more than 2 we can get lower in the case of the block policy estimator; for mempool we always get 2.

DrahtBot removed the label CI failed on May 28, 2026

willcl-ark commented at 11:15 AM on June 1, 2026: member

but that we could also do 1 block too. Is this incorrect?

It will do one.

Well, if I'm not mistaken it will return "an estimate" when you call bitcoin-cli estimatesmartfee 1 ... block_policy_only=false, which I think it what you're saying. But it will be a 2 block estimate, right?

All 1-conf target inputs are collapsed to 2.

Would we not want to have true 1 block mempool-based estimates to reduce overestimation when e.g. there has been congestion for a while, but the mempool is clearing rapidly (or has cleared)? Perhaps 2 blocks is just as good here...

in src/wallet/rpc/spend.cpp:193 in 1c2b0ea77b

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

willcl-ark commented at 1:55 PM on June 16, 2026:

In 1c2b0ea77bbc557e0d3c11ce20c1936904d499e9

These FIXME's feel a little unfortunate. They are fixed later from what I can see, but they are a bit hacky too. Is there no way to preserve "fee_reason" until "fee_source" replaces it?

in src/rpc/fees.cpp:50 in 9a3937920e outdated

  47 | @@ -48,6 +48,7 @@ static RPCMethod estimatesmartfee()
  48 |              RPCResult::Type::OBJ, "", "",
  49 |              {
  50 |                  {RPCResult::Type::NUM, "feerate", /*optional=*/true, "estimate fee rate in " + CURRENCY_UNIT + "/kvB (only present if no errors were encountered)"},

willcl-ark commented at 2:00 PM on June 16, 2026:

In 9a3937920eb7c41d57123633238c1cbfd850c171

in estimatesmartfee the RPC help says feerate will only be present if no errors were encountered. I think after this change it's not quite true; we can both return a feerate from say block policy estimator, and errors from mempool-based estimator. Could fix by updating the help text.

Could even add a functional test where block policy has a valid estimate, mempool health is false, and block_policy_only=false still returns a feerate with/without an errors key (depending on how this was resolved)

in src/policy/fees/mempool_estimator.cpp:90 in 42afc37dae outdated

  85 | +        std::vector<MinedBlockStats> v;
  86 | +        file >> Using<VectorFormatter<EncodedBlockDataFormatter>>(v);
  87 | +        if (v.size() > NUMBER_OF_BLOCKS) {
  88 | +            LogWarning("%s: Number of previously mined blocks read from disk exceeds the maximum of %s; continuing anyway",
  89 | +                       FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), NUMBER_OF_BLOCKS);
  90 | +        }

willcl-ark commented at 2:07 PM on June 16, 2026:

In 42afc37daec97e4b94a7cf9fe209acfbd3fd2caf

A more general question here; how do we handle restarts? I wonder if we should wait for the mempool to be loaded by calling CTxMemPool::GetLoadTried()

ismaelsadeeq commented at 1:01 PM on June 17, 2026:

After a restart, the fee estimator locks the mutex to read the previous stats from disk and load them into the deque.

What would waiting for GetLoadTried prevent here? The only case where it seems useful is when a fee estimate is requested while the mempool is still loading. The most common won't have enough transactions for an accurate estimate anyway.

The estimate could be temporarily lower if only low-fee transactions have been loaded and the high-fee ones haven't yet. This appears to be a block assembler issue; it shouldn't build a template while the mempool is still loading. So I left it as is?

ismaelsadeeq commented at 11:12 PM on June 29, 2026:

This is fixed now, thanks @willcl-ark

in src/wallet/rpc/spend.cpp:194 in c3e1aa838c outdated

 190 | @@ -190,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", ""); // FIXME Return fee_source
 195 | +        entry.pushKV("fee_reason", StringForFeeSource(res->fee_source));

willcl-ark commented at 2:10 PM on June 16, 2026:

In c3e1aa838c83f850d789f92b5731da0f996c4294

Does this mean we lose our (nice?) details like: "Target 85% Threshold"or"Conservative Double Target longer horizon" and will only get "Fee Rate Estimator" (or Block Policy) out now?

ismaelsadeeq commented at 1:03 PM on June 17, 2026:

Yeah, that's a motivation for the refactor in the top commits, because these details are implementation-specific to the block policy fee rate estimator; estimatesmartfee. The mempool fee rate estimator does not have it. This information is logged instead in the estimatesmartfee method.

willcl-ark commented at 2:11 PM on June 16, 2026: member

Taken another look at this today, left a few more comments. Slightly nitty perhaps, but IMO most of them are certainly nice-to-haves.

ismaelsadeeq force-pushed on Jun 17, 2026

ismaelsadeeq commented at 12:38 PM on June 17, 2026: member

Forced pushed from 0f6fa3fc339b319f117bd6c0fc523bf1b296123a to d5831b26523ff80a71bf5882c5ac8b874b1c07a3 compare diff

#34075 (review) The fixme is gone now.
#34075 (review) Thanks for noticing. I updated to not return a fee rate estimate when an error is encountered in any of the fee rate estimators.

in src/wallet/rpc/spend.cpp:271 in eac8da05a8

 267 | @@ -268,7 +268,7 @@ RPCMethod sendtoaddress()
 268 |                          RPCResult::Type::OBJ, "", "",
 269 |                          {
 270 |                              {RPCResult::Type::STR_HEX, "txid", "The transaction id."},
 271 | -                            {RPCResult::Type::STR, "fee_reason", "The transaction fee reason."}
 272 | +                            {RPCResult::Type::STR, "fee_reason", "(DEPRECATED) The transaction fee source. (The key is not renamed to fee_source to maintain backward compatibility during the deprecation period)"}

polespinasa commented at 9:48 AM on June 25, 2026:

in eac8da05a8ade7171dc2c880c5e37dea58369bfb wallet: limit FeeCalculation struct usage to wallet/fees

This would probably need a release note.

in src/common/messages.cpp:25 in 380b40cca2

  39 | -    if (reason_string == fee_reason_strings.end()) return "Unknown";
  40 | -
  41 | -    return reason_string->second;
  42 | -}
  43 | -
  44 |  std::string StringForFeeSource(FeeSource source)

polespinasa commented at 9:55 AM on June 25, 2026:

in 380b40cca2f0f68fee0a4f9cbc59b3c6b8c5e2d8 fees: move StringForFeeReason to block policy estimator

Looks a bit weird to have StringForFeeReason and StringForFeeSource in different files. I think the helpers should be in the same place.

in src/rpc/fees.cpp:45 in 23d0e3e247

  41 | @@ -42,6 +42,7 @@ static RPCMethod estimatesmartfee()
  42 |              {"conf_target", RPCArg::Type::NUM, RPCArg::Optional::NO, "Confirmation target in blocks (1 - 1008)"},
  43 |              {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"economical"}, "The fee estimate mode.\n"
  44 |                + FeeModesDetail(std::string("default mode will be used"))},
  45 | +            {"block_policy_only", RPCArg::Type::BOOL, RPCArg::Default{true}, "When true (default), use only the block policy estimator."},

polespinasa commented at 10:12 AM on June 25, 2026:

in 23d0e3e24780a065cec3bc9f688318174195dae4 rpc: add block_policy_only param to estimatesmartfee

Probably I missed some discussion from the previous comments (there are a lot :) ) but do we want the default to be true? Imho the default should be false and users who are concerned about it should go and enable the block policy only behavior.

polespinasa commented at 10:13 AM on June 25, 2026:

in 23d0e3e24780a065cec3bc9f688318174195dae4 rpc: add block_policy_only param to estimatesmartfee

This new argument needs a release note

in src/wallet/rpc/spend.cpp:378 in eac8da05a8

 374 | @@ -375,7 +375,7 @@ RPCMethod sendmany()
 375 |                          {
 376 |                              {RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
 377 |                  "the number of addresses."},
 378 | -                            {RPCResult::Type::STR, "fee_reason", "The transaction fee reason."}
 379 | +                            {RPCResult::Type::STR, "fee_reason", "(DEPRECATED) The transaction fee source. (The key is not renamed to fee_source to maintain backward compatibility during the deprecation period)"}

polespinasa commented at 10:39 AM on June 25, 2026:

in eac8da0 wallet: limit FeeCalculation struct usage to wallet/fees

This would probably need a release note.

in src/policy/fees/estimator_man.cpp:39 in c1f2543342

  37 | +                 selected_estimate.returned_target,
  38 | +                 CFeeRate(selected_estimate.feerate.fee, selected_estimate.feerate.size).GetFeePerK(),
  39 | +                 CURRENCY_ATOM);
  40 | +        return selected_estimate;
  41 | +    }
  42 | +    return mempool_estimate;

polespinasa commented at 10:51 AM on June 25, 2026:

in c1f254334230b47d266eb33474f8f9828da834e9 fees: return mempool estimates when it's lower than block policy

Shouldn't this be:

    return block_policy_estimate;

If we cannot use mempool estimations fallback to block estimations.

Probably the whole function could be simplified to:

$ git diff
diff --git a/src/policy/fees/estimator_man.cpp b/src/policy/fees/estimator_man.cpp
index 0159aef7ee..d5be7f6acd 100644
--- a/src/policy/fees/estimator_man.cpp
+++ b/src/policy/fees/estimator_man.cpp
@@ -23,20 +23,18 @@ FeeRateEstimatorManager::FeeRateEstimatorManager(const fs::path& block_policy_pa
 FeeRateEstimatorResult FeeRateEstimatorManager::GetFeeRateEstimate(int target, bool conservative) const
 {
     auto block_policy_estimate = m_block_policy_estimator->EstimateFeeRate(target, conservative);
-    if (block_policy_estimate.feerate.IsEmpty()) {
-        return block_policy_estimate;
-    }
     auto mempool_estimate = m_mempool_estimator->EstimateFeeRate(conservative);
-    if (!mempool_estimate.feerate.IsEmpty()) {
-        auto selected_estimate = std::min(block_policy_estimate, mempool_estimate);
-        LogDebug(BCLog::ESTIMATEFEE, "Fee rate estimated using %s: for target %s, fee rate %s %s/kvB.\n",
-                 FeeRateEstimatorTypeToString(selected_estimate.feerate_estimator),
-                 selected_estimate.returned_target,
-                 CFeeRate(selected_estimate.feerate.fee, selected_estimate.feerate.size).GetFeePerK(),
-                 CURRENCY_ATOM);
-        return selected_estimate;
+    if (block_policy_estimate.feerate.IsEmpty() || mempool_estimate.feerate.IsEmpty()) {
+        return block_policy_estimate;
     }
-    return mempool_estimate;
+
+    auto selected_estimate = std::min(block_policy_estimate, mempool_estimate);
+    LogDebug(BCLog::ESTIMATEFEE, "Fee rate estimated using %s: for target %s, fee rate %s %s/kvB.\n",
+             FeeRateEstimatorTypeToString(selected_estimate.feerate_estimator),
+             selected_estimate.returned_target,
+             CFeeRate(selected_estimate.feerate.fee, selected_estimate.feerate.size).GetFeePerK(),
+             CURRENCY_ATOM);
+    return selected_estimate;
 }

in src/rpc/fees.cpp:97 in 93973d442f

  92 | @@ -93,7 +93,8 @@ static RPCMethod estimatesmartfee()
  93 |                  CFeeRate min_relay_feerate{mempool.m_opts.min_relay_feerate};
  94 |                  fee_rate = std::max({fee_rate, min_mempool_feerate, min_relay_feerate});
  95 |                  result.pushKV("feerate", ValueFromAmount(fee_rate.GetFeePerK()));
  96 | -            } else {
  97 | +            }
  98 | +            if (!estimate.errors.empty()) {

polespinasa commented at 10:58 AM on June 25, 2026:

in 93973d442febf868e03f0030622385aa2e7a5448 rpc:fees: return errors in estimatesmartfee when they exist

Just a question, we should never return a feerate lower than the min relay fee. Shouldn't a feerate = 0 mean there is an error, thus making the if unnecessary?

in src/policy/fees/mempool_estimator.cpp:66 in 2727b83f17

  61 | +    AssertLockHeld(cs);
  62 | +    if (prev_mined_blocks.size() < NUMBER_OF_BLOCKS) return false;
  63 | +    uint64_t total_block_weight{0};
  64 | +    uint64_t total_removed_weight{0};
  65 | +    for (size_t i = 0; i < prev_mined_blocks.size(); ++i) {
  66 | +        // Handle reorg by returning false (Dont clear as blocks are added it will stabilize).

polespinasa commented at 11:10 AM on June 25, 2026:

in 2727b83f1750a4407ca00ea27b19ce7157ba2ea1 fees: only return mempool fee rate estimate when mempool is healthy

nit: dont -> don't

in src/rpc/fees.cpp:46 in be70396de3

  42 | @@ -42,6 +43,7 @@ static RPCMethod estimatesmartfee()
  43 |              {"conf_target", RPCArg::Type::NUM, RPCArg::Optional::NO, "Confirmation target in blocks (1 - 1008)"},
  44 |              {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"economical"}, "The fee estimate mode.\n"
  45 |                + FeeModesDetail(std::string("default mode will be used"))},
  46 | +            {"verbosity", RPCArg::Type::NUM, RPCArg::Default{1}, "1 returns feerate and errors. 2 also returns mempool health statistics for recently mined blocks (only when block_policy_only is false)."},

polespinasa commented at 11:18 AM on June 25, 2026:

in be70396de3f7eb85f7df4dc01c4e19e6ca1e59c0 rpc: add verbosity param to estimatesmartfee to expose mempool coverage stats

Needs release note

in src/policy/fees/estimator_args.cpp:11 in d294e0b637

   6 | +
   7 | +#include <common/args.h>
   8 | +
   9 | +namespace {
  10 | +const char* FEES_BASE_DIR = "fees";
  11 | +const char* BLOCK_POLICY_ESTIMATES_FILENAME = "block_policy_estimates.dat";

polespinasa commented at 11:51 AM on June 25, 2026:

in d294e0b637c8713b121eaf127bed31d37546ddd1 fees: rename and move fee_estimates.dat to fees/block_policy_estimates.dat nit:

constexpr const char* for both FEES_BASE_DIR and BLOCK_POLICY_ESTIMATES_FILENAME?

in src/policy/fees/mempool_estimator.cpp:93 in e1fbab3e07

  88 | +            LogWarning("%s: Number of previously mined blocks read from disk exceeds the maximum of %s; continuing anyway",
  89 | +                       FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), NUMBER_OF_BLOCKS);
  90 | +        }
  91 | +        prev_mined_blocks.assign(v.begin(), v.end());
  92 | +    } catch (std::exception&) {
  93 | +        LogWarning("%s: Unable to read data to %s (non-fatal)", FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), fs::PathToString(m_mempool_estimates_file_path));

polespinasa commented at 11:55 AM on June 25, 2026:

in e1fbab3e079086dbd9afbde0104e4115968d1f47 fees: persist mempool estimator block stats across restarts

nit:

        LogWarning("%s: Unable to read data from %s (non-fatal)", FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), fs::PathToString(m_mempool_estimates_file_path));

in src/policy/fees/mempool_estimator.cpp:107 in e1fbab3e07

 102 | +        fs::create_directories(m_mempool_estimates_file_path.parent_path());
 103 | +    }
 104 | +    AutoFile file{fsbridge::fopen(m_mempool_estimates_file_path, "wb")};
 105 | +    if (file.IsNull()) {
 106 | +        LogDebug(BCLog::ESTIMATEFEE, " %s: %s does not exist. Continue anyway", FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), fs::PathToString(m_mempool_estimates_file_path));
 107 | +        (void)file.fclose();

polespinasa commented at 11:56 AM on June 25, 2026:

in e1fbab3 fees: persist mempool estimator block stats across restarts

I think this line is redundant. If file.IsNull() the file was never opened, so there is no need to close it.

in src/policy/fees/mempool_estimator.cpp:106 in e1fbab3e07

 101 | +    if (!m_mempool_estimates_file_path.parent_path().empty()) {
 102 | +        fs::create_directories(m_mempool_estimates_file_path.parent_path());
 103 | +    }
 104 | +    AutoFile file{fsbridge::fopen(m_mempool_estimates_file_path, "wb")};
 105 | +    if (file.IsNull()) {
 106 | +        LogDebug(BCLog::ESTIMATEFEE, " %s: %s does not exist. Continue anyway", FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), fs::PathToString(m_mempool_estimates_file_path));

polespinasa commented at 11:57 AM on June 25, 2026:

in e1fbab3 fees: persist mempool estimator block stats across restarts

nit: there is an empty-space before the first %s. Probably should be removed?

in src/policy/fees/mempool_estimator.cpp:92 in e1fbab3e07

  87 | +        if (v.size() > NUMBER_OF_BLOCKS) {
  88 | +            LogWarning("%s: Number of previously mined blocks read from disk exceeds the maximum of %s; continuing anyway",
  89 | +                       FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY), NUMBER_OF_BLOCKS);
  90 | +        }
  91 | +        prev_mined_blocks.assign(v.begin(), v.end());
  92 | +    } catch (std::exception&) {

polespinasa commented at 11:58 AM on June 25, 2026:

in e1fbab3 fees: persist mempool estimator block stats across restarts

nit: const std::exception&

in src/policy/fees/mempool_estimator.h:80 in e1fbab3e07

  74 | @@ -73,12 +75,9 @@ class MemPoolFeeRateEstimatorCache
  75 |  class MemPoolFeeRateEstimator
  76 |  {
  77 |  public:
  78 | -    MemPoolFeeRateEstimator(const CTxMemPool* mempool, ChainstateManager* chainman)
  79 | -        : m_mempool(mempool), m_chainman(chainman)
  80 | -    {
  81 | -        assert(m_mempool);
  82 | -        assert(m_chainman);

polespinasa commented at 12:00 PM on June 25, 2026:

in e1fbab3 fees: persist mempool estimator block stats across restarts

nit: These two guards were removed. Not sure if this is intended. Maybe worth keeping them.

in src/test/fuzz/policy_estimator_io.cpp:35 in d5831b2652

  33 |      FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider};
  34 | -    AutoFile fuzzed_auto_file{fuzzed_file_provider.open()};
  35 | -    // Reusing block_policy_estimator across runs to avoid costly creation of CBlockPolicyEstimator object.
  36 | +    AutoFile fuzzed_auto_file_block_policy{fuzzed_file_provider.open()};
  37 | +    AutoFile fuzzed_auto_file_mempool_policy{fuzzed_file_provider.open()};
  38 | +    // Reusing block_policy_estimator and mempool_policy_estimator across runs to avoid costly creation of CBlockPolicyEstimator object.

polespinasa commented at 12:02 PM on June 25, 2026:

in d5831b26523ff80a71bf5882c5ac8b874b1c07a3 test: add mempool estimator i/o fuzz test

nit:

    // Reusing block_policy_estimator and mempool_policy_estimator across runs to avoid costly creation of CBlockPolicyEstimator and MemPoolFeeRateEstimator objects.

in test/functional/feature_fee_estimation.py:149 in d5831b2652

 144 | @@ -139,6 +145,10 @@ def check_fee_estimates_btw_modes(node, expected_conservative, expected_economic
 145 |      assert_equal(fee_est_economical, expected_economical)
 146 |      assert_equal(fee_est_default, expected_economical)
 147 |  
 148 | +def verify_estimate_response(estimate, feerate, errors):
 149 | +    if feerate:

polespinasa commented at 12:03 PM on June 25, 2026:

This is falsy for feerate = 0

Should be if feerate is not None

polespinasa commented at 12:04 PM on June 25, 2026: member

concept ACK

maybe can squash 2af9529879c4c219e91a94fe9c3c9ea6c1cf2f97 and 77b18e8143281c8ef0d219f1b22b587a8ea36d02 They are pretty straight forward to review.

I think ecf85f5ed96f974f1ce1fed9264cdb7f786a3554 (refactor: test: make blockpolicyestimator_tests independent of mempool) needs a commit message. A bit of motivation on the change would be good.

Kino1994 referenced this in commit acb674f49c on Jun 28, 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

refactor: test: make block policy estimator tests files to be specific

This commit rename policyestimator_tests to blockpolicyestimator_tests.

It also rename policy estimator to block policy estimator

52fc0e4365

refactor: test: make blockpolicyestimator_tests independent of mempool b2bbe3a5bd

ismaelsadeeq force-pushed on Jun 29, 2026

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

Forced pushed d5831b2652...5a0048bcb

#34075 (review), #34075 (review), #34075 (review), #34075 (review), I added a release note
#34075 (review), why? This changed a bit now to show that they are completely unrelated fields.
#34075 (review), I can't remember why, too, done. It is now the default.
#34075 (review), Taken
#34075 (review) Fixed.
#34075 (review) Fixed now.
#34075 (review), Fixed thanks
#34075 (review), Fixed
#34075 (review), Fixed
#34075 (review), Fixed
#34075 (review) Fixed
#34075 (review) Fixed
#34075 (review) Fixed thanks.
#34075 (review)

Thanks for thorough review @polespinasa gracias :)

I addition to fixing your review comments I made a series of changes to improve the PR and make review easy. I squashed commits and removed the fix-me.

<details> <summary>Details of changes.</summary> - The `FeeSource` direction was dropped. The new push instead splits the overloaded fee reason enum into: wallet `FeeReason`, for why the wallet selected a fee rate and`BlockPolicyEstimateReason`, for Block Policy Estimator internal threshold details. - The wallet RPC field remains named `fee_reason`, but now reports wallet fee reason without Block Policy Estimator internals (this is explicitly mentioned).

block_policy_estimator_args.{cpp,h} were removed; fee estimator path helpers now live in estimator_args.{cpp,h}.
Block policy data is stored in fees/block_policy_estimates.dat, with legacy fee_estimates.dat migrated at startup.
- formatter renamed from EncodedBlockDataFormatter to MinedBlockStatsFormatter
- directory creation uses non-throwing fs::create_directories(..., std::error_code)
- close failure logs with LogWarning, not LogError
- oversized persisted mined-block stats are rejected by returning false
- read/write log messages were clarified
Tests now cover missing estimator directories for both block policy and mempool policy estimator files.
Functional cache handling no longer preserves fees/, so estimator files do not leak across cached test chain reuse.
estimatesmartfee uses an options object.
block_policy_only is an option in that object, defaulting to false.
verbosity is also in the options object.
The result includes estimator when block_policy_only=false.
The mempool health statistics fields now does not include the ratio field.
Avoided two simultaneously open AutoFiles from the same FuzzedFileProvider; the new version opens, uses, and closes the block-policy file before opening the mempool-policy file.
The PR now adds a release notes

</detials>

DrahtBot added the label CI failed on Jun 30, 2026

DrahtBot commented at 12:26 AM on June 30, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/28408330944/job/84175694812 LLM reason (✨ experimental): CI failed because the IWYU (include-what-you-use) check detected and rejected incorrect headers (triggering “Failure generated from IWYU”).

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

fees: add EstimateFeeRate and MaximumTarget to CBlockPolicyEstimator

FeeRateEstimatorType identifies the source estimator in a result.

FeeRateEstimatorResult carries the feerate, target, and any
error messages returned by a fee rate estimator.

Add EstimateFeeRate method that wraps estimateSmartFee and returns
a FeeRateEstimatorResult.

Add MaximumTarget convenience method that delegates to
HighestTargetTracked(LONG_HALFLIFE).

Update call sites in rpc/fees.cpp and node/interfaces.cpp.

Co-authored-by: willcl-ark <will@256k1.dev>

c030b546b0

fees: add FeeRateEstimatorManager class

Introduce FeeRateEstimatorManager to wrap CBlockPolicyEstimator and
act as the single point of contact for fee rate estimation in the node.
It inherits CValidationInterface so it can register directly with the
validation signals and receive mempool/block events.

Wire it into NodeContext (fee_estimator_man), init, shutdown, the RPC
server utility helpers (EnsureAnyFeeEstimatorMan), and the wallet-facing
interfaces::Chain API.

The Chain method estimateSmartFee is renamed to getFeeRateEstimate and
now returns FeeRateEstimatorResult instead of CFeeRate, so callers get
the full estimation context without needing FeeCalculation.
estimateMaxBlocks is renamed to maximumFeeEstimationTargetBlocks (still
returns the max target).

CBlockPolicyEstimator no longer inherits CValidationInterface; the
manager now receives the mempool/block validation events and forwards
them to it.

Co-authored-by: willcl-ark <will@256k1.dev>

c7debc62b9

rpc: add block_policy_only option to estimatesmartfee

Add a boolean block_policy_only option (default false) to
estimatesmartfee. When true, only the block-policy estimator is
consulted. When false, the manager may consult any available estimator;
today that is still only the block-policy estimator, so the result is
unchanged until a second estimator is added.

Also adds GetFeeRateEstimate(FeeRateEstimatorType, target, conservative)
to FeeRateEstimatorManager so callers can query a single estimator by type.

6b3f106d4a

fees: add MemPoolFeeRateEstimator class

Add CalculatePercentiles, which takes a vector of chunk feerates sorted
in descending mining-score order and returns the 50th and 75th
percentile feerates. Returns empty Percentiles if the mempool is too
sparse to fill 95% of a block.

Add MemPoolFeeRateEstimator, which calls Bitcoin Core's block-building
algorithm against the  mempool to produce a feerate estimate.

The 50th-percentile feerate is returned as the conservative estimate and
the 75th-percentile as the economical estimate.

When the mempool is too sparse to reach 95% block coverage, the mempool fee rate
estimator falls back to max(minrelayfee, mempoolminfee).

Add FeeRateEstimatorTypeToString and the MEMPOOL_POLICY enumerator to
FeeRateEstimatorType.

Unit tests are added for both CalculatePercentiles and
MemPoolFeeRateEstimator.

Co-authored-by: willcl-ark <will@256k1.dev>

c7dc430045

ismaelsadeeq force-pushed on Jun 30, 2026

DrahtBot removed the label CI failed on Jun 30, 2026

BigcoinBGC referenced this in commit 58d5a51972 on Jun 30, 2026

in src/policy/fees/mempool_estimator.cpp:64 in cd96fdf16e outdated

  59 | +{
  60 | +    LOCK(cs);
  61 | +    return isMempoolHealthy();
  62 | +}
  63 | +
  64 | +bool MemPoolFeeRateEstimator::isMempoolHealthy() const

willcl-ark commented at 8:47 AM on July 1, 2026:

In cd96fdf16e19c9ce3efc2068926953e401efb314

This does some nice health checks, but, unless I'm mistaken, there's an argument that it misses the most important one; checking that the stuff we are sampling is actually for the current tip.

Might be able to make it work if isMempoolHealthy took an active_tip param, and the stats also stored a blockhash, then we could also check that the stats we are using as health checks are in the active chain.

A cheaper check could be to check only the sample height, but we might trip over on re-orgs of the same height.

ismaelsadeeq commented at 3:31 PM on July 1, 2026:

In most scenarios where you're not at the tip, the sequential block-height check with:

sum(mempool txs removed from block) / sum(block tx weights excluding coinbase) > some desired threshold

should be enough.

When you're syncing, your mempool won't have most of the block's transactions, and the sanity check will prevent any mempool-based fee rate estimate.
When a reorg happens, we lock the mempool until it's completed. This involves disconnecting the blocks, adding the unconfirmed transactions back to the mempool, and then reconnecting the blocks. That process locks cs_main and then enqueues the notification to the validation interface queue, so you get the top block's statistics at the end.

The only potential concern is when the fee estimator call happens before the queued notifications have been processed. I'm not too worried about ensuring all notifications are emitted before we estimate fees, because I assume that in real-world, non-test scenarios, it's unlikely we'd receive a late notification. Even if we do, the consequences are minor:

We don't estimate with the mempool when we should, because we receive a new block where the mempool transaction removals cause the health check to succeed. (This could also happen because your node hasn't seen the block yet, even though it's already been mined. In this case, the block has reached your node but the notification hasn't reached the estimator yet.)
We estimate with the mempool when we shouldn't, because a notification that could cause us to become unhealthy hasn't reached the estimator yet, for the same reason as above. These are edge cases that are unlikely to occur, since we track 6 blocks and then run the sanity check on them.

I think cleaner ways to address this would be:

Have the mempool save this data for us. However, that would defeat the purpose of the decoupling done when separating mempool logic from fee estimation.
Drain notifications before calling fee estimation. We do this in estimatesmartfee, but we can't do it in the wallet because of the potential for a deadlock: the wallet locks cs_wallet, calls the fee estimator, waits for all fee notifications, and then a queued notification tries to lock cs_wallet while it's already locked, causing it to wait forever. This could be fixed by creating a new validation signal-draining call that only drains the notifications we care about and doesn't deadlock, but that's beyond the scope of this and is more involved.
Refactor the wallet code to get the fee rate estimate before locking the wallet mutex.

All of this seems like overkill for this?

in src/policy/fees/estimator_man.cpp:37 in 8a24899b6f outdated

  33 | +    }
  34 | +    auto selected_estimate = std::min(block_policy_estimate, mempool_estimate);
  35 | +    LogDebug(BCLog::ESTIMATEFEE, "Fee rate estimated using %s: target=%s feerate=%s %s/kvB.\n",
  36 | +             FeeRateEstimatorTypeToString(selected_estimate.feerate_estimator),
  37 | +             selected_estimate.returned_target, selected_estimate.feerate.EvaluateFeeDown(1000), CURRENCY_ATOM);
  38 | +    return selected_estimate;

willcl-ark commented at 8:59 AM on July 1, 2026:

In 8a24899b6f719677a0c16802e6e339811e7f0054

GetFeeRateEstimate is used in the wallet by GetDiscardRate() which used to use the longest available block policy target, but now can use a lower (and hopefully more accurate?) 2 block mempool estimation silently.

It feels like on the whole this should be beneficial as we will discard less change, however, in the case that our mempool is inaccurate/has some local policy not actually being refclected by blocks, then we might start not discarding change that we should discard?

cc @achow101 here perhaps.

ismaelsadeeq commented at 9:41 AM on July 1, 2026:

however, in the case that our mempool is inaccurate/has some local policy not actually being refclected by blocks, then we might start not discarding change that we should discard?

This is not an issue because the mempool health sanity check will most likely detect such issues, and we won't return any fee rate estimate from the mempool in those scenarios.

in src/policy/fees/estimator_man.cpp:32 in 8a24899b6f

  28 | +    if (block_policy_estimate.feerate.IsEmpty() || mempool_estimate.feerate.IsEmpty()) {
  29 | +        if (!mempool_estimate.error.empty()) {
  30 | +            LogDebug(BCLog::ESTIMATEFEE, "%s\n", mempool_estimate.error);
  31 | +        }
  32 | +        return block_policy_estimate;
  33 | +    }

willcl-ark commented at 9:15 AM on July 1, 2026:

In 8a24899b6f719677a0c16802e6e339811e7f0054

I think we can avoid wasted mempool estimations by doing something like:

auto block_policy_estimate = m_block_policy_estimator->EstimateFeeRate(target, conservative);
if (block_policy_estimate.feerate.IsEmpty()) return block_policy_estimate;

// Only run the mempool estimator if block_policy is not empty to avoid wasted block building
auto mempool_estimate = m_mempool_estimator->EstimateFeeRate(conservative);
if (mempool_estimate.feerate.IsEmpty()) {
    if (!mempool_estimate.error.empty()) {
        LogDebug(BCLog::ESTIMATEFEE, "%s\n", mempool_estimate.error);
    }
    return block_policy_estimate;
}

return std::min(block_policy_estimate, mempool_estimate);

in src/policy/fees/mempool_estimator.cpp:68 in be6afb81bc outdated

  64 | @@ -36,6 +65,7 @@ FeeRateEstimatorResult MemPoolFeeRateEstimator::EstimateFeeRate(bool conservativ
  65 |          // Mempool is too sparse to produce a fee rate estimate.
  66 |          return {estimator_type, FeePerVSize{0, 0}, MEMPOOL_FEE_ESTIMATOR_MAX_TARGET, {strprintf("%s: Mempool is too sparse for fee rate estimation", FeeRateEstimatorTypeToString(estimator_type))}};
  67 |      }
  68 | +    cache.Update(percentiles.p50, percentiles.p75, blocktemplate->block.hashPrevBlock);

willcl-ark commented at 9:17 AM on July 1, 2026:

In be6afb81bc3223038cc336a93e4f6e24815dcdff

So we only cache if the percentiles are not good/empty. This could mean we coul dbe made to do repeated uncached builds/estimations while this is not the case. I wonder if we could cache sparse results too?

ismaelsadeeq commented at 9:43 AM on July 1, 2026:

That's a good point; we should cache the sparse ones too.

willcl-ark approved

willcl-ark commented at 9:26 AM on July 1, 2026: member

ACK c56f05eb34fad7c4e2d2054232d1c21b535571a9

I think this is the right direction for fee estimation to head in. The current block-policy estimator can overestimate badly in periods where the mempool has ~ cleared, and using current mempool state to lower estimates in those scenarios is just a useful improvement with basically no downsides.

I've re-reviewed the latest version and I think the major earlier concerns have been addressed. Persisted mempool stats are gated on mempool load having been attempted, oversized persisted mempool estimator data is rejected, the legacy fee_estimates.dat migration path is now covered, and the fee_reason RPC behavior is now documented as deprecated / wallet-level.

I still see a few follow-up-worthy areas, left as inline comments, but IMO they don't need to block this PR:

The persisted mempool health data is not tied to the current active chain/tip
GetDiscardRate() now goes through the combined estimator, so wallet change/discard behavior can be influenced by the 2-block mempool estimator rather than only the longest block-policy estimate (probably fine).
FeeRateEstimatorManager::GetFeeRateEstimate() queries the mempool estimator even when the block-policy estimate is empty, even though the mempool result will be discarded in that case. We can easily clean this up to not do that.

DrahtBot requested review from davidgumberg on Jul 1, 2026

DrahtBot requested review from sedited on Jul 1, 2026

DrahtBot requested review from polespinasa on Jul 1, 2026

sedited added this to the milestone 32.0 on Jul 1, 2026

fees: add caching to MemPoolFeeRateEstimator

Cache successful {conservative, economical} feerates as a pair.

Only refresh a successful cached fee rate estimate when the cache is
stale (older than CACHE_LIFE) or the chain tip has changed.

This avoids generating block templates too often.

Co-authored-by: willcl-ark <will@256k1.dev>

9a00cf796d

fees: return mempool estimates when it's lower than block policy

Integrate MemPoolFeeRateEstimator into FeeRateEstimatorManager.
Select the lower of block policy and mempool estimates.

add test that ensures estimatesmartfee returns mempool fee rate estimate

Co-authored-by: willcl-ark <will@256k1.dev>

08add853df

validation: pass connected block txs to mempool removal callbacks

A follow-up change needs access to all transactions in the connected
block, not only the subset removed from the mempool. Pass the full
block transaction list through the existing callback.

5cfa21a884

fees: only return mempool fee rate estimate when mempool is healthy

This ensures that the users mempool has seen at least 75% of their
last 6 blocks transactions.

4456a3dc24

rpc: add verbosity param to estimatesmartfee to expose mempool coverage stats

Add a verbosity parameter (default=1) to estimatesmartfee. When verbosity
>= 2 and block_policy_only=false, the response includes a
mempool_health_statistics array reporting the last NUMBER_OF_BLOCKS mined
blocks in most-recent-first order, each with:

- block_height
- block_weight: total non-coinbase transaction weight in the block
- mempool_txs_weight: weight of transactions removed from our mempool
  to build this block

This exposes the per-block coverage data that the mempool health check
relies on, allowing users to inspect why their node considers the mempool
healthy or unhealthy for fee estimation purposes.

d86e1a5d3e

fees: rename and move fee_estimates.dat to fees/block_policy_estimates.dat 2899aafee3

fees: persist mempool policy estimator data

Persist MemPoolFeeRateEstimator's recent mined-block statistics
to fees/mempool_policy_estimates.dat and reload them at startup.

Without this, the mempool estimator starts cold after each restart
and treats the mempool as unhealthy until NUMBER_OF_BLOCKS blocks
have been observed, causing estimatesmartfee to fall back to the
block policy estimator.

Add MempoolPolicyFeeEstPath(), pass the path through
FeeRateEstimatorManager, and flush both block-policy
and mempool-policy estimator files on interval and shutdown.

33a00e2600

test: add mempool estimator i/o fuzz test 53f457694a

doc: add release notes 23cacb13e7

ismaelsadeeq force-pushed on Jul 1, 2026

ismaelsadeeq commented at 3:31 PM on July 1, 2026: member

Forced pushed from c56f05eb34fad7c4e2d2054232d1c21b535571a9 to 23cacb13e7baf66b75950dd3dd4f595abfb9a38f compare diff

#34075 (review) Fixed.
#34075 (review) Taken, thanks.