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

• Separation of feerate format from the fee estimate mode enumerator (separation of concerns, as they are semantically distinct)
• Move logging after fee estimation estimateSmartFee from the wallet to the Block Policy Estimator internal method (also separation of concerns and avoiding leaking fee estimator internals). This change also comes with updating createTransactionInternal to log only the fee and its source (fee estimator, fallback fee, etc.), which is more appropriate.
• Separate the fee estimate mode enum from the fee source enumerator (separation of concerns, as they are semantically distinct). This introduces a minor breaking change: the wallet RPCs now return fee source instead of fee reason, which is more accurate. Let me know if we prefer to keep the field name unchanged while returning the fee source.
• Move the StringForReason enum from common to the Block Policy Estimator (also separation of concerns)
• Various test file updates and renames for improved accuracy
• Update Block Policy Estimator unit tests to be independent of the mempool and the validation interface

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

• Introduce FeeRateEstimateResult as the response type, containing metadata and avoiding the use of out-parameters. This is easily extensible and makes future changes less invasive, as method signatures do not need to change.

• Add FeeEstimatorType enum to identify different fee estimators

• Make CBlockPolicyEstimator derive from the new fee estimator base class

• Add FeeRateEstimatorManager, responsible for managing all fee estimators

• 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 percentile calculation function that takes the chunk feerates of a block template and returns feerate percentiles; this can also be reused by the getblockstats RPC.

• When the mempool is empty we return the max of minrelaytxfee and mempoolminfee as the fee rate estimate

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

• 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: https://github.com/bitcoin/bitcoin/issues/33389

• Update MempoolTransactionRemovedForBlock to return the actual block transactions as well

• Track the weight of block transactions and evicted transactions 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 evicted due to BLOCK reasons 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 blocks to disk during periodic flushes and shutdown, so this data is available after restarts

• Expose this data to users when running estimatesmartfee with verbosity > 2

• Expose the selected fee rate estimator that is used to generate a fee rate estimate by the manager.

• Add block_policy_only only option in the estimatesmartfee R.P.C (It is the default now).

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

bitcoin-cli estimatesmartfee 1 "conservative" 2

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

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

• #34995 (ci, iwyu: Fix warnings in src/common and treat them as errors by hebasto)
• #34806 (refactor: logging: Various API improvements by ajtowns)
• #34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
• #34405 (wallet: skip APS when no partial spend exists by 8144225309)
• #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
• #33637 (refactor: optimize block index comparisons (1.4-6.8x faster) by l0rinc)
• #33421 (node: add BlockTemplateCache by ismaelsadeeq)
• #32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
• #32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)
• #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 typos and grammar issues:

• // mempool_txs[j] is populated with transactions either of -> // mempool_txs[j] is populated with transactions of the corresponding fee bucket [the added comment is incomplete, so the intended meaning is hard to parse]

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

• estimatesmartfee(1, "economical", 2, False) in test/functional/feature_fee_estimation.py

^{2026-04-30 11:58:50}

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