This PR does two things:
It prevents redundant writes to the fee_estimates.dat file 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 estimatefee category. It also ensures the log consistently includes only the full file path.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32748.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | Eunovo |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Possible typos and grammar issues:
drahtbot_id_4_m
🚧 At least one of the CI tasks failed.
Task fuzzer,address,undefined,integer, no depends: https://github.com/bitcoin/bitcoin/runs/44091090061
LLM reason (✨ experimental): The CI failure is caused by a thread-safety analysis error in policy_estimator.cpp due to unsafe mutex usage during a function call.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
559@@ -560,18 +560,18 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath
560 AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "rb")};
561
562 if (est_file.IsNull()) {
563- LogPrintf("%s is not found. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
564+ LogPrintf("%s is not found. Continue anyway.\n", fs::PathToString(m_estimation_filepath.filename()));
LogPrintf is deprecated, See https://github.com/bitcoin/bitcoin/blob/master/src/logging.h#L265-L266
Consisder using LogInfo instead
566 }
567
568 std::chrono::hours file_age = GetFeeEstimatorFileAge();
569 if (file_age > MAX_FILE_AGE && !read_stale_estimates) {
570- LogPrintf("Fee estimation file %s too old (age=%lld > %lld hours) and will not be used to avoid serving stale estimates.\n", fs::PathToString(m_estimation_filepath), Ticks<std::chrono::hours>(file_age), Ticks<std::chrono::hours>(MAX_FILE_AGE));
571+ LogPrintf("Fee estimation file %s too old (age=%lld > %lld hours) and will not be used to avoid serving stale estimates.\n", fs::PathToString(m_estimation_filepath.filename()), Ticks<std::chrono::hours>(file_age), Ticks<std::chrono::hours>(MAX_FILE_AGE));
- Also don’t log the full file path, just the filename.
What’s the reason for this? This reduces debugging information and bitcoin-core generally uses file path logging over filename logging.
Concept ACK https://github.com/bitcoin/bitcoin/pull/32748/commits/48a3e8f5778276dfde3d16ebcf5754721f0deaf1
Left some comments about logging.
959@@ -960,6 +960,13 @@ void CBlockPolicyEstimator::Flush() {
960
961 void CBlockPolicyEstimator::FlushFeeEstimates()
962 {
963+ {
964+ LOCK(m_cs_fee_estimator);
965+ // We should only flush the fee estimates if usable estimates are available.
966+ // This prevents redundant flushes e.g during IBD.
967+ if (!MaxUsableEstimate()) return;
968+ }
nit: could write this in a single line instead.
0if (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.
Taken, also please add 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:
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 .
- Also log the full file path of fee_estimates.dat consistently
not just the filename.
Labels
Needs rebase