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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #33741 (rpc: Optionally print feerates in sat/vb by polespinasa)
• #33199 (fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)
• #32966 (Silent Payments: Receiving by Eunovo)
• #31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
• #31382 (kernel: Flush in ChainstateManager destructor by sedited)
• #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
• #28690 (build: Introduce internal kernel library by sedited)

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.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/25327260112}

Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.

The state of the node's mempool may not accurately reflect the state of others' mempools, and not even its own mempool when the block is found in the future. It isn't a good single source of information. Perhaps it is a good idea to use it as a secondary source, but probably it should only ever adjust fee estimations upward, not down.

The state of the node's mempool may not accurately reflect the state of others' mempools, and not even its own mempool when the block is found in the future. It isn't a good single source of information.

Correct. The rationale behind this set of changes can be summed up briefly as follows:

• Add a new standalone modular fee estimation manager to which (many) Forcasters can be trivially added or removed (vs modifying BlockPolicyEstimator).
• Provide two forcaster implementations which do not exist today ( 1. mempool-based and 2. previous-6-blocks-seen-in-mempool) to demonstrate functionality.

In the present, there is a strong tendency for users of Bitcoin Core to consult external fee estimation services when they want timely (next 1 or 2 blocks) confirmation of transactions, whilst also not overpaying. Examples of these include mempool.space, whatthefee.io, Samourai's nextblock.is (now down), johoe, blockchair etc., with more popping up every month.

We also analysed/estimated Bitcoin Core users not using in-built estimation in a post here.

In our opinion having users feel the need to use external fee estimation services that they could equally have served to them by their own node, feels sub-optimal.

In addition to this, when users do use the current Bitcoin Core fee estimator, there are often times when they end up overpaying, see delving bitcoin post Mempool based fee estimation and various issues over the years e.g. #30009 . This is avoidable.

Work from a student of @renepickhardt link demonstrated something we also measured independently -- that bitcoin core's current estimates are often overpaying significantly following fee spikes. This effect can be directly mitigated by using a mempool-based estimation.

Perhaps it is a good idea to use it as a secondary source, but probably it should only ever adjust fee estimations upward, not down.

In this changeset we take the approach of using the lowest result from all "confident" Forcasters. The rationale is that we expect users wanting fast confirmation to have RBF enabled, allowing them to bump fees if we still undershoot.

Comments from @harding link explained that it may be possible for miners to artificially increase a strictly-mempool-based fee-rate. By taking the lower (confident) result from n Forcasters, we attempt to protect against this attack (and others like it), at the potential cost of having to RBF.

We do plan to add additional sanity checks to the mempool-based Forcaster as described in #27995, but these are not yet implemented. In any case, even without these additional checks we have been seeing much-improved short time-scale estimations from Bitcoin Core.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/27003108803}

I restarted my relatively long-running node after building this PR and notice that now I get the following when trying to get any estimate:

src/core/bitcoin on  pr-30157 [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env)
❯ /home/will/src/core/bitcoin/build/bin/bitcoin-cli estimatesmartfee 1
{
  "blocks": 0,
  "errors": [
    "Insufficient data (at least 6 blocks needs to be processed after startup)"
  ]
}

src/core/bitcoin on  pr-30157 [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env)
❯ /home/will/src/core/bitcoin/build/bin/bitcoin-cli estimatesmartfee 3
{
  "blocks": 0,
  "errors": [
    "Insufficient data (at least 6 blocks needs to be processed after startup)"
  ]
}

This is not the case when I restart v29.0:

src/core/bitcoin on  pr-30157 [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env)
❯ bitcoind
Bitcoin Core starting

src/core/bitcoin on  pr-30157 [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env)
❯ /home/will/src/core/bitcoin/build/bin/bitcoin-cli estimatesmartfee 2
{
  "feerate": 0.00005081,
  "blocks": 2
}

It's able to return a fee estimate to me immediately, as it has saved estimation data from the previous run. I don't think we should break this behaviour here; waiting ~ an hour to get a new estimate after a restart is not a pleasant UX.

At the very least we should not nerf blockpolicy estimations as part of this change. Although, I think thought that we should be smart enough to know if we have a ~ up-to-date mempool and use that data if we do, to return estimates via the mempool-based estimator. If we need, perhaps we can return a warning about the age of the mempool when returning e.g. a next-block estimate, although with RBF prevalance now I'm not sure even that is necessary?

I restarted my relatively long-running node after building this PR and notice that now I get the following when trying to get any estimate

Yep, the node needs to see six blocks mined before making an estimate. Were you able to see an estimate after six blocks were mined?

It's able to return a fee estimate to me immediately, as it has saved estimation data from the previous run. I don't think we should break this behaviour here; waiting ~ an hour to get a new estimate after a restart is not a pleasant UX.

At the very least we should not nerf blockpolicy estimations as part of this change. Although, I think thought that we should be smart enough to know if we have a ~ up-to-date mempool and use that data if we do, to return estimates via the mempool-based estimator. If we need, perhaps we can return a warning about the age of the mempool when returning e.g. a next-block estimate, although with RBF prevalance now I'm not sure even that is necessary?

There is a change in behavior. Before restarting, it was solely using the block policy. Now, it uses both the block policy and the mempool.
The forecaster manager needs to see some blocks mined and determine whether the mempool policy is sane. It should work fine after six blocks are mined (I agree that six blocks is a bit extreme; note: it just a conservative arbitrary value to be sure).

This methodology aligns with the current status quo of not returning anything when we don't have enough data.
Although, I think we should return something which is why I modified block policy to do so in #32395. It’s true that this isn’t a pleasant UX, and I think we might see more complaints if this is rolled out as-is.

So perhaps we should return the block policy estimate if it's available in the meantime and reduce NUMBER_OF_BLOCKS?

Yep, the node needs to see six blocks mined before making an estimate. Were you able to see an estimate after six blocks were mined?

I'm still waiting 😄 but I assume that I will be able to eventually...

When we have both fee_estimates.dat and mempool.dat saving all the relevant data I don't think we should be introducing a 6 block wait to get fee estimates. My data was ~ 10 seconds out of date after a restart, and now I must wait an hour.

I'm still waiting 😄 but I assume that I will be able to eventually...

When we have both fee_estimates.dat and mempool.dat saving all the relevant data I don't think we should be introducing a 6 block wait to get fee estimates. My data was ~ 10 seconds out of date after a restart, and now I must wait an hour.

This is fixed now, thanks for testing.

closed in favour of #34075