Fees: add Fee rate Forecaster Manager

ismaelsadeeq commented at 9:42 pm on January 15, 2025: member

This PR implements the core component of #30392, introducing a new fee rate forecasting module. The primary addition is a ForecasterManager that coordinates multiple forecasting strategies to be able to provide better transaction fee rate predictions.

Key Changes

Refactoring of CBlockPolicyEstimator #33218
- Renames fees to block_policy_estimator for clarity.
- Renamesfees_args to block_policy_estimator_args for clarity
- Renames policy_fee_tests to feerounder_tests also for clarity.
Addition of Fee Rate Forecasting Utility Structures
- Forecaster abstract class, serving as the base class for all fee rate forecasters.
- ForecastResult (the response from a Forecaster) with all metadata.
- ForecastType enum, identify and distinguish forecasters.
Introduce FeeRateForecasterManager class
- Adds the FeeRateForecasterManager class, responsible for managing all fee rate forecasters, including CBlockPolicyEstimator, which is now a forecaster.
- Updates the node context to hold a unique pointer to FeeRateForecasterManager instead of CBlockPolicyEstimator.
- Changes CBlockPolicyEstimator instance from unique_ptr to a shared_pointer, allowing it to be registered in the validation interface without requiring explicit unregistration during shutdown (current master behaviour). Registering for CBlockPolicyEstimator flushes also get a shared_pointer.
- Exposes a raw pointer of CBlockPolicyEstimator through FeeRateForecasterManager for compatibility with estimateSmartFee, friends, and node interfaces.
- Modifies CBlockPolicyEstimator to inherit from the Forecaster class and implement the pure abstract classes
Introduce MempoolForecaster class
- Adds the MempoolForecaster, which forecast the fee rate required for a transaction to confirm as soon as possible. It generates a block template and returns the 75th and 50th percentile fee rates as low-priority and high-priority fee rate forecasts.
- Implements caching for the last fee rate forecast, using a 30-second rate limit to avoid frequent block generation via the block assembler.

The FeeRateForecasterManager now includes a method ForecastFeeRateFromForecasters. This method polls forecasts from all registered forecasters and returns the lowest value. This behavior aligns with discussions and research outlined in this post. But the primary aim of the FeeRateForecasterManager is to be able to perform sanity checks and potentially apply heuristics to determine the most appropriate fee rate forecast from polled forecasters, for now it just return the lowest.

This PR also add unit tests for the classes.

In Draft because it depends on #33218

DrahtBot commented at 9:42 pm on January 15, 2025: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	willcl-ark, frankomosh, polespinasa

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:

#33218 (refactor: rename fees.{h,cpp} to fees/block_policy_estimator.{h,cpp} by ismaelsadeeq)
#33199 (fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)
#32983 (rpc: refactor: use string_view in Arg/MaybeArg by stickies-v)
#32395 (fees: rpc: estimatesmartfee now returns a fee rate estimate during low network activity by ismaelsadeeq)
#31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#28690 (build: Introduce internal kernel library by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Jan 22, 2025

ismaelsadeeq force-pushed on Jan 22, 2025

DrahtBot removed the label Needs rebase on Jan 22, 2025

glozow added the label TX fees and policy on Feb 12, 2025

in src/policy/fees/forecaster_man.cpp:50 in c6b94403bf outdated

45+            if (selected_forecast.Empty()) {
46+                // If there's no selected forecast, choose curr_forecast as selected_forecast.
47+                selected_forecast = curr_forecast;
48+            } else {
49+                // Otherwise, choose the smaller as selected_forecast.
50+                selected_forecast = std::min(selected_forecast, curr_forecast);

frankomosh commented at 2:32 pm on April 8, 2025:

from my understanding here, the manager is to always choose a forecast having the lowest high_priority fee rate from

0bool operator<(const ForecastResult& other) const { return m_response.high_priority << other.m_response.high_priority; }

assuming we have mempool congestion spikes or other time-sensitive operations, wouldn’t we have a problem with underpayment if we always taking the minimum fee estimate here?

ismaelsadeeq commented at 3:06 pm on April 8, 2025:

Good question, @frankomosh.

You’re right to think the lowest fee rate might be an underestimate during instant spikes.

This approach was specifically chosen to address the concern raised by @harding here: https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/6.

Although the concern is currently theoretical, @harding raised a valid point, and it seems reasonable to use mempool forecasts to adjust or correct block policy forecasts.

Empirical data also shows that this approach maintains around 60% accuracy, which performs better than the current status quo for economical fee rate forecasts: https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8?u=ismaelsadeeq.

My goal is to fix these issues with the following steps:

Select the lower value between the mempool forecast and the block policy forecast.
Return the certainty level of the mempool fee rate forecast based on statistics from previous block data in the next PR after this — see the POC here: #30157.
Use fee rate forecast from forecaster manager in the wallet

Potential Next Steps

Ensure the mempool is not being manipulated, and use the mempool forecast even during congestion for ASAP fee rate estimates. / Or expose the mempool state so users can fee bump when congestion persists and they absolutely need to do so.
Switch to a more economical fee rate forecaster for the long term, because the current one pays more than necessary, as by quite a large margin see research https://bitcoin.stackexchange.com/questions/124227/utility-of-longterm-fee-estimation/.

frankomosh commented at 7:41 am on April 9, 2025:

Thanks, @ismaelsadeeq, for sharing more context on this topic. I now see and understand why you took this approach. I think that the plan for improvement is also solid.

In 1. Select the lower value between the mempool forecast and the block policy forecast. I appreciate the approach of selecting the lower value forecasters to avoid fee overpayment.

Additionally, I would like to ask if we could improve accuracy by implementing a weighted selection strategy based on confirmation targets (this is just theoretical thinking since I haven’t done any tests on it).

From the little context I have tried to gather, I see that different forecasters seem to do well in different scenarios i.e CBlockPolicyEstimator likely offers a more reliable estimates during periods of stability and for longer confirmation targets, while the MempoolForecaster might offer better predictions during rapid changes and for immediate confirmation targets of like (1-2 blocks).

Would it, therefore, be worth considering a more targeted approach where we prefer MempoolForecaster for short-term targets and gradually shift to CBlockPolicyEstimator for longer-term targets of 3+ blocks?

I think this could maintain the fee savings of your current approach while improving accuracy in time-sensitive scenarios where underpayment could have repercussions for users. Even if not for this PR, it might be something to consider for a future enhancement.

(Note that I’m making assumptions here and have not looked into the overhead of actually doing this.)

ismaelsadeeq commented at 10:04 am on April 9, 2025:

Mempool forecaster only provide fee rate forecast for 1-2 blocks. see MEMPOOL_FORECAST_MAX_TARGET constant in https://github.com/bitcoin/bitcoin/pull/31664/commits/c7cdeafd7045c794b289e2438cc60baeee7e6f69

ismaelsadeeq commented at 3:06 pm on April 8, 2025: member

There is going to be a review club tomorrow on this PR https://bitcoincore.reviews/31664 notes and questions are up

in src/policy/fees/forecaster_util.h:47 in c6b94403bf outdated

123+ * Represents the input for a parameter of fee rate forecaster.
124+ */
125+struct ConfirmationTarget {
126+    unsigned int value;
127+    ConfirmationTargetType type;
128+};

glozow commented at 5:04 pm on April 9, 2025:

Are there any specific ideas for another ConfirmationTargetType? The only type defined here is a number of blocks (which could just be an integer) so I’m wondering why the complexity?

ismaelsadeeq commented at 7:07 pm on April 9, 2025:

Yes, as is now, it might seem like overkill (though it will be useful in the future). This is just forward-looking I believe that, realistically, businesses and users care more about time in minutes or hours, etc.

So, I envision a fee rate forecaster that accepts input based on time in hours rather than blocks. For example: ‘Give me a fee rate that will allow my transaction to confirm within the next 4 hours.’

this is a wishful thinking for now; no concrete idea or even reviewable design about it I don’t have a strong opinion on this we can easily refactor and enable it when needed.

Happy to remove it if we prefer to keep only structures that are used immediately.

glozow commented at 7:48 pm on April 9, 2025:

If there aren’t any concrete ideas, it seems better to add the extra complexity if/when there is a reason to. fwiw I don’t see why translating between hours and blocks would require another target type?

ismaelsadeeq commented at 2:21 pm on April 13, 2025:

Fixed in recent push.

glozow added the label Review club on Apr 9, 2025

in src/policy/fees/block_policy_estimator.cpp:746 in 9355da6de4 outdated

741+    }
742+    // Note: size can be any positive non-zero integer; the evaluated fee/size will result in the same fee rate,
743+    // and we only care that the fee rate remains consistent.
744+    int32_t size = 1000;
745+    response.low_priority = FeeFrac(feerate_economical.GetFee(size), size);
746+    response.high_priority = FeeFrac(feerate_conservative.GetFee(size), size);

glozow commented at 6:11 pm on April 9, 2025:

this might just be a personal opinion, but “low priority” and “high priority” usually mean different block targets to me. people who are accustomed to the mempool.space wording might say the same. but really, here and in the MempoolForecaster, it’s a measure of how conservative/confident we are in our estimate no?

ismaelsadeeq commented at 6:54 pm on April 9, 2025:

Good point Yes, it will be better to maintain the conservative and economical name? Happy to update.

ismaelsadeeq commented at 2:22 pm on April 13, 2025:

This is fixed now.

ismaelsadeeq commented at 9:35 am on April 10, 2025: member

I will update the PR to address recent comments, I also want to remove some redundancy and restructure the commits.

in src/policy/fees/mempool_forecaster.cpp:31 in 5bd2220422 outdated

30@@ -31,6 +31,13 @@ ForecastResult MemPoolForecaster::EstimateFee(ConfirmationTarget& target)
31                                             target.value, MEMPOOL_FORECAST_MAX_TARGET, MEMPOOL_FORECAST_MAX_TARGET));
32     }
33 
34+    const auto cached_estimate = cache.get();

polespinasa commented at 2:35 pm on April 10, 2025:

Already commented during the review club but leaving a comment to make it easy to track.

The cache should not be returned when there is a chain reorganization. This can be done by simply checking if there was a reorganization during the last CACHE_LIFE window or by clearing the cache after a reorganization.

Personally, clearing the cache seems to me to be the best option, since the method can also be used for testing, as we may want to call the forecaster several times and do not want the cache to influence it.

ismaelsadeeq commented at 3:45 pm on April 25, 2025:

Even better, I modified CachedMempoolForecast to store the last known tip. Now, the mempool forecaster will only return the cached fee rate if the last known tip matches the current tip.

This also prevents a case where a cache exists but a new block has just been mined and the mempool has been cleared—in such cases, a new block template should be generated instead of using the cache.

Thanks for your review @polespinasa

ismaelsadeeq force-pushed on Apr 13, 2025

ismaelsadeeq commented at 2:28 pm on April 13, 2025: member

Forced pushed c6b94403bf9d5bf4fe9d5e5360b3d2c30a430d69..9455f2f0927014f2cc2495d432be635b58cc8fe2

I’ve made the following changes:

Rebased on master.
Reorganized the commits (refactor comes first).
Squashed the structure-related commits into one.
Addressed comments by @glozow.
Removed some redundancy and things that seemed like YAGNI.
Comparison of forecast results is now based on fee rate, and we now return "economical" by default—following comments by @polespinasa and @monlovesmango in the recent review club meeting.

DrahtBot added the label Needs rebase on Apr 14, 2025

ismaelsadeeq force-pushed on Apr 15, 2025

DrahtBot removed the label Needs rebase on Apr 15, 2025

ismaelsadeeq force-pushed on Apr 25, 2025

in src/policy/fees/mempool_forecaster.cpp:33 in ed220500c8 outdated

27@@ -28,6 +28,13 @@ ForecastResult MemPoolForecaster::ForecastFeeRate(int target, bool conservative)
28         return result;
29     }
30 
31+    const auto cached_estimate = cache.get_cached_forecast();
32+    const auto last_tip = cache.get_last_known_tip();
33+    if (cached_estimate && activeTip->nHeight == last_tip) {

polespinasa commented at 9:23 am on April 26, 2025:

I like the new approach!

What do you think about saving the hash and not the heigh? In case of a reorg we could be at the same height and think that nothing happened.

0if (chache_estimate && activeTip->phashBlock == last_tip)
1...

ismaelsadeeq commented at 1:06 pm on April 27, 2025:

Done.

ismaelsadeeq force-pushed on Apr 27, 2025

bitcoin deleted a comment on Apr 27, 2025

DrahtBot added the label Needs rebase on May 13, 2025

ismaelsadeeq force-pushed on May 14, 2025

DrahtBot removed the label Needs rebase on May 14, 2025

DrahtBot added the label Needs rebase on May 20, 2025

ismaelsadeeq force-pushed on May 20, 2025

DrahtBot removed the label Needs rebase on May 20, 2025

DrahtBot added the label CI failed on May 21, 2025

DrahtBot commented at 1:16 am on May 21, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/42588192236 LLM reason (✨ experimental): The CI failure is due to a build error: an uninitialized variable in forecaster_man.cpp causes a compilation failure.

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.

in src/policy/fees/forecaster_util.h:55 in cc4794b699 outdated

50+// Block percentiles fee rate (in BTC/vB).
51+struct Percentiles {
52+    FeeFrac p25; // 25th percentile
53+    FeeFrac p50; // 50th percentile
54+    FeeFrac p75; // 75th percentile
55+    FeeFrac p95; // 5th percentile

maflcko commented at 7:06 am on May 21, 2025:

could remove the redundant comments, given that at least one of them is wrong?

ismaelsadeeq commented at 12:56 pm on May 21, 2025:

fixed

in src/policy/fees/mempool_forecaster.cpp:46 in cc4794b699 outdated

41+
42+    const auto pblocktemplate = assembler.CreateNewBlock();
43+    const auto& m_package_feerates = pblocktemplate->m_package_feerates;
44+    const auto percentiles = CalculatePercentiles(m_package_feerates, DEFAULT_BLOCK_MAX_WEIGHT);
45+    if (percentiles.empty()) {
46+        result.m_error = "Forecaster unable a fee rate due to insufficient data";

maflcko commented at 7:07 am on May 21, 2025:

unable a fee rate -> unable to find a fee rate [Incorrect grammar; “unable to find” or “unable to provide” would be clearer.]

ismaelsadeeq commented at 12:54 pm on May 21, 2025:

thanks fixed.

maflcko commented at 7:07 am on May 21, 2025: member

(from the llm linter)

ismaelsadeeq force-pushed on May 21, 2025

in src/policy/fees/mempool_forecaster.h:28 in 7aca1af0cf outdated

23+// as mempool conditions are likely to change.
24+constexpr int MEMPOOL_FORECAST_MAX_TARGET{2};
25+constexpr std::chrono::seconds CACHE_LIFE{30};
26+
27+/**
28+ * CachedMempoolForecast holds a cache of recent fee rate forecast.

maflcko commented at 2:45 pm on May 21, 2025:

CachedMempoolForecast holds a cache of recent fee rate forecast. -> CachedMempoolForecast holds a cache of recent fee rate forecasts. [grammatical error: “forecast” should be plural when referring to multiple items]

ismaelsadeeq commented at 9:11 pm on May 21, 2025:

Fixed

maflcko commented at 2:45 pm on May 21, 2025: member

(one more lint, also the ci test-each-commit failed)

DrahtBot removed the label CI failed on May 21, 2025

ismaelsadeeq force-pushed on May 21, 2025

in src/policy/fees/block_policy_estimator.h:98 in 73dceb1917 outdated

94@@ -95,6 +95,7 @@ struct FeeCalculation
95     FeeReason reason = FeeReason::NONE;
96     int desiredTarget = 0;
97     int returnedTarget = 0;
98+    unsigned int bestheight{0};

willcl-ark commented at 11:02 am on June 11, 2025:

If you touch, to match the rest here?:

0    unsigned int bestHeight{0};

ismaelsadeeq commented at 10:11 am on June 12, 2025:

I think doc guideline say camel case, so I switched to that.

in src/policy/fees/forecaster_util.h:30 in f12941032d outdated

25+    std::optional<ForecastType> forecaster;
26+
27+    //! Fee rate sufficient to confirm a package within target.
28+    FeeFrac feerate;
29+
30+    //! The chain tip at which the forecast was made.

willcl-ark commented at 11:13 am on June 11, 2025:

0    //! The block height at which the forecast was made.

ismaelsadeeq commented at 10:11 am on June 12, 2025:

done

in src/policy/fees/forecaster_util.h:37 in f12941032d outdated

32+
33+    std::optional<std::string> error; ///< Optional error message.
34+
35+    /**
36+     * Overloaded less than operator which compares two ForecastResult objects based on
37+     * their fee rate.

willcl-ark commented at 11:14 am on June 11, 2025:

This could be simplified if you touch:

0     * Compare two ForecastResult objects based on fee rate.

ismaelsadeeq commented at 10:11 am on June 12, 2025:

Taken

in src/policy/fees/forecaster.h:26 in f12941032d outdated

21+    ForecastType GetForecastType() const { return m_forecast_type; }
22+
23+    /**
24+     * @brief Returns the estimated fee rate for a package to confirm within the given target.
25+     * @param target Confirmation target in blocks.
26+     * @param conservative True if the package cannot be fee bumped later.

willcl-ark commented at 12:14 pm on June 11, 2025:

0     * [@param](/bitcoin-bitcoin/contributor/param/) conservative True if the package cannot be fee bumped later.

Is this description correct? It looks like it passes through to estimateSmartFee’s conservative param (as I would have expected from the naming), and so not related to fee bumping?

ismaelsadeeq commented at 9:44 am on June 12, 2025:

I think it it correct we also use it to decide which percentile fee rate to return to user in mempool forecaster.

50th or 75th percentile fee rate.

The main idea is that conservative fee rate is for users who are willing to pay a bit higher to increase likelihood of confirmation; because they don’t want to fee bump.

Since all packages can be fee bumped now (due to full rbf).

I rephrased this to “If true, returns a higher fee rate for greater confirmation probability.” let me know how it sounds.

willcl-ark commented at 10:13 am on June 12, 2025:

Ok that makes sense to me

in src/policy/fees/block_policy_estimator.cpp:732 in e50b57a33b outdated

723@@ -723,6 +724,29 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) const
724     return estimateRawFee(confTarget, DOUBLE_SUCCESS_PCT, FeeEstimateHorizon::MED_HALFLIFE);
725 }
726 
727+ForecastResult CBlockPolicyEstimator::ForecastFeeRate(int target, bool conservative) const
728+{
729+    ForecastResult result;
730+    result.forecaster = ForecastType::BLOCK_POLICY;
731+    FeeCalculation feeCalcConservative;
732+    CFeeRate feerate{estimateSmartFee(target, &feeCalcConservative, conservative)};

willcl-ark commented at 12:21 pm on June 11, 2025:

If you touch, should this really be named feeCalc*Conservative*? This might not be a conservative estimate right?

ismaelsadeeq commented at 10:12 am on June 12, 2025:

Nice catch, fixed.

in src/policy/fees/forecaster_util.cpp:24 in 34f0674e01 outdated

19+    auto percentiles = Percentiles{};
20+
21+    // Process histogram entries while maintaining monotonicity
22+    for (const auto& curr_feerate : package_feerates) {
23+        accumulated_weight += curr_feerate.size * WITNESS_SCALE_FACTOR;
24+        // Maintain monotonicity by taking the minimum between the current and last tracked fee rate

willcl-ark commented at 12:28 pm on June 11, 2025:

0        // Maintain monotonicity by taking the minimum between the current and last tracked fee rate

Nice, I like this!

in src/policy/fees/forecaster_util.cpp:17 in 34f0674e01 outdated

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

willcl-ark commented at 12:37 pm on June 11, 2025:

I briefly wondered whether it would be worth using ceil or floor here to avoid any floating point rounding issues, but I can’t convince myself that they would cause any practical issues in this case.

ismaelsadeeq commented at 10:14 am on June 12, 2025:

The defaults round down I think, not sure if there is any potential difference by rounding up. So will leave as is.

in src/policy/fees/forecaster_util.cpp:43 in 34f0674e01 outdated

38+            percentiles.p95 = last_tracked_feerate;
39+            break; // Early exit once all percentiles are calculated
40+        }
41+    }
42+
43+    // Return empty percentiles if we couldn't calculate the 95th percentile.

willcl-ark commented at 12:49 pm on June 11, 2025:

I see why conservatively this might be a sound approach, but does this not make it so that if:

0sum_of_weights(package_feerates) < (0.95 * DEFAULT_BLOCK_MAX_WEIGHT)

then we would fail to return any estimate at all? This could happen when mempool is (somewhat) empty, for example. And IMO we should be able to return an estimate even in that case?

ismaelsadeeq commented at 10:01 am on June 12, 2025:

Nice Idea, this lead me to think about doing

 0    const int32_t p95_weight = 0.95 * total_weight;
 1    int32_t total_package_size = std::accumulate(
 2        package_feerates.begin(),
 3        package_feerates.end(),
 4        0,
 5        [](int32_t sum, const FeeFrac& f) {
 6            return sum + f.size;
 7        }
 8    );
 9    // Return empty percentiles if the total packages weight is less than the the 95th percentile weight.
10    if (total_package_size < p95_weight) return Percentiles{};

And remove the final check at the bottom, but then there might be an edge case were we have more exactly or a little higher weight than the 95th percentile. But due to skips while maintaining monotocity we ended up not returning the 95th percentile weight (in such cases we will return incomplete forecast).

This could happen when mempool is (somewhat) empty, for example. And IMO we should be able to return an estimate even in that case?

This is another seperate discussion of whether we will want to return fee rate forecast when we have sparse data. I have a PR for that which I ping you for review :) https://github.com/bitcoin/bitcoin/pull/32395

willcl-ark commented at 11:43 am on June 17, 2025:

Yeah OK, I agree this is fine to leave as-is.

in src/policy/fees/mempool_forecaster.h:24 in 6221f3dd6b outdated

11+class Chainstate;
12+class CTxMemPool;
13+
14+// Fee rate forecasts above this confirmation target are not reliable,
15+// as mempool conditions are likely to change.
16+constexpr int MEMPOOL_FORECAST_MAX_TARGET{2};

willcl-ark commented at 12:55 pm on June 11, 2025:

I wonder whether we may want to make this value a configuration option? I know some folks definitely look at other mempool-based estimates at time scales longer than this (whilst not being something we’d probably recommend, for the reasons you state)

ismaelsadeeq commented at 10:27 am on June 12, 2025:

Not sure whether that a good idea for ASAP-mempool Forecaster. But can good if you want something like https://github.com/FelixWeis/WhatTheFee--legacy So far I’ve not experimented with whatTheFee yet to determine its correctness.

willcl-ark commented at 10:04 am on June 17, 2025:

Yeah I agree we should start off scoping the mempool estimator to 2 blocks. You can resolve this.

in src/policy/fees/mempool_forecaster.h:25 in 917750fc16 outdated

20 class CTxMemPool;
21 
22 // Fee rate forecasts above this confirmation target are not reliable,
23 // as mempool conditions are likely to change.
24 constexpr int MEMPOOL_FORECAST_MAX_TARGET{2};
25+constexpr std::chrono::seconds CACHE_LIFE{30};

willcl-ark commented at 7:29 pm on June 11, 2025:

Also wonder here whether folks will want a config option for this…

ismaelsadeeq commented at 10:24 am on June 12, 2025:

I don’t think users should configure this. It should be determined by the forecaster, based on the potential increment of the top block in the mempool.

Previously, I researched how long it takes for a transaction to finish propagating through the network. @sr-gi responded with an estimate of approximately 17 seconds and ~7 seconds to the 50% of the network: https://bitcoin.stackexchange.com/questions/125776/how-long-does-it-take-for-a-transaction-to-propagate-through-the-network.

I thought I had updated this accordingly, but I did not. The assumption is that, 7 seconds after the last forecast, potential transactions that could improve the top block template might have entered node mempool

Earlier, I also discussed with @sipa the possibility of enabling the mempool to determine whether the top block has improved: #31283 (review).

If that feature becomes available, it would be a better approach than relying on a hardcoded 7-second delay.

After that, updating this logic should be straightforward.

A nice + to this cache limit is that it helps prevent a potential DoS due to resource contention from repeatedly requesting forecasts from the mempool forecaster.

in src/policy/fees/forecaster_man.h:25 in f79873fb51 outdated

22@@ -22,7 +23,6 @@ class FeeRateForecasterManager
23 private:
24     //! Map of all registered forecasters to their shared pointers.
25     std::unordered_map<ForecastType, std::shared_ptr<Forecaster>> forecasters;
26-

willcl-ark commented at 7:30 pm on June 11, 2025:

If you re-touch f79873fb512c20a97c5a6286118ef5b85df05027 you could undo this whitespace change

ismaelsadeeq commented at 10:24 am on June 12, 2025:

Done

willcl-ark commented at 8:22 pm on June 11, 2025: member

This looks pretty complete now, nice work.

Slightly sad it’s not wired up to the rpc for so I could do some live testing, but the unit tests confirm the expected behaviour.

Left a few comments about possibly allowing some of the mempool-based fee estimator constants to be altered at startup, as I suspect those will be wanted. But otherwise, this looks good to me!

ismaelsadeeq force-pushed on Jun 12, 2025

ismaelsadeeq commented at 10:33 am on June 12, 2025: member

This looks pretty complete now, nice work.

Thanks 👍🏾

Slightly sad it’s not wired up to the rpc for so I could do some live testing, but the unit tests confirm the expected behaviour.

Please do it is implemented in the complete PR #30157, the RPC changes is next to review If we manage to get this in.

Left a few comments about possibly allowing some of the mempool-based fee estimator constants to be altered at startup, as I suspect those will be wanted. But otherwise, this looks good to me!

I’ve responded to all comments and forced pushed from f79873fb512c20a97c5a6286118ef5b85df05027 to bad72f1a24585768f4bac937a17597879063400e df05027..bad72f1a to address others.

The changes are minor:

Updated variable name to match doc guideline and be correct
Improve comments correctness

in src/policy/fees/forecaster.h:26 in bad72f1a24 outdated

21+    ForecastType GetForecastType() const { return m_forecast_type; }
22+
23+    /**
24+     * @brief Returns the estimated fee rate for a package to confirm within the given target.
25+     * @param target Confirmation target in blocks.
26+     * @param conservative If true, returns a higher fee rate for greater confirmation probability.

willcl-ark commented at 10:03 am on June 17, 2025:

Just noting that, although currently we use this wording for conservative mode in estimatesmartfee rpc help:

0conservative estimates use a longer time horizon, making them
1less responsive to short-term drops in the prevailing fee market

I think the docstring wording makes sense here, as the time-horizon would not apply to a mempool-based estimator, where it would be expect to simply be higher.

ismaelsadeeq commented at 12:39 pm on June 17, 2025:

yep will update estimatesmartfee help text after it’s integrated to the forecaster manager

willcl-ark approved

willcl-ark commented at 11:55 am on June 17, 2025: member

ACK bad72f1a24585768f4bac937a17597879063400e

Reviewed the code and built and ran the tests. Also did various manual tests with commits picked from #30157.

This is a great step towards getting a native mempool-based fee estimator, along with providing a clean interface into which additional future fee estimators could more easily be added.

I’m still of the opinion that folks may want some of the constants here to be configuration options (for better or for worse), but that can easily be done in a follow-up.

I like that this change as currently structured doesn’t conflict with the main cluster mempool changes. Merging here does not require code changes there (other than perhaps a rebase), and upgrading the mempool-based estimator here to take advantage of cluster mempool in the future is a separate, independent change which can be done (or not) after cluster mempool is activated.

DrahtBot added the label Needs rebase on Jul 3, 2025

ismaelsadeeq force-pushed on Jul 7, 2025

ismaelsadeeq commented at 4:58 pm on July 7, 2025: member

Rebased to address conflict diff

DrahtBot removed the label Needs rebase on Jul 7, 2025

DrahtBot added the label Needs rebase on Jul 25, 2025

ismaelsadeeq force-pushed on Jul 29, 2025

DrahtBot removed the label Needs rebase on Jul 29, 2025

ismaelsadeeq requested review from willcl-ark on Aug 1, 2025

ismaelsadeeq requested review from polespinasa on Aug 1, 2025

ismaelsadeeq requested review from maflcko on Aug 1, 2025

ismaelsadeeq requested review from glozow on Aug 1, 2025

ismaelsadeeq requested review from frankomosh on Aug 1, 2025

frankomosh commented at 7:06 pm on August 12, 2025: contributor

ACK 26632b9; forecasting architecture is well designed. I tested locally and code successfully builds and tests pass.

DrahtBot added the label Needs rebase on Aug 13, 2025

ismaelsadeeq force-pushed on Aug 15, 2025

DrahtBot removed the label Needs rebase on Aug 15, 2025

ismaelsadeeq commented at 5:51 pm on August 19, 2025: member

I separate the refactoring commits (first four) into it’s own PR in #33218

Hence putting this in draft.

ismaelsadeeq marked this as a draft on Aug 19, 2025

in src/policy/fees/block_policy_estimator.cpp:742 in ba33047b6f outdated

737+        return result;
738+    }
739+    // Note: size can be any positive non-zero integer; the evaluated fee/size will result in the same fee rate,
740+    // and we only care that the fee rate remains consistent.
741+    int32_t size = 1000;
742+    result.feerate = FeeFrac(feerate.GetFee(size), size);

polespinasa commented at 5:56 pm on August 19, 2025:

Maybe after #32750 can use CFeeRate to keep consistency with the rest of the code.

ismaelsadeeq commented at 5:58 pm on August 19, 2025:

Yeah makes sense, they are basically identical :P

polespinasa commented at 6:07 pm on August 19, 2025: member

ACK ba33047b6f3ee468eab250c8515e92b595e08322 reviewed the code and build and run the tests locally. Adding a mempool-based fee estimation is a great step towards improving Core fee estimation.

DrahtBot added the label Needs rebase on Aug 21, 2025

ismaelsadeeq force-pushed on Sep 1, 2025

ismaelsadeeq commented at 7:20 pm on September 1, 2025: member

Rebased due to merge conflict.

DrahtBot removed the label Needs rebase on Sep 1, 2025

DrahtBot added the label Needs rebase on Oct 24, 2025

fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp

- Also remame the test suite name to match the new name.

252653f65d

fees: refactor: rename fees to block_policy_estimator

- Also move it to policy/fees and update the includes

a617e774a8

fees: rename fees_args to block_policy_estimator_args

- Also move them to policy/fees/ and update includes
- Note: the block_policy_estimator_args.h include in block_policy_estimator_args.cpp was done manually.

95b26ab0fb

fees: return current block height in `estimateSmartFee` 08d0d0be26

fees: add forecaster structures

i  - Forecaster abstract class is the base class of fee rate forecasters.
     Derived classes must provide concrete implementation
     of the virtual methods.

ii - ForecastResult struct represent the response returned by
     a fee rate forecaster.

iii - ForecastType will be used to identify forecasters.
      Each time a new forecaster is added, a corresponding
      enum value should be added to ForecastType.

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

289a5c14cb

fees: add ForecasterMan class

- Its a module for managing and utilising multiple
  fee rate forecasters to provide fee rate forecast.

- The ForecasterManager class allows for the registration of
  multiple fee rate forecasters.

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

2f72fd3cdb

fees: make block_policy_estimator a forecaster 66a4f626db

fees: add block policy estimator to forecaster manager

- This changes `CBlockPolicyEstimator` to a shared pointer
  this gives us three advantages.
   - Registering to validation interface using shared pointer
   - Scheduling block policy estimator flushes using shared pointer
   - Registering block policy estimator to forecaster_man

b9075e0bf0

fees: add `forecastTypeToString` method

- This method converts a ForecastType enum to its
  string representation.

2467e5b3f3

fees: add `CalculatePercentiles` function

- The CalculatePercentiles function, given
  a vector of feerates in the order they were added
  to the block, will return the 25th, 50th, 75th,
  and 95th percentile feerates.

- Also add a unit test for this function.

7f04b23e45

fees: add `MemPoolForecaster` class

- The mempool forecaster uses the unconfirmed transactions in the mempool
  to generate a fee rate that a package should have for it to confirm as soon as possible.

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

4f3d510f92

test: add mempool forecaster unit test 1f4a9f8abf

fees: cache `MemPoolPolicyEstimator` forecasts

- Provide new forecast only when the time delta from previous
  forecast is older than 30 seconds.

- This caching helps avoid the high cost of frequently generating block templates.

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

0edd89123e

fees: add `ForecastFeeRateFromForecasters` method

- Polls all registered forecasters and selects the lowest fee rate.

aa4efc7283

ismaelsadeeq force-pushed on Oct 27, 2025

DrahtBot removed the label Needs rebase on Oct 27, 2025

ismaelsadeeq commented at 5:35 pm on October 27, 2025: member

C.I is falling after a rebase due to merge of #33210 I have to update FeeEstimatorTestingSetup, I did that locally but will not push because I am working on some minor changes.

#33218 is not affected by the planned change at all.

Fees: add Fee rate Forecaster Manager #31664

Key Changes

Code Coverage & Benchmarks

Reviews

Conflicts