in 74b764de510abb3538fb365a749bff4fe2b2c989 fees: fall back to block policy while the mempool estimator is not ready
nit: Probably cleaner IsReady(...) and then in extimator_man.cpp use if (!IsReady()). Feels a bit weird to have a function expecting a negative result.
Also probably worth making this function a member of MemPoolFeeRateEstimator so we can check here and in other parts of the code if necessary in the future if the mempool is ready and skip the other checks.
<details>
<summary> diff </summary>
$ git diff
diff --git a/src/policy/fees/estimator_man.cpp b/src/policy/fees/estimator_man.cpp
index e1ae7e8b03..5b207d4cd8 100644
--- a/src/policy/fees/estimator_man.cpp
+++ b/src/policy/fees/estimator_man.cpp
@@ -29,10 +29,11 @@ util::Expected<FeeRateEstimation, FeeRateEstimationError> FeeRateEstimatorManage
LogDebug(BCLog::ESTIMATEFEE, "%s", block_policy_estimate.error().reason);
return block_policy_estimate;
}
+ // If mempool fee rate estimator is not ready, fallback to block
+ // policy extimator.
+ if (!m_mempool_estimator->IsReady()) return block_policy_estimate;
auto mempool_estimate = m_mempool_estimator->EstimateFeeRate(conservative);
if (!mempool_estimate) {
- if (IsNotReady(mempool_estimate.error())) return block_policy_estimate;
- // When mempol fee rate estimator is ready, return the error.
// Callers can still request block policy explicitly.
auto mempool_error = EstimationError(mempool_estimate.error());
LogDebug(BCLog::ESTIMATEFEE, "%s", mempool_error.error().reason);
diff --git a/src/policy/fees/mempool_estimator.cpp b/src/policy/fees/mempool_estimator.cpp
index 35ccfcc5f7..42c53c0262 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/src/policy/fees/mempool_estimator.cpp
@@ -157,20 +157,6 @@ std::string_view MempoolEstimationFailureToString(MempoolEstimationFailure failu
assert(false);
}
-bool IsNotReady(MempoolEstimationFailure failure)
-{
- switch (failure) {
- case MempoolEstimationFailure::INSUFFICIENT_DATA:
- return true;
- case MempoolEstimationFailure::MEMPOOL_NOT_LOADED:
- case MempoolEstimationFailure::LOW_COVERAGE:
- case MempoolEstimationFailure::BLOCK_TEMPLATE_FAILED:
- return false;
- }
- // no default case, so the compiler can warn about missing cases
- assert(false);
-}
-
util::Unexpected<FeeRateEstimationError> EstimationError(MempoolEstimationFailure failure)
{
constexpr auto estimator_type{FeeRateEstimatorType::MEMPOOL_POLICY};
@@ -351,7 +337,7 @@ std::optional<MempoolEstimationFailure> MemPoolFeeRateEstimator::GetMempoolHealt
{
LOCK(cs);
const auto estimator_name{FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY)};
- if (m_prev_mined_blocks.size() < MEMPOOL_HEALTH_WINDOW_BLOCKS) {
+ if (!IsReady()) {
LogDebug(BCLog::ESTIMATEFEE, "%s: mempool health check failed; tracked_blocks=%s required_blocks=%s",
estimator_name, m_prev_mined_blocks.size(), MEMPOOL_HEALTH_WINDOW_BLOCKS);
return MempoolEstimationFailure::INSUFFICIENT_DATA;
diff --git a/src/policy/fees/mempool_estimator.h b/src/policy/fees/mempool_estimator.h
index 5475b37563..e4856c92fc 100644
--- a/src/policy/fees/mempool_estimator.h
+++ b/src/policy/fees/mempool_estimator.h
@@ -152,6 +152,8 @@ public:
//! Serialize mined-block stats without taking ownership of file.
//! Callers must explicitly close file and check for errors after writing.
bool Write(AutoFile& file) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
+ //! Checks if the estimator has enought block data making it ready.
+ bool IsReady() const { return m_prev_mined_blocks.size() >= MEMPOOL_HEALTH_WINDOW_BLOCKS; };
private:
void ReadFromDisk() EXCLUSIVE_LOCKS_REQUIRED(!cs);
diff --git a/src/test/mempool_fee_estimator_tests.cpp b/src/test/mempool_fee_estimator_tests.cpp
index eda6a69154..3e25d03866 100644
--- a/src/test/mempool_fee_estimator_tests.cpp
+++ b/src/test/mempool_fee_estimator_tests.cpp
@@ -337,14 +337,6 @@ BOOST_AUTO_TEST_CASE(MempoolFeeRateEstimator)
}
}
-BOOST_AUTO_TEST_CASE(is_not_ready)
-{
- BOOST_CHECK(IsNotReady(MempoolEstimationFailure::INSUFFICIENT_DATA));
- BOOST_CHECK(!IsNotReady(MempoolEstimationFailure::MEMPOOL_NOT_LOADED));
- BOOST_CHECK(!IsNotReady(MempoolEstimationFailure::LOW_COVERAGE));
- BOOST_CHECK(!IsNotReady(MempoolEstimationFailure::BLOCK_TEMPLATE_FAILED));
-}
-
BOOST_AUTO_TEST_CASE(mempool_reload_drops_stale_stats)
{
MemPoolFeeRateEstimator estimator{MempoolPolicyEstimatorPath(*m_node.args), *m_node.mempool, *m_node.chainman};
</details>
Or just remove the function at all and minimize the diff, it is only used once.
<details>
<summary> diff </summary>
$ git diff
diff --git a/src/policy/fees/estimator_man.cpp b/src/policy/fees/estimator_man.cpp
index e1ae7e8b03..bce8ef2368 100644
--- a/src/policy/fees/estimator_man.cpp
+++ b/src/policy/fees/estimator_man.cpp
@@ -31,7 +31,7 @@ util::Expected<FeeRateEstimation, FeeRateEstimationError> FeeRateEstimatorManage
}
auto mempool_estimate = m_mempool_estimator->EstimateFeeRate(conservative);
if (!mempool_estimate) {
- if (IsNotReady(mempool_estimate.error())) return block_policy_estimate;
+ if (MempoolEstimationFailure::INSUFFICIENT_DATA == mempool_estimate.error()) return block_policy_estimate;
// When mempol fee rate estimator is ready, return the error.
// Callers can still request block policy explicitly.
auto mempool_error = EstimationError(mempool_estimate.error());
diff --git a/src/policy/fees/mempool_estimator.cpp b/src/policy/fees/mempool_estimator.cpp
index 35ccfcc5f7..b36ad696ec 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/src/policy/fees/mempool_estimator.cpp
@@ -157,20 +157,6 @@ std::string_view MempoolEstimationFailureToString(MempoolEstimationFailure failu
assert(false);
}
-bool IsNotReady(MempoolEstimationFailure failure)
-{
- switch (failure) {
- case MempoolEstimationFailure::INSUFFICIENT_DATA:
- return true;
- case MempoolEstimationFailure::MEMPOOL_NOT_LOADED:
- case MempoolEstimationFailure::LOW_COVERAGE:
- case MempoolEstimationFailure::BLOCK_TEMPLATE_FAILED:
- return false;
- }
- // no default case, so the compiler can warn about missing cases
- assert(false);
-}
-
util::Unexpected<FeeRateEstimationError> EstimationError(MempoolEstimationFailure failure)
{
constexpr auto estimator_type{FeeRateEstimatorType::MEMPOOL_POLICY};
diff --git a/src/policy/fees/mempool_estimator.h b/src/policy/fees/mempool_estimator.h
index 5475b37563..2fd740763e 100644
--- a/src/policy/fees/mempool_estimator.h
+++ b/src/policy/fees/mempool_estimator.h
@@ -49,9 +49,6 @@ enum class MempoolEstimationFailure {
std::string_view MempoolEstimationFailureToString(MempoolEstimationFailure failure);
-//! Whether a caller should fall back to another estimator for this failure.
-bool IsNotReady(MempoolEstimationFailure failure);
-
//! Flatten a fee rate estimation failure into a fee rate estimation error.
util::Unexpected<FeeRateEstimationError> EstimationError(MempoolEstimationFailure failure);
diff --git a/src/test/mempool_fee_estimator_tests.cpp b/src/test/mempool_fee_estimator_tests.cpp
index eda6a69154..3e25d03866 100644
--- a/src/test/mempool_fee_estimator_tests.cpp
+++ b/src/test/mempool_fee_estimator_tests.cpp
@@ -337,14 +337,6 @@ BOOST_AUTO_TEST_CASE(MempoolFeeRateEstimator)
}
}
-BOOST_AUTO_TEST_CASE(is_not_ready)
-{
- BOOST_CHECK(IsNotReady(MempoolEstimationFailure::INSUFFICIENT_DATA));
- BOOST_CHECK(!IsNotReady(MempoolEstimationFailure::MEMPOOL_NOT_LOADED));
- BOOST_CHECK(!IsNotReady(MempoolEstimationFailure::LOW_COVERAGE));
- BOOST_CHECK(!IsNotReady(MempoolEstimationFailure::BLOCK_TEMPLATE_FAILED));
-}
-
BOOST_AUTO_TEST_CASE(mempool_reload_drops_stale_stats)
{
MemPoolFeeRateEstimator estimator{MempoolPolicyEstimatorPath(*m_node.args), *m_node.mempool, *m_node.chainman};
</details>
