Skip to content

Conversation

@ismaelsadeeq
Copy link
Member

@ismaelsadeeq ismaelsadeeq commented Jan 15, 2025

Key Changes

  1. 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.
  2. 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
  3. 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
Copy link
Contributor

DrahtBot commented Jan 15, 2025

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:

  • #33199 (fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)
  • #31382 (kernel: Flush in ChainstateManager destructor by sedited)
  • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
  • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
  • #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.

@ismaelsadeeq
Copy link
Member Author

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

@ismaelsadeeq
Copy link
Member Author

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

@ismaelsadeeq ismaelsadeeq force-pushed the 01-2025-feerate-forecastman branch from c6b9440 to 9455f2f Compare April 13, 2025 14:19
@ismaelsadeeq
Copy link
Member Author

Forced pushed c6b9440..9455f2f0927014f2cc2495d432be635b58cc8fe2

I've made the following changes:

  1. Rebased on master.
  2. Reorganized the commits (refactor comes first).
  3. Squashed the structure-related commits into one.
  4. Addressed comments by @glozow.
  5. Removed some redundancy and things that seemed like YAGNI.
  6. 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.

@ismaelsadeeq ismaelsadeeq force-pushed the 01-2025-feerate-forecastman branch from 9455f2f to a665ff1 Compare April 15, 2025 09:59
@ismaelsadeeq ismaelsadeeq force-pushed the 01-2025-feerate-forecastman branch from a665ff1 to 9ae1585 Compare April 25, 2025 15:42
@polespinasa
Copy link
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.

@ismaelsadeeq
Copy link
Member Author

Rebased due to merge conflict.

@ismaelsadeeq
Copy link
Member Author

ismaelsadeeq commented Oct 27, 2025

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.

ismaelsadeeq and others added 9 commits October 29, 2025 19:23
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>
- 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>
- 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
- This method converts a ForecastType enum to its
  string representation.
- 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.
- 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>
- 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>
@ismaelsadeeq ismaelsadeeq force-pushed the 01-2025-feerate-forecastman branch from aa4efc7 to 34cf9e1 Compare October 29, 2025 19:24
- Polls all registered forecasters and selects the lowest fee rate.
@ismaelsadeeq
Copy link
Member Author

Closed in favour of #34075

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants