Fee Estimation via Fee rate Forecasters #30157
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/30157. 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. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
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. |
Correct. The rationale behind this set of changes can be summed up briefly as follows:
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.
In this changeset we take the approach of using the lowest result from all "confident" 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 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. |
ismaelsadeeq
force-pushed
the
new-fee-estimator
branch
2 times, most recently
from
f6748c9 to
6fb2e37
Compare
ismaelsadeeq
force-pushed
the
new-fee-estimator
branch
from
6fb2e37 to
64ebda7
Compare
ismaelsadeeq
force-pushed
the
new-fee-estimator
branch
from
64ebda7 to
a28d079
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
ismaelsadeeq
force-pushed
the
new-fee-estimator
branch
from
a28d079 to
df7c856
Compare
ismaelsadeeq
force-pushed
the
new-fee-estimator
branch
from
8c16919 to
b3a121d
Compare
…stimator.{h,cpp}`
1a7fb5e fees: return current block height in estimateSmartFee (ismaelsadeeq)
ab49480 fees: rename fees_args to block_policy_estimator_args (ismaelsadeeq)
06db08a fees: refactor: rename fees to block_policy_estimator (ismaelsadeeq)
6dfdd7e fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp (ismaelsadeeq)
Pull request description:
This PR is a simple refactoring that does four things:
1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.
**Motivation**
In preparation for adding a new fee estimator, the `fees` directory is created so we can organize code into `block_policy_estimator` and `mempool` because
a) It would be clunky to add more code directly under `fees`.
b) Having `policy/fees.{h,cpp}` and `policy/mempool.{h,cpp}` would also be undesirable.
Therefore, it makes sense to structure the it as `policy/fees/block_policy_estimator`, `policy/fees/mempool`, etc.
Hence test file were also updated accordingly.
The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.
ACKs for top commit:
maflcko:
re-ACK 1a7fb5e 🐤
polespinasa:
re ACK 1a7fb5e
willcl-ark:
ACK 1a7fb5e
janb84:
re ACK 1a7fb5e
Tree-SHA512: fef7ace2a9f262ec0361fb7a46df5108afc46b5c4b059caadf2fd114740aefbb2592389d11646c13d0e28bf0ef2cfcfbab3e659c4d4288b8ebe64725fd1963c0
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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
- Polls all registered forecasters and selects the lowest fee rate.
- Given a confirmation target, we use fee estimator module that call all available fee estimator forcasters and return the lowest fee rate that if a transaction use will likely confirm in a given confirmation target. - This also provides backward compatibility Co-authored-by: willcl-ark <[email protected]>
- Also add functional test
ismaelsadeeq
force-pushed
the
new-fee-estimator
branch
from
b3a121d to
fa39061
Compare
|
closed in favour of #34075 |
12 participants
This PR is the complete implementation of #30392 it aim to improve Bitcoin Core Fee Estimation by:
BlockPolicyEstimator. This issue has been documented and acknowledged by various sources:The detailed design document for this PR can be found here.
Please note that the design is subject to change as we refine the approach.
This is a collaborative effort with @willcl-ark and incorporates insights from other contributors.