https://github.com/bitcoin/bitcoin/pull/36182/changes/7f46e2749b3831401be6202032e765bd4159e863 (fees: enable the mempool estimator to return a typed failure enum)
nit: I think the semantics of util::Expected<void, MempoolEstimationFailure> are better than std::optional here. The current approach seems to me to invert the convention around optional which is some value if success, nullopt if there was an error, it might look like e.g. (this is combined with the suggestion below of deleting IsMempoolHealthy()
diff --git a/src/policy/fees/mempool_estimator.cpp b/src/policy/fees/mempool_estimator.cpp
index 35f5815bb6..24f0ad16e2 100644
--- a/src/policy/fees/mempool_estimator.cpp
+++ b/src/policy/fees/mempool_estimator.cpp
@@ -326,14 +326,14 @@ void MemPoolFeeRateEstimator::MempoolTxsRemovedForBlock(const std::shared_ptr<co
// the coverage ratio as a representative mempool health signal.
static constexpr uint64_t MIN_REPRESENTATIVE_WINDOW_WEIGHT{DEFAULT_BLOCK_MAX_WEIGHT};
-std::optional<MempoolEstimationFailure> MemPoolFeeRateEstimator::GetMempoolHealthCheck() const
+util::Expected<void, MempoolEstimationFailure> MemPoolFeeRateEstimator::GetMempoolHealthCheck() const
{
LOCK(cs);
const auto estimator_name{FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY)};
if (m_prev_mined_blocks.size() < MEMPOOL_HEALTH_WINDOW_BLOCKS) {
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;
+ return util::Unexpected{MempoolEstimationFailure::INSUFFICIENT_DATA};
}
uint64_t total_block_weight{0};
uint64_t total_removed_weight{0};
@@ -348,7 +348,7 @@ std::optional<MempoolEstimationFailure> MemPoolFeeRateEstimator::GetMempoolHealt
if (total_block_weight < MIN_REPRESENTATIVE_WINDOW_WEIGHT) {
LogDebug(BCLog::ESTIMATEFEE, "%s: mempool health check passed; low activity, total_block_weight=%s minimum=%s",
estimator_name, total_block_weight, MIN_REPRESENTATIVE_WINDOW_WEIGHT);
- return std::nullopt;
+ return {};
}
const double representation_ratio = static_cast<double>(total_removed_weight) / total_block_weight;
LogDebug(BCLog::ESTIMATEFEE,
@@ -360,8 +360,8 @@ std::optional<MempoolEstimationFailure> MemPoolFeeRateEstimator::GetMempoolHealt
total_block_weight,
representation_ratio,
MEMPOOL_REPRESENTATION_THRESHOLD);
- if (representation_ratio < MEMPOOL_REPRESENTATION_THRESHOLD) return MempoolEstimationFailure::LOW_COVERAGE;
- return std::nullopt;
+ if (representation_ratio < MEMPOOL_REPRESENTATION_THRESHOLD) return util::Unexpected{MempoolEstimationFailure::LOW_COVERAGE};
+ return {};
}
util::Expected<FeeRateEstimation, MempoolEstimationFailure> MemPoolFeeRateEstimator::EstimateFeeRate(bool conservative) const
@@ -370,8 +370,8 @@ util::Expected<FeeRateEstimation, MempoolEstimationFailure> MemPoolFeeRateEstima
if (!m_mempool.GetLoadTried()) {
return util::Unexpected{MempoolEstimationFailure::MEMPOOL_NOT_LOADED};
}
- if (auto health_failure{GetMempoolHealthCheck()}) {
- return util::Unexpected{*health_failure};
+ if (const auto health_check{GetMempoolHealthCheck()}; !health_check) {
+ return util::Unexpected{health_check.error()};
}
// The estimator lock is not held while building a block template, so
// in a rare edge case concurrent callers may duplicate work.
diff --git a/src/policy/fees/mempool_estimator.h b/src/policy/fees/mempool_estimator.h
index 3f6ad58aa7..cc110f2fc1 100644
--- a/src/policy/fees/mempool_estimator.h
+++ b/src/policy/fees/mempool_estimator.h
@@ -137,9 +137,7 @@ public:
unsigned int block_height)
EXCLUSIVE_LOCKS_REQUIRED(!cs);
//! Return INSUFFICIENT_DATA or LOW_COVERAGE if recent mined blocks fail the health check; otherwise, return nullopt.
- std::optional<MempoolEstimationFailure> GetMempoolHealthCheck() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
- //! Checks if recent mined blocks indicate a healthy mempool state.
- bool IsMempoolHealthy() const EXCLUSIVE_LOCKS_REQUIRED(!cs) { return !GetMempoolHealthCheck().has_value(); }
+ util::Expected<void, MempoolEstimationFailure> GetMempoolHealthCheck() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
void FlushMinedBlockStats() EXCLUSIVE_LOCKS_REQUIRED(!cs);
//! Deserialize mined-block stats without taking ownership of file.
bool Read(AutoFile& file) EXCLUSIVE_LOCKS_REQUIRED(!cs);
<details><summary>And the test changes, which are mostly repetitive:</summary>
diff --git a/src/test/mempool_fee_estimator_tests.cpp b/src/test/mempool_fee_estimator_tests.cpp
index 98efb2b7c2..e913735a18 100644
--- a/src/test/mempool_fee_estimator_tests.cpp
+++ b/src/test/mempool_fee_estimator_tests.cpp
@@ -140,8 +140,8 @@ BOOST_AUTO_TEST_CASE(MempoolFeeRateEstimator)
}
m_node.mempool->SetLoadTried(true);
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
- BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck() == MempoolEstimationFailure::INSUFFICIENT_DATA);
+ auto health = mempool_estimator.GetMempoolHealthCheck();
+ BOOST_CHECK(!health && health.error() == MempoolEstimationFailure::INSUFFICIENT_DATA);
{
const auto result = mempool_estimator.EstimateFeeRate(/*conservative=*/true);
BOOST_CHECK(!result);
@@ -156,7 +156,7 @@ BOOST_AUTO_TEST_CASE(MempoolFeeRateEstimator)
/*removed_txs_weight=*/0,
/*block_txs_weight=*/0,
custom_height);
- BOOST_CHECK(!custom_mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(!custom_mempool_estimator.GetMempoolHealthCheck());
}
{
const int64_t low_activity_weight{1000};
@@ -165,7 +165,7 @@ BOOST_AUTO_TEST_CASE(MempoolFeeRateEstimator)
// Below one block worth of total activity across the full window, even
// poor coverage in the only non-empty block is too noisy to reject the
// mempool as unhealthy.
- BOOST_CHECK(custom_mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(custom_mempool_estimator.GetMempoolHealthCheck());
}
size_t block_count = 1;
const int64_t weight{DEFAULT_BLOCK_MAX_WEIGHT / 2};
@@ -174,67 +174,67 @@ BOOST_AUTO_TEST_CASE(MempoolFeeRateEstimator)
while (block_count <= MEMPOOL_HEALTH_WINDOW_BLOCKS) {
AddRemovedBlock(mempool_estimator, weight, weight, height);
if (block_count < MEMPOOL_HEALTH_WINDOW_BLOCKS) {
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(!mempool_estimator.GetMempoolHealthCheck());
}
block_count += 1;
}
// Total txs weight ~11999k WU (~3.0 blocks), removed txs ~11999k WU (~3.0 blocks); coverage = 100%.
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// Adding a single underrepresented block will not make the mempool unhealthy
// while the window coverage remains above the threshold.
AddRemovedBlock(mempool_estimator, weight / 2, weight, height);
// Total txs weight ~11999k WU (~3.0 blocks), removed txs ~10999k WU (~2.75 blocks); coverage = ~92%.
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// Empty block
// Total txs weight ~9999k WU (~2.5 blocks), removed txs ~8999k WU (~2.25 blocks); coverage = 90%.
AddRemovedBlock(mempool_estimator, 0, 0, height);
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// Total txs weight ~9999k WU (~2.5 blocks), removed txs ~7999k WU (~2.0 blocks); coverage = 80%.
AddRemovedBlock(mempool_estimator, weight / 2, weight, height);
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// Total txs weight ~9999k WU (~2.5 blocks), removed txs ~7000k WU (~1.75 blocks); coverage = 70%.
AddRemovedBlock(mempool_estimator, weight / 2, weight, height);
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
- BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck() == MempoolEstimationFailure::LOW_COVERAGE);
+ health = mempool_estimator.GetMempoolHealthCheck();
+ BOOST_CHECK(!health && health.error() == MempoolEstimationFailure::LOW_COVERAGE);
block_count = 1;
while (block_count <= 3) {
AddRemovedBlock(mempool_estimator, weight, weight, height);
if (block_count < 3) {
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(!mempool_estimator.GetMempoolHealthCheck());
}
block_count += 1;
}
// Total txs weight ~9999k WU (~2.5 blocks), removed txs ~7999k WU (~2.0 blocks); coverage = 80%.
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// Reorg out and replace the last block. Replacing the tip block should keep a full
// healthy window when the replacement block has good mempool representation.
height -= 1;
AddRemovedBlock(mempool_estimator, weight, weight, height);
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// Reorg out the last two blocks. The estimator should discard the stale suffix,
// become temporarily unhealthy due to having fewer than MEMPOOL_HEALTH_WINDOW_BLOCKS stats,
// then recover after the replacement chain catches up.
height -= 2;
AddRemovedBlock(mempool_estimator, weight, weight, height);
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(!mempool_estimator.GetMempoolHealthCheck());
AddRemovedBlock(mempool_estimator, weight, weight, height);
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
// A forward height gap (e.g. stale persisted stats after an unclean shutdown
// while the chain advanced) resets the tracked window entirely; the estimator
// stays unhealthy until a full window of contiguous blocks is seen again.
height += 3;
AddRemovedBlock(mempool_estimator, weight, weight, height);
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(!mempool_estimator.GetMempoolHealthCheck());
for (size_t i = 1; i < MEMPOOL_HEALTH_WINDOW_BLOCKS; ++i) {
AddRemovedBlock(mempool_estimator, weight, weight, height);
if (i < MEMPOOL_HEALTH_WINDOW_BLOCKS - 1) {
- BOOST_CHECK(!mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(!mempool_estimator.GetMempoolHealthCheck());
}
}
- BOOST_CHECK(mempool_estimator.IsMempoolHealthy());
+ BOOST_CHECK(mempool_estimator.GetMempoolHealthCheck());
{
LOCK(m_node.mempool->cs);
BOOST_CHECK_EQUAL(m_node.mempool->GetTotalTxSize(), 0);
