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

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 reason this PR uses the mempool only to lower the Block Policy Estimator result. The Block Policy Estimator itself is not gameable in this way, so the combined estimate is not susceptible to this attack increasing the returned feerate.

The underlying assumption is that, with the current tools and work done to make RBF and CPFP feasible and reliable (TRUC transaction relay, ephemeral anchors, cluster size 2 package RBF), underestimation is safer than overestimation. 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 this PR's approach can 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 FeeRateEstimation and FeeRateEstimationError as common estimator result types, avoiding new out-parameters for fee estimation results.
• 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 std::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 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 new estimates are generated at most every 7 seconds while the chain tip is unchanged, 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 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 ratio of mempool transaction weight removed due to block connection to block transaction weight is greater than 75% across the tracked window. 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_estimator.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 fee_rate_estimator to the estimatesmartfee options object. Supported values are "none" (default combined behavior), "block_policy" (use only the Block Policy Estimator), and "mempool_policy" (use only the Mempool Fee Rate Estimator). Unknown values are treated as "none".
• Add verbosity to the estimatesmartfee options object. With verbosity >= 2, the RPC returns recent mempool health statistics.
• Expose the selected fee rate estimator in estimatesmartfee results when fee_rate_estimator is "none" and the estimate succeeds.
• 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, "fee_rate_estimator": "none"}'

{
  "feerate": 0.00002133,
  "estimator": "mempool_policy",
  "blocks": 2,
  "mempool_health_statistics": [
    {
      "block_height": 927953,
      "block_weight": 3991729,
      "mempool_txs_weight": 3942409
    }
  ]
}

</details>

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35900 (iwyu: Fix warnings in src/interfaces and treat them as errors by hebasto)
• #35675 (mining: add block template manager by ismaelsadeeq)
• #35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
• #35474 (node: move index ownership to NodeContext by w0xlt)
• #34803 (mempool: asynchronous mempool fee rate diagram updates via validation interface by ismaelsadeeq)
• #34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
• #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)
• #33117 (Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications by D33r-Gee)
• #32857 (wallet: allow skipping script paths by Sjors)
• #32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
• #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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

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

• broadcast_and_maybe_mine(self.nodes[1], high_feerate, TXS_COUNT, 12, self.nodes[2]) in test/functional/feature_fee_estimation.py
• broadcast_and_maybe_mine(self.nodes[1], low_feerate, TXS_COUNT, 4, self.nodes[2]) in test/functional/feature_fee_estimation.py
• 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
• broadcast_and_maybe_mine(node0, 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-08-20 15:54:05}

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.

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.

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.

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

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

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

<!--85328a0da195eb286784d51f73fa0af9-->

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

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.

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.

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.

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

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

I did a few changes.

<details>

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

<!--85328a0da195eb286784d51f73fa0af9-->

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

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

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

Posting a few simple notes during our pairing session.

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

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

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?

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

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

<!--85328a0da195eb286784d51f73fa0af9-->

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

<!--85328a0da195eb286784d51f73fa0af9-->

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

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.

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.

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

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.

<!--85328a0da195eb286784d51f73fa0af9-->

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

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?

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.

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

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

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.

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.

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>

<!--85328a0da195eb286784d51f73fa0af9-->

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

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.

Forced pushed from c56f05eb34fad7c4e2d2054232d1c21b535571a9 to 23cacb13e7baf66b75950dd3dd4f595abfb9a38f compare diff

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

ACK 23cacb13e7

I think having the mempool estimator is a good new functionality that will help to not overestimate the transaction fees.

I only have a non-blocking suggestion: When the mempool estimator fails for whatever reason, I think we could add a new field in the RPC call to inform why the mempool estimator was not choosen. Currently, we save in the LogDebug the information of why the mempool estimator failed, but even here it can be difficult to see why the mempool is not healthy (not having enough blocks, block reorg, not having the block weight, or a low representation ratio). I think we could return the reason of why the mempool estimator failed or at least add it to the LogDebug.

<details> <summary>I implemented a version of how I would return the reason (probably there is a better way to do this):</summary>

diff --git a/src/policy/fees/estimator_man.cpp b/src/policy/fees/estimator_man.cpp
index f161d6e942..ffe3842f6d 100644
--- a/src/policy/fees/estimator_man.cpp
+++ b/src/policy/fees/estimator_man.cpp
@@ -22,10 +22,15 @@ FeeRateEstimatorManager::FeeRateEstimatorManager(const fs::path& block_policy_pa
 
 FeeRateEstimatorResult FeeRateEstimatorManager::GetFeeRateEstimate(int target, bool conservative) const
 {
+    // Clear reason from previous computation
+    m_mempool_estimator->AddReason(std::nullopt);
     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()) {
+        if (mempool_estimate.reason.has_value()) {
+            m_mempool_estimator->AddReason(mempool_estimate.reason.value());
+        }
         if (!mempool_estimate.error.empty()) {
             LogDebug(BCLog::ESTIMATEFEE, "%s\n", mempool_estimate.error);
         }
@@ -35,6 +40,9 @@ FeeRateEstimatorResult FeeRateEstimatorManager::GetFeeRateEstimate(int target, b
     LogDebug(BCLog::ESTIMATEFEE, "Fee rate estimated using %s: target=%s feerate=%s %s/kvB.\n",
              FeeRateEstimatorTypeToString(selected_estimate.feerate_estimator),
              selected_estimate.returned_target, selected_estimate.feerate.EvaluateFeeDown(1000), CURRENCY_ATOM);
+    if (FeeRateEstimatorTypeToString(selected_estimate.feerate_estimator) == FeeRateEstimatorTypeToString(block_policy_estimate.feerate_estimator)) {
+        m_mempool_estimator->AddReason("Mempool estimator not used: block policy returned a lower fee rate");
+    }
     return selected_estimate;
 }
 
diff --git a/src/policy/fees/mempool_estimator.cpp b/src/policy/fees/mempool_estimator.cpp
index 58505efbe3..3408e2008f 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/src/policy/fees/mempool_estimator.cpp
@@ -158,35 +158,55 @@ void MemPoolFeeRateEstimator::MempoolTxsRemovedForBlock(const std::vector<CTrans
     const uint64_t removed_weight = std::accumulate(txs_removed_for_block.begin(), txs_removed_for_block.end(), uint64_t{0},
                                                     [](uint64_t acc, const RemovedMempoolTransactionInfo& tx) { return acc + static_cast<uint64_t>(GetTransactionWeight(*tx.info.m_tx)); });
     if (prev_mined_blocks.size() == NUMBER_OF_BLOCKS) prev_mined_blocks.pop_front();
+    if (!prev_mined_blocks.empty()) prev_mined_blocks.back().reason.reset();
     prev_mined_blocks.push_back({nBlockHeight, removed_weight, block_weight});
 }
 
+void MemPoolFeeRateEstimator::AddReason(std::optional<std::string> reason) 
+{
+    LOCK(cs);
+    if (prev_mined_blocks.empty()) return;
+    prev_mined_blocks.back().reason = reason;
+}
+
 bool MemPoolFeeRateEstimator::IsMempoolHealthy() const
 {
     LOCK(cs);
-    return isMempoolHealthy();
+    return !isMempoolHealthy().has_value();
 }
 
-bool MemPoolFeeRateEstimator::isMempoolHealthy() const
+std::optional<std::string> MemPoolFeeRateEstimator::isMempoolHealthy() const
 {
     AssertLockHeld(cs);
-    if (prev_mined_blocks.size() < NUMBER_OF_BLOCKS) return false;
+    if (prev_mined_blocks.size() < NUMBER_OF_BLOCKS) {
+        return "Mempool Unhealthy: Not enough blocks";
+    }
     uint64_t total_block_weight{0};
     uint64_t total_removed_weight{0};
     for (size_t i = 0; i < prev_mined_blocks.size(); ++i) {
         // Handle reorg by returning false (don't clear as blocks are added it will stabilize).
-        if (i > 0 && prev_mined_blocks[i].m_height != prev_mined_blocks[i - 1].m_height + 1) return false;
+        if (i > 0 && prev_mined_blocks[i].m_height != prev_mined_blocks[i - 1].m_height + 1) {
+            return "Mempool Unhealthy: Reorg found";
+        }
         total_block_weight += prev_mined_blocks[i].m_block_weight;
         total_removed_weight += prev_mined_blocks[i].m_removed_block_txs_weight;
     }
-    if (!total_block_weight) return false;
+    if (!total_block_weight) {
+        return "Mempool Unhealthy: Zero block weight";
+    }
     const double representation_ratio = static_cast<double>(total_removed_weight) / total_block_weight;
     LogDebug(BCLog::ESTIMATEFEE, "%s: mempool health avg=%.2f threshold=%.2f (mempool %s)",
              FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY),
              representation_ratio,
              MEMPOOL_REPRESENTATION_THRESHOLD,
              representation_ratio >= MEMPOOL_REPRESENTATION_THRESHOLD ? "healthy" : "unhealthy");
-    return representation_ratio >= MEMPOOL_REPRESENTATION_THRESHOLD;
+    if (representation_ratio < MEMPOOL_REPRESENTATION_THRESHOLD) {
+        return "Mempool Unhealthy: Low representation ratio (" 
+        + std::to_string(representation_ratio) + " < threshold " 
+        + std::to_string(MEMPOOL_REPRESENTATION_THRESHOLD) + ")";
+    } else {
+        return std::nullopt;
+    }
 }
 
 FeeRateEstimatorResult MemPoolFeeRateEstimator::EstimateFeeRate(bool conservative) const
@@ -196,8 +216,9 @@ FeeRateEstimatorResult MemPoolFeeRateEstimator::EstimateFeeRate(bool conservativ
     if (!m_mempool->GetLoadTried()) {
         return {estimator_type, FeePerVSize{0, 0}, MEMPOOL_FEE_ESTIMATOR_MAX_TARGET, {strprintf("%s: Mempool not loaded yet, no fee rate estimate available", FeeRateEstimatorTypeToString(estimator_type))}};
     }
-    if (!isMempoolHealthy()) {
-        return {estimator_type, FeePerVSize{0, 0}, MEMPOOL_FEE_ESTIMATOR_MAX_TARGET, {strprintf("%s: Mempool is unreliable for fee rate estimation", FeeRateEstimatorTypeToString(estimator_type))}};
+    std::optional<std::string> healthy = isMempoolHealthy();
+    if (healthy.has_value()) {
+        return {estimator_type, FeePerVSize{0, 0}, MEMPOOL_FEE_ESTIMATOR_MAX_TARGET, {strprintf("%s: Mempool is unreliable for fee rate estimation", FeeRateEstimatorTypeToString(estimator_type))}, healthy.value()};
     }
     Chainstate& chainstate = WITH_LOCK(::cs_main, return m_chainman->CurrentChainstate());
     const uint256 tip_hash{WITH_LOCK(::cs_main, return Assume(chainstate.m_chain.Tip())->GetBlockHash())};
@@ -217,7 +238,7 @@ FeeRateEstimatorResult MemPoolFeeRateEstimator::EstimateFeeRate(bool conservativ
         // Mempool is too sparse to produce a fee rate estimate.
         const auto error{strprintf("%s: Mempool is too sparse for fee rate estimation", FeeRateEstimatorTypeToString(estimator_type))};
         cache.Update(FeePerVSize{0, 0}, FeePerVSize{0, 0}, blocktemplate->block.hashPrevBlock, error);
-        return {estimator_type, FeePerVSize{0, 0}, MEMPOOL_FEE_ESTIMATOR_MAX_TARGET, error};
+        return {estimator_type, FeePerVSize{0, 0}, MEMPOOL_FEE_ESTIMATOR_MAX_TARGET, error,"Mempool is too sparse"};
     }
     cache.Update(percentiles.p50, percentiles.p75, blocktemplate->block.hashPrevBlock);
     // Reference size (1 kvB) used to express fee rates as fee-per-kvB.
diff --git a/src/policy/fees/mempool_estimator.h b/src/policy/fees/mempool_estimator.h
index ce4bceb6d4..57489d0114 100644
--- a/src/policy/fees/mempool_estimator.h
+++ b/src/policy/fees/mempool_estimator.h
@@ -41,6 +41,7 @@ struct MinedBlockStats {
     uint64_t m_height{0};                   //!< Block height.
     uint64_t m_removed_block_txs_weight{0}; //!< Weight of mempool transactions removed for this block (excluding coinbase).
     uint64_t m_block_weight{0};             //!< Total non-coinbase transaction weight in the block.
+    std::optional<std::string> reason{std::nullopt};                                 //!< Reason of why it is not being used this estimator.
 };
 
 /**
@@ -102,10 +103,11 @@ public:
     void Flush() EXCLUSIVE_LOCKS_REQUIRED(!cs);
     bool Read(AutoFile& file) EXCLUSIVE_LOCKS_REQUIRED(!cs);
     bool Write(AutoFile& file) EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    void AddReason(std::optional<std::string> reason) EXCLUSIVE_LOCKS_REQUIRED(!cs);;
 
 private:
     //! Checks if recent mined blocks indicate a healthy mempool state (internal-only).
-    bool isMempoolHealthy() const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::optional<std::string> isMempoolHealthy() const EXCLUSIVE_LOCKS_REQUIRED(cs);
     //! Tracks weight statistics for the last NUMBER_OF_BLOCKS mined blocks.
     std::deque<MinedBlockStats> prev_mined_blocks GUARDED_BY(cs);
 
diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp
index e79b11d4b4..f7d4edf44c 100644
--- a/src/rpc/fees.cpp
+++ b/src/rpc/fees.cpp
@@ -74,6 +74,7 @@ static RPCMethod estimatesmartfee()
                                 {RPCResult::Type::NUM, "mempool_txs_weight", "Total weight of transactions removed from the mempool for this block"},
                             }},
                     }},
+                {RPCResult::Type::STR, "mempool_estimator_skip_reason", /*optional=*/true, "Reason why the mempool estimator was not used"},
         }},
         RPCExamples{
             HelpExampleCli("estimatesmartfee", "6") +
@@ -127,6 +128,9 @@ static RPCMethod estimatesmartfee()
                     entry.pushKV("mempool_txs_weight", it->m_removed_block_txs_weight);
                     mempool_health_stats.push_back(std::move(entry));
                 }
+                if (blocks_data.back().reason.has_value()) {
+                    result.pushKV("mempool_estimator_skip_reason", blocks_data.back().reason.value());
+                }
                 result.pushKV("mempool_health_statistics", std::move(mempool_health_stats));
             }
             return result;
diff --git a/src/util/fees.h b/src/util/fees.h
index 08d5186062..82773bc97f 100644
--- a/src/util/fees.h
+++ b/src/util/fees.h
@@ -70,8 +70,9 @@ struct FeeRateEstimatorResult {
     //! The returned target at which the package is likely to confirm within
     int returned_target;
     std::string error; // error message encountered
-    FeeRateEstimatorResult(FeeRateEstimatorType estimator, FeePerVSize feerate, int returned_target, std::string error)
-        : feerate_estimator{estimator}, feerate{feerate}, returned_target{returned_target}, error{error} {}
+    std::optional<std::string> reason;
+    FeeRateEstimatorResult(FeeRateEstimatorType estimator, FeePerVSize feerate, int returned_target, std::string error, std::optional<std::string> reason = std::nullopt)
+        : feerate_estimator{estimator}, feerate{feerate}, returned_target{returned_target}, error{error}, reason{reason} {}
     /**
      * Compare two FeeRateEstimatorResult objects based on fee rate
      * [@param](/bitcoin-bitcoin/contributor/param/) other The other FeeRateEstimatorResult object to compare with

</details>

Also I've been using the new estimator to see how it behaves. I was worried that the mempool had a low respresnatation ratio or that it was too sparse to fill 95% of a block. After collecting data for 3 days, I've seen this concerns were not justified.

<details> <summary>You can see here the results of the data I gathered:</summary>

What I did was to call the estimatesmartfee rpc every 7 seconds, with the parameters conf_target=6, estimate_mode=economical and {block_policy_only: false, verbosity: 2}. I ran my own version so the rpc returns the reason of why the mempool estimator was not choosen. Between calls I also used getmempoolinfo to get general information about the state of the mempool.

This are the results I got:

<img width="2100" height="3000" alt="fee_monitor_charts_2" src="https://github.com/user-attachments/assets/5ad304b0-6ec0-4ac1-8d23-3a9222dce9e6" />

In the graph, the backround blue represents that the mempool estimator was choosend, and the backround amber reresents that block policy was choosen.

We have that more or less 60% of the times the block policy estimator is used, and the other 40% the mempool estimator. The representation ratio, is always higher than 90%, so unless a user have too many mempool policies, it shouldn't be difficult to have it over the 75% threshold (I was worried that it were difficult to get to this thershold).

All the time that the block estimator was choosend over the mempool one, it was because the block estimator returned a lower feerate, so at least for the past 3 days, the mempool estimator did not have any problems regarding the ismempoolhealthy.

I've noticed that when the estimator is set to the block policy, the returned fee rate exhibits spikes, which may lead to an overestimation. In contrast, when the mempool estimator is selected, the estimated fee rate is generally more stable. Also, the block policy estimator tends to be selected when fee rates in the mempool vary significantly, whereas the mempool estimator produces more consistent fee estimates even under those conditions.

</details>

re-ACK 23cacb13e7baf66b75950dd3dd4f595abfb9a38f

Since my last ACK addressed some small feedback from other reviewers.

ACK 23cacb13e7baf66b75950dd3dd4f595abfb9a38f

Most of my previous review concerns were addressed; FeeRateEstimatorManager::GetFeeRateEstimate() now returns immediately if block-policy has no estimate, so it no longer does wasted work when the mempool result would be discarded, and MemPoolFeeRateEstimatorCache now caches negative results.

Happy that anythign else can be done in follow-ups at this point.

Left some minor review comments, none critical for the overall direction of this PR. Ideally this gets in soon for users soon, this would be a great improvement.

Thanks for your review @rustaceanrob. I will address them when there is a need to update or in a follow-up.

I'm keen on getting this change in for v32. I've been weighing between suggesting keeping this PR complete and splitting off the first two phases. We seem to have a group of dedicated reviewers here now, so I'm not too worried about a split introducing significant delay. I think the first four commits stand nicely on their own legs and would suggest putting them in a separate PR. I feel like the commits introducing the estimator manager can probably stay here, What do you think?

I've been weighing between suggesting keeping this PR complete and splitting off the first two phases. We seem to have a group of dedicated reviewers here now, so I'm not too worried about a split introducing significant delay. I think the first four commits stand nicely on their own legs and would suggest putting them in a separate PR. I feel like the commits introducing the estimator manager can probably stay here, What do you think?

There is already a separate PR for logically related commits: #34617 I’m generally not a fan of splitting changes across multiple PRs unless the changes are very large. I see this PR as logically flowing from the refactors, so I’d prefer to merge them together. The recent comments are helpful, and I’d like to take them.

Forced pushed from 23cacb13e7 to 86f5c1c99 23cacb13e7...86f5c1c99

• #34075 (comment) I now log reasons that mempool is unhealthy.
• #34075 (review) The new method does this using Util::Result, but changing estimateSmartfee method to do this is beyond the scope of this change.
• #34075 (review) This is gone, but I took the idea and implemented it in the new commits.
• #34075 (review) Done
• #34075 (review) Done
• #34075 (review) Fixed
• #34075 (review) I did not take this because I would like to maintain the transaction and not risk a race between two threads trying to read/write the file.
• #34075 (review) Persisting integral types are supported. If I change to double, I have to write code to encode to double for persistence. I only cast once, so I will just cast here.
• #34075 (review) Done
• #34075 (review) Done
• #34075 (review) Done
• #34075 (review) Good idea, taken
• #34075 (review) Fixed
• #34075 (review) Yeah, changing the test is beyond the scope; I will limit it to the current patch.
• #34075 (review) I added another constructor to the CFeeRate with a comment, hopefully that makes it clearer.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/29027994565/job/86152818424} _{LLM reason (✨ experimental): CI failed because IWYU reported incorrect/missing #include directives (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>

All user facing features look good to me. Approach has not change since my last review.

ACK 3c57780022f0d11a971521698e2ffde843842a9e

Force-pushed from e586844902 to 3c57780022 e586844902...3c57780022

Thanks for the review @sedited and @rustaceanrob. This is in a much better state after the recent fixes.

• #34075 (review) Changed to Assume.
• #34075 (review) Indeed, done. I moved it to the mempool estimator class instead; it does not seem like a block template method (the sorting concern is mostly cheap O(n), so not an issue imo).
• #34075 (review) Done, thanks.
• #34075 (review) Fixed.
• #34075 (review) Fixed.
• #34075 (review) Great catch, it is not safe. I moved the lock and documented the new behaviour.
• #34075 (review) Done.
• #34075 (review) Fixed here and in other places.
• #34075 (review) I applied the suggestion and documented the new behaviour in the release notes. This turns out to fix #35507 indirectly: instead of returning this high fee rate estimate, we now notify that the mempool is sparse or unhealthy (users can manually see what the min relay fee is and construct the transaction with it, or adjust their node to run the signet with inquisition to see all traffic, for example). Note: this only affects test networks, but it works well on current mainnet.
• #34075 (review) Added this suggestion and other fixes (mainly rejecting gaps in the data, e.g. due to an unclean shutdown where recent stats were not persisted; another potential fix is to persist on each block connection, but that's too aggressive).
• #34075 (review) Done, this is a great suggestion; it makes exposing mempool-only in the future easy. This replaced the block_policy_only boolean with the more general fee_rate_estimator string option ("none"/"block_policy"), and the RPC/functional tests were updated accordingly.

Other changes

• Relaxed the mempool estimator sparse-template requirement from enough chunks to fill 95% of a block to enough chunks to calculate the 75th percentile. Requiring a nearly full template meant erroring in low-demand periods, precisely when estimates matter most; the 75th percentile is the highest percentile the estimator returns, so coverage beyond it adds no information.
• GetFeeRateEstimate(NONE, …) now returns the manager-selected combined estimate instead of erroring, and the estimator response field is only emitted for successful combined estimates.
• The release notes are also improved.

Rebased to fix conflict 3c57780022...f7ba53dc9

ACK f7ba53dc90eb31365e6d0aae8b6875429ff9dd84

ACK f7ba53d.

I agree with the policy when mempool health is unavailable or unreliable, returning the block-policy estimate could preserve the overestimation this PR targets.

Unless anyone spots anything drastically wrong, I think any further changes will likely be ok to be in follow-ups at this stage.

Code Review ACK f7ba53d

The PR has progressed significantly since my last review comment and a lot of improvements have been made. The changes are looking really good, the commit messages are well written, and the release notes clearly summarize the user-facing changes, all of which made it much easier to follow the evolution of the PR while reviewing the changes.

Some notable changes since my last review include:

• The removal of the estimator abstraction. Previously, the estimators were hidden behind a generic interface, but this abstraction provided little practical benefit because the FeeRateEstimatorManager still needed to know the concrete estimator types in order to invoke type-specific functionality. This often required casting back to the concrete types, reducing the value of the abstraction. The manager now owns the estimators directly via std::unique_ptr<CBlockPolicyEstimator> and std::unique_ptr<MemPoolFeeRateEstimator>. This design is simpler and clearer, avoids unnecessary casting, and gives the manager direct access to the full API of each estimator while preserving its role as the central coordinator for fee estimation.

• estimatesmartfee now accepts an options object containing fee_rate_estimator. Recognized values are "none" (the default, which uses the combined behavior i.e mempool and block policy estimator, using mempool fee rate estimator to only reduce the fee rate returned by block policy fee rate estimator) and "block_policy" (which uses only the block policy fee rate estimator). The verbosity 2 also returns mempool health statistics for recently mined blocks when block_policy_only is false.

• The FeeSource direction was removed. The previously overloaded fee reason enum has been split into two separate enums: the wallet FeeReason, which explains why the wallet selected a particular fee rate, and BlockPolicyEstimateReason, which contains the Block Policy Estimator's internal threshold details. The wallet RPC field remains named fee_reason, but it now reports only the wallet's fee reason without exposing Block Policy Estimator internals.

• The argument parsing for both the mempool and block policy estimators has been consolidated into the shared helper files estimator_args.{cpp,h}.

• Added MaybeMigrateLegacyFeeEstimates(). Block policy fee estimates are now stored in fees/block_policy_estimates.dat. If the new file does not exist, the legacy fee_estimates.dat file is moved to the new location during startup. If both files exist, the legacy file is removed.

• Avoided unnecessary work by skipping the mempool estimator entirely when the block policy estimator has already failed and the result would not be used.

-Cached failure states (such as a sparse mempool), preventing repeated expensive fee estimation work when nothing has changed.

• The estimation functions now return util::Expected<FeeRateEstimation, FeeRateEstimationError>, where the error type contains both the error string and the corresponding zero-value estimation.

The only thing I have left to do is run the branch on my own node and compare the estimates against those shown on bitcoincorefeerate.com to observe the behavior in practice.

Overall, this is some really nice work! I learned a lot while reviewing this PR.

Made it to the end. Left a few more questions, thanks for the work here.

Forced pushed from f7ba53dc90eb31365e6d0aae8b6875429ff9dd84 to 5d5a66dbbbf9a10ea80d107ce915684b4f07ab11 f7ba53dc90...5d5a66dbbb

Thanks for your review @sedited

I addressed review comments and fixed a few nits and other issues discovered in my review with agents.

<details>

• Mempool estimator filename renamed from mempool_policy_estimates.dat to mempool_policy_estimator.dat.
• Function renamed from MempoolPolicyFeeEstPath to MempoolPolicyEstimatorPath.
• Logging now says mined-block stats / estimator data instead of estimates.
• NUMBER_OF_BLOCKS renamed to MEMPOOL_HEALTH_WINDOW_BLOCKS.
• Low-activity windows below one block worth of total non-coinbase weight are considered healthy.
• Mempool health logs now include removed_weight, total_block_weight, coverage, and required coverage.
• CalculateMaxWeightPercentiles no longer returns std::optional.
• Empty/unfilled percentiles are represented by empty FeePerVSize.
• CTxMemPool::removeForBlock now returns removed txs info.
• Validation emits MempoolTransactionsRemovedForBlock from ConnectTip, because the validation signal now includes the connected block hash (piping the hash and height through the mempool to fire notification indicates the notification should not be in the mempool, so I just returned the removed txs and the notification is emitted in the ConnectTip. This is safe because all fee estimation unit tests are now tested directly).
• Fix some Cache documentation issues.
• Sparse-mempool tests now expect the fee floor instead of an error.
• Mined-block stats test helper now passes a tip hash.
• feature_fee_estimation.py uses fees/mempool_policy_estimator.dat.
• feature_fee_estimation.py updates log expectations.
• Empty healthy mempool functional test now expects the fee floor.
• Added stale mempool block stats rejection on load using tip-hash mismatch test.
• policy_estimator_io fuzz target now uses TestingSetup with setup_net=false so chainman exists.
• policy_estimator_io removes unused mock-time consumption.
• policy_estimator_io uses the renamed mempool estimator path.
• policy_estimator_io explicitly closes fuzzed AutoFile after write.
• removeForBlock call sites updated in mempool_stress.cpp, tx_pool.cpp, mempool_tests.cpp, and rbf_tests.cpp.
• wallet_bumpfee.py dust-to-fee numbers made deterministic with explicit destination/change sizing and lower fee rate.
• wallet_taproot.py cleanup sendall calls now pass fee_rate=20.
• release notes improvements
• commit message improvement and rewords
• RPC help text line-wrapped and wording cleaned.
• Manager constructor/callback signatures line-wrapped.
• Comments corrected from fee estimates/params to estimator data/options.
• CBlockPolicyEstimator uses GetFeePerVSize instead of reconstructing through a dummy 1000 vsize.

</details>

ACK 5d5a66dbbbf9a10ea80d107ce915684b4f07ab11

reACK 5d5a66dbbbf9a10ea80d107ce915684b4f07ab11

Did a swift re-review of the code (skipped the tests). Left some minor nits that can be ignored and fixed only if touching again. The code lgtm as is :)