Fees: add Fee rate Forecaster Manager #31664
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31664. ReviewsSee 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
ba0bede to
c6b9440
Compare
|
There is going to be a review club tomorrow on this PR |
|
I will update the PR to address recent comments, I also want to remove some redundancy and restructure the commits. |
c6b9440 to
9455f2f
Compare
|
Forced pushed c6b9440..9455f2f I've made the following changes:
|
9455f2f to
a665ff1
Compare
a665ff1 to
9ae1585
Compare
9ae1585 to
75e6bb5
Compare
|
ACK 26632b9; forecasting architecture is well designed. I tested locally and code successfully builds and tests pass. |
- Also remame the test suite name to match the new name.
- Also move it to policy/fees and update the includes
- 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.
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>
- Polls all registered forecasters and selects the lowest fee rate.
26632b9 to
ba33047
Compare
|
I separate the refactoring commits (first four) into it's own PR in #33218 Hence putting this in draft. |
| // Note: size can be any positive non-zero integer; the evaluated fee/size will result in the same fee rate, | ||
| // and we only care that the fee rate remains consistent. | ||
| int32_t size = 1000; | ||
| result.feerate = FeeFrac(feerate.GetFee(size), size); |
There was a problem hiding this comment.
Maybe after #32750 can use CFeeRate to keep consistency with the rest of the code.
There was a problem hiding this comment.
Yeah makes sense, they are basically identical :P
|
ACK ba33047 reviewed the code and build and run the tests locally. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
The primary addition is a
ForecasterManagerthat coordinates multiple forecasting strategies to be able to provide better transaction fee rate predictions.Key Changes
Refactoring of
CBlockPolicyEstimatorfeestoblock_policy_estimatorfor clarity.fees_argstoblock_policy_estimator_argsfor claritypolicy_fee_teststofeerounder_testsalso for clarity.Addition of Fee Rate Forecasting Utility Structures
Forecasterabstract class, serving as the base class for all fee rate forecasters.ForecastResult(the response from aForecaster) with all metadata.ForecastTypeenum, identify and distinguish forecasters.Introduce
FeeRateForecasterManagerclassFeeRateForecasterManagerclass, responsible for managing all fee rate forecasters, includingCBlockPolicyEstimator, which is now a forecaster.FeeRateForecasterManagerinstead ofCBlockPolicyEstimator.CBlockPolicyEstimatorinstance fromunique_ptrto ashared_pointer, allowing it to be registered in the validation interface without requiring explicit unregistration during shutdown (current master behaviour). Registering forCBlockPolicyEstimatorflushes also get ashared_pointer.CBlockPolicyEstimatorthroughFeeRateForecasterManagerfor compatibility withestimateSmartFee, friends, and node interfaces.CBlockPolicyEstimatorto inherit from theForecasterclass and implement the pure abstract classesIntroduce
MempoolForecasterclassMempoolForecaster, 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.The
FeeRateForecasterManagernow includes a methodForecastFeeRateFromForecasters. 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 theFeeRateForecasterManageris 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.