Fee Estimation: change estimatesmartfee default mode to economical
#30275
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
Concept ACK 5a4a84a6809919561aec46f272f08ae57c0e386e. Slightly surprised no tests need to be changed.
Aside from seeming to overestimate much less, I think "economical" is much closer to what users would expect from a fee estimator. From #10589:
The logic used here is that any time a transaction signals opt-in-RBF and uses automatic fee estimation then it will use the non-conservative estimate. Transactions which do not signal opt-in-RBF will still use the default conservative estimate.
So accuracy of estimates aside, interpreting these settings as:
- "conservative" = I want to have very high confidence that this feerate is sufficient for my confirmation target, even if it means overestimating
- "economical" = I want to be reasonably confident about this feerate. I prefer not to overestimate and expect to be able to update it later with a fee-bump if needed
It is pretty normal/default to expect to be able to RBF the transaction later (for example, Core wallet defaults to signaling BIP125 since #25610), so it makes sense that the fee estimation settings should reflect this expectation.
ismaelsadeeq
force-pushed
the
06-2024-change-estimatesmartfee-default
branch
2 times, most recently
from
a4ec803 to
c308144
Compare
ismaelsadeeq
force-pushed
the
06-2024-change-estimatesmartfee-default
branch
from
c308144 to
41a2545
Compare
|
concept ACK could we return a new field that says the mode, then add a test for that to prevent regressions? |
Yes, I plan on addressing this 2ee3d77, in a follow-up PR: as suggested here #30275 (comment) along with some documentation updates proposed in #18217
Could you please clarify about the test what specific regressions you're concerned about? Thanks! |
A boolean flip where the intended behavior isn't followed and we are giving conservative again? |
Because the fee estimation functional test for
I added a simple deterministic test which test the default mode is |
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
better test than what I was asking for, thanks
ismaelsadeeq
force-pushed
the
06-2024-change-estimatesmartfee-default
branch
from
307ceb4 to
48faceb
Compare
|
ACK 48faceb |
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
Nice test! I just have a few comments
willcl-ark
left a comment
willcl-ark
left a comment
There was a problem hiding this comment.
Looks good to me, left a micro style nit if you end up touching to address @glozow 's comments.
It makes sense to default to more economical fee estimation by default, since users can use RBF (or "conservative" estimation) in those cases when it may be required. RBF opt-in has been enabled by default in Bitcoin Core wallets since ~2022 and there is a proposal to enable "full" RBF as default as mempool policy currently open: #30493 (previously #28132)
ismaelsadeeq
force-pushed
the
06-2024-change-estimatesmartfee-default
branch
from
48faceb to
b64c7b1
Compare
ismaelsadeeq
force-pushed
the
06-2024-change-estimatesmartfee-default
branch
from
b64c7b1 to
25bf86a
Compare
willcl-ark
left a comment
willcl-ark
left a comment
There was a problem hiding this comment.
ACK 25bf86a
Looks good to me with those nits fixed 👍🏼
|
reACK 25bf86a tests make me feel much better about this thanks |
|
Will need a release note. |
Thanks, I will add it in the follow-up! |
|
Release note being added in #30525. |
fa2f269 [rpc, fees]: add more detail on the fee estimation modes (ismaelsadeeq) 6e7e620 [doc]: add `30275` release notes (ismaelsadeeq) Pull request description: This PR: 1. Adds release notes for #30275 2. Describe fee estimation modes in RPC help texts ACKs for top commit: achow101: ACK fa2f269 glozow: ACK fa2f269 willcl-ark: ACK fa2f269 tdb3: re ACK fa2f269 Tree-SHA512: b8ea000b599297b954dc770137c29b47153e68644c58550a73e34b74ecb8b65e78417875481efdfdf6aab0018a9cd1d90d8baa5a015e70aca0975f6e1dc9598c
8 participants
Fixes #30009
This PR changes the
estimatesmartfeedefault mode toeconomical.This was also suggested on IRC https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-04-26#1021609
conservativemode: This is theestimatesmartfeeRPC mode which considers a longer history of blocks. It potentially returns a higher fee rate and is more likely to be sufficient for the desired target, but it is not as responsive to short-term drops in the prevailing fee market.economicalmode: This is theestimatesmartfeeRPC mode where estimates are potentially lower and more responsive to short-term drops in the prevailing fee market.Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.
For an in-depth analysis of how significantly the
conservativemode overestimates, seehttps://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.