fees: prevent redundant estimates flushes #32748
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/32748. 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
21746c6 to
48a3e8f
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
48a3e8f to
458fdac
Compare
There was a problem hiding this comment.
Addressed comments by @Eunovo thanks for review 21f0deaf1..458fdac
458fdac to
4a0672f
Compare
src/policy/fees.cpp
Outdated
| { | ||
| LOCK(m_cs_fee_estimator); | ||
| // We should only flush the fee estimates if usable estimates are available. | ||
| // This prevents redundant flushes e.g during IBD. | ||
| if (!MaxUsableEstimate()) return; | ||
| } |
There was a problem hiding this comment.
nit: could write this in a single line instead.
if (WITH_LOCK(m_cs_fee_estimator, return !MaxUsableEstimate())) return;Also, it would be nice to explain why MaxUsableEstimate returning 0 means "nothing to do". It is not immediately clear to me without checking the MaxUsableEstimate internals.
There was a problem hiding this comment.
Taken, also added an explanation.
I also note a behavior change introduced by this. It is possible that we may track some unconfirmed transactions and, in rare cases, see some unconfirmed transactions get evicted. However, since we have no confirmations yet, if we shut down in this state, we will lose that data and have to start from scratch. To mitigate this, we need a way to detect when we are in such a state.
I see two possible approaches:
- Brute-force check : Go through all the stats to ensure we have no useful data.
- Smart tracking : Add some of flag or tracker to each stat that indicates whether there is any useful data available. This would require extra processing when adding transactions to the mempool and, in rare cases, when transactions are evicted from the mempool.
Also related to this: checking whether MaxUsableEstimate is non-zero is not sufficient to decide whether we should avoid updating the unconfirmed circular buffer or decaying all exponential averages.
Additionally, simply tracking whether we have ever seen an unconfirmed transaction or a failure is not enough. There is an edge case where you may have seen that event,but if the relevant data has since decayed, you could end up updating with effectively empty stats. The indicator would need to account for data decay to provide an accurate signal.
Given this, I am starting to doubt whether the performance gain is worth the added complexity cc @l0rinc .
4a0672f to
e2539c8
Compare
e2539c8 to
45d35d3
Compare
- Also log the full file path of fee_estimates.dat consistently not just the filename.
45d35d3 to
3a4a4ba
Compare
This PR does two things:
It prevents redundant writes to the
fee_estimates.datfile when there is no usable data available. For example, during IBD, blocks are still being connected, but unnecessary flushes still occur every hour.It updates the flushing log to use debug-level logging under the
estimatefeecategory. It also ensures the log consistently includes only the full file path.