fees: validate persisted mempool policy estimator data after restoration #36095

w0xlt commented at 12:11 AM on August 27, 2026: contributor

The mempool policy estimator reloads its mined-block statistics separately from mempool.dat. Currently, those statistics are retained even when mempool persistence is disabled, mempool.dat is missing, or most snapshot transactions are not restored. This could make an empty or unrepresentative mempool appear healthy and produce misleading fee estimates.

This PR tracks the snapshot’s total and restored transaction weights during loading. Persisted estimator data is retained only when loading succeeds and the restored weight meets the existing mempool representation threshold. The data is preserved when startup is interrupted, and is neither read nor written when -persistmempool=0.

Tests cover partial restoration, disabled persistence, a missing snapshot, and expired snapshot transactions.

DrahtBot added the label TX fees and policy on Aug 27, 2026

DrahtBot commented at 12:11 AM on August 27, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36095.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	sedited, stickies-v
Approach ACK	ismaelsadeeq, musaHaruna

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36167 ([RFC] Enable -Wunused by fanquake)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#33854 (fix assumevalid is ignored during reindex by Eunovo)

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.

sedited requested review from ismaelsadeeq on Aug 27, 2026

sedited commented at 9:23 AM on August 27, 2026: contributor

Concept ACK

sedited added this to the milestone 32.0 on Aug 27, 2026

in src/node/mempool_persist.cpp:44 in 50d8d6e36c outdated

  40 | @@ -40,14 +41,14 @@ namespace node {
  41 |  static const uint64_t MEMPOOL_DUMP_VERSION_NO_XOR_KEY{1};
  42 |  static const uint64_t MEMPOOL_DUMP_VERSION{2};
  43 |  
  44 | -bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active_chainstate, ImportMempoolOptions&& opts)
  45 | +MempoolLoadResult LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active_chainstate, ImportMempoolOptions&& opts)

rustaceanrob commented at 9:56 AM on August 27, 2026:

If we are introducing a new return type, I think this may as well be util::Expected<MempoolRestoreStats, MempoolRestoreError>. There are many possible error cases here and all return {}

w0xlt commented at 9:33 PM on August 27, 2026:

@rustaceanrob Thanks for the suggestion. I agree that util::Expected could make sense if LoadMempool() also exposed structured failure reasons, but I think that would be a broader refactor.

This PR change has two independent outcomes: whether loading completed successfully and optional restoration statistics, which are collected only when needed.

An Expected<MempoolRestoreStats, MempoolRestoreError> cannot represent a successful load where statistics were intentionally not collected. With the current API, that would require something like Expected<std::optional<MempoolRestoreStats>, MempoolRestoreError> or collecting the statistics unconditionally.

The current callers also do not currently consume distinct failure reasons. Startup only needs to know whether loading completed, with interruption handled separately, while importmempool reports a generic error and directs users to the debug log.

I would prefer to preserve the existing error semantics in this PR. A follow-up could introduce structured load errors and decide whether callers such as importmempool should expose them. Does that sound reasonable?

rustaceanrob commented at 7:53 AM on August 28, 2026:

Sure, seems fine to leave for later, and can proceed as is.

FWIW, for the future, I think Expected<std::optional<MempoolRestoreStats>, MempoolRestoreError> is a very clear signal in the function signature of what exactly is happening. As you explained, we may have mempool restore stats but they are optional, and we may also fail for some number of reasons. The return type expresses that directly.

w0xlt commented at 8:32 AM on August 31, 2026:

With @ismaelsadeeq’s simplification, util::Expected became a much cleaner fit because the successful result is now just the snapshot weight. LoadMempool() now returns util::Expected<uint64_t, MempoolLoadError>. I’ve also added you as a co-author.

in src/policy/fees/mempool_estimator.h:41 in 50d8d6e36c

  35 | @@ -36,6 +36,10 @@ constexpr std::chrono::seconds CACHE_LIFE{7};
  36 |  // Constants for mempool sanity checks.
  37 |  constexpr size_t MEMPOOL_HEALTH_WINDOW_BLOCKS = 6;
  38 |  constexpr double MEMPOOL_REPRESENTATION_THRESHOLD = 0.75;
  39 | +//! Reuse persisted mined-block statistics only when the restored snapshot
  40 | +//! meets the mempool representation threshold.
  41 | +constexpr double MEMPOOL_SNAPSHOT_RESTORATION_THRESHOLD{

ismaelsadeeq commented at 1:14 PM on August 27, 2026:

In "fees: validate persisted mempool estimator data after restoration" 50d8d6e36ca3f44d10f0c20c271fde5a06e565cc

I think we should not reuse the MEMPOOL_REPRESENTATION_THRESHOLD constant here just copy the .75 value there. The representations are orthogonal.

w0xlt commented at 9:12 PM on August 27, 2026:

Done. Thanks,

ismaelsadeeq commented at 1:14 PM on August 27, 2026: member

Concept ACK

I think this can be simplified. We can drop tracking the wtxids of the whole mempool (and the second GetIter pass) by accumulating the weight of the persisted transactions during load, then, after loading, querying the mempool's total transaction weight (can be cached, but in the diff below it is computed in-flight) and passing both weights to the mempool fee rate estimator.

diff --git a/src/init.cpp b/src/init.cpp
index aa2a61fac1..8772eac736 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -2136,14 +2136,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             const auto load_result{LoadMempool(
                 *pool,
                 ShouldPersistMempool(args) ? MempoolPath(args) : fs::path{},
-                chainman.ActiveChainstate(),
-                {.collect_restore_stats = node.fee_estimator_man != nullptr})};
+                chainman.ActiveChainstate(), {})};
             if (node.fee_estimator_man && !chainman.m_interrupt) {
-                const auto restore_stats{load_result.restore_stats.value_or(node::MempoolRestoreStats{})};
+                // The mempool started empty, so its total weight after loading is
+                // effectively the portion of the snapshot that was restored.
+                const uint64_t restored_weight{WITH_LOCK(pool->cs, return pool->GetTotalTxWeight())};
                 node.fee_estimator_man->MempoolLoadCompleted(
-                    load_result.success && load_result.restore_stats.has_value(),
-                    restore_stats.total_tx_weight,
-                    restore_stats.restored_tx_weight);
+                    load_result.success, load_result.snapshot_weight, restored_weight);
             }
             pool->SetLoadTried(!chainman.m_interrupt);
         }
diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp
index 04bd414ca0..20c683d78c 100644
--- a/src/node/mempool_persist.cpp
+++ b/src/node/mempool_persist.cpp
@@ -57,8 +57,7 @@ MempoolLoadResult LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chain
     int64_t already_there = 0;
     int64_t unbroadcast = 0;
     const auto now{NodeClock::now()};
-    std::vector<Wtxid> snapshot_wtxids;
-    uint64_t total_snapshot_tx_weight{0};
+    uint64_t snapshot_weight{0};

     try {
         uint64_t version;
@@ -94,10 +93,7 @@ MempoolLoadResult LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chain
             file >> TX_WITH_WITNESS(tx);
             file >> nTime;
             file >> nFeeDelta;
-            if (opts.collect_restore_stats) {
-                snapshot_wtxids.push_back(tx->GetWitnessHash());
-                total_snapshot_tx_weight += static_cast<uint64_t>(GetTransactionWeight(*tx));
-            }
+            snapshot_weight += static_cast<uint64_t>(GetTransactionWeight(*tx));

             if (opts.use_current_time) {
                 nTime = TicksSinceEpoch<std::chrono::seconds>(now);
@@ -154,20 +150,9 @@ MempoolLoadResult LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chain
     }

     LogInfo("Imported mempool transactions from file: %i succeeded, %i failed, %i expired, %i already there, %i waiting for initial broadcast\n", count, failed, expired, already_there, unbroadcast);
-    std::optional<MempoolRestoreStats> restore_stats;
-    if (opts.collect_restore_stats) {
-        restore_stats.emplace();
-        restore_stats->total_tx_weight = total_snapshot_tx_weight;
-        LOCK(pool.cs);
-        for (const auto& wtxid : snapshot_wtxids) {
-            if (const auto it{pool.GetIter(wtxid)}) {
-                restore_stats->restored_tx_weight += static_cast<uint64_t>((*it)->GetTxWeight());
-            }
-        }
-    }
     return {
         .success = true,
-        .restore_stats = std::move(restore_stats),
+        .snapshot_weight = snapshot_weight,
     };
 }

diff --git a/src/node/mempool_persist.h b/src/node/mempool_persist.h
index 8b9858a839..3bdc0fdd59 100644
--- a/src/node/mempool_persist.h
+++ b/src/node/mempool_persist.h
@@ -8,7 +8,6 @@
 #include <util/fs.h>

 #include <cstdint>
-#include <optional>

 class Chainstate;
 class CTxMemPool;
@@ -25,22 +24,13 @@ struct ImportMempoolOptions {
     bool use_current_time{false};
     bool apply_fee_delta_priority{true};
     bool apply_unbroadcast_set{true};
-    //! Collect final restored transaction weight for startup health validation.
-    bool collect_restore_stats{false};
-};
-
-struct MempoolRestoreStats {
-    //! Total transaction weight in the persisted mempool snapshot.
-    uint64_t total_tx_weight{0};
-    //! Weight of snapshot transactions present after loading finishes.
-    uint64_t restored_tx_weight{0};
 };

 struct MempoolLoadResult {
     //! Whether the mempool file was successfully deserialized.
     bool success{false};
-    //! Populated only when restore statistics were requested.
-    std::optional<MempoolRestoreStats> restore_stats;
+    //! Total weight of the transactions contained in the snapshot file.
+    uint64_t snapshot_weight{0};
 };

 /** Import the file and attempt to add its contents to the mempool. */
diff --git a/src/txmempool.h b/src/txmempool.h
index 7d7aafde78..8da1ce6456 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -504,6 +504,18 @@ public:
         return totalTxSize;
     }

+    //! Sum of all mempool tx's weights (BIP 141). Computed in flight by walking
+    //! the pool, so this method should not be called in hot paths.
+    uint64_t GetTotalTxWeight() const EXCLUSIVE_LOCKS_REQUIRED(cs)
+    {
+        AssertLockHeld(cs);
+        uint64_t total_weight{0};
+        for (const auto& entry : mapTx) {
+            total_weight += entry.GetTxWeight();
+        }
+        return total_weight;
+    }
+
     CAmount GetTotalFee() const EXCLUSIVE_LOCKS_REQUIRED(cs)
     {
         AssertLockHeld(cs);

</details>

w0xlt force-pushed on Aug 27, 2026

DrahtBot added the label CI failed on Aug 27, 2026

DrahtBot removed the label CI failed on Aug 27, 2026

w0xlt commented at 9:11 PM on August 27, 2026: contributor

@ismaelsadeeq Thanks for the suggestion. If I understand correctly, this relies on the mempool containing only transactions from the snapshot until loading finishes. I am not sure if Bitcoin Core currently guarantees that.

For example, wallets can submit transactions while the background mempool load is still running. The existing loader explicitly accounts for this possibility:

mempool may contain the transaction already, e.g. from wallet(s) having loaded it while we were processing mempool transactions

See src/node/mempool_persist.cpp:116.

Consequently, the final mempool weight can include transactions that were not in mempool.dat and overstate the restored snapshot weight. Subtracting the initial mempool weight would not fully solve this because transactions can be inserted, removed, replaced, or evicted during loading.

Or am I missing something?

ismaelsadeeq commented at 9:30 AM on August 28, 2026: member

Or am I missing something?

Correct, the mempool total can include wallet txs that were not in mempool.dat spapshot. But I don't think it's a problem.

Most of what a wallet re-adds is already in the persisted snapshot — that's the already_there case in the comment — so it doesn't inflate the total. The only genuinely extra txs are wallet ones that were never persisted, which is most likely a small set and won't occur frequently.

The effect, as you mentioned, is that it pushes the ratio up, so the worst case is we keep stats we could have dropped (user-wise effect they see a low fee rate estimate, because some high fee rate txs are not seen now); we never drop good ones, which is the case we actually care about.

Also worth keeping in mind this is a heuristic check, and the mempool estimator only looks at the top block of the mempool anyway, not the whole thing so chasing exact whole-mempool weight is more precision than needed.

So it would be nice if we keep this simpler approach.

bitcoin deleted a comment on Aug 28, 2026

w0xlt force-pushed on Aug 31, 2026

w0xlt commented at 8:36 AM on August 31, 2026: contributor

@ismaelsadeeq Thanks for the clarification. I applied your proposed simplification, using post_load_mempool_weight and retention_ratio to make clear that this is a permissive heuristic rather than an exact measurement of restored snapshot weight. I’ve also added you as a co-author.

w0xlt commented at 8:36 AM on August 31, 2026: contributor

CI error seems unrelated.

DrahtBot added the label CI failed on Aug 31, 2026

DrahtBot commented at 8:50 AM on August 31, 2026: contributor

🚧 At least one of the CI tasks failed. Task previous releases: https://github.com/bitcoin/bitcoin/actions/runs/33372693809/job/99427161856 LLM reason (✨ experimental): CI failed due to a GCC 12 internal compiler error (segmentation fault) while compiling txmempool/tinyformat.h during the build.

<details><summary>Hints</summary>

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.

</details>

DrahtBot removed the label CI failed on Aug 31, 2026

ismaelsadeeq commented at 2:21 PM on September 2, 2026: member

Approach ACK

Apologies for taking a while to get back to you, I was afk, now I'm back.

I took another look and have a couple of comments.

66df06f9de "fees: validate persisted mempool estimator data using post-load weight" does a lot, can you split it into two commits?

First: skip the initial persisted mined-block stats read when -persistmempool=0, and gate the mempool-policy estimator's own read/write paths on persistmempool. Keep IntervalFlush() and ShutdownFlush() scheduled and calling the estimator unconditionally (they also flush the block-policy estimator); the mempool estimator no-ops its own read and flush when persistence is off. (I think this is the commit that should be on the milestone for 32.0.)
Second: use the mempool load result to decide whether to read the persisted mined-block statistics.

There is a narrow window where stale/unvalidated persisted statistics can be exposed. The fee-rate estimate itself is already safe, since EstimateFeeRate returns "Mempool not loaded yet" until pool.SetLoadTried() is true, which runs after the load completes. But the mempool_health_statistics field (backed by GetPrevBlockData) is not gated, so between estimator construction and the retention decision it can surface persisted mined-block stats that have not been validated against the post-load weight yet. We could gate that field too, but that is a not the right fix imo. A cleaner fix is to make it correct by construction: defer the persisted mined-block read until the mempool has finished loading and we have the load result, so m_prev_mined_blocks only ever holds a validated window. For mined-block notifications that arrive during startup (before the load completes) we can just ignore them rather than buffer and replay: they fire during ActivateBestChain(), before LoadMempool(), so the mempool is likely empty when they connect and they may record ~0 coverage, which is an artifact rather than a real signal, and buffering them would only fold that noise into the window. Possible follow-up (not this PR): if we want accurate coverage for the case where a node was down long enough for blocks to pile up, we could remember the last connected blocks' txids and, as the mempool loads, credit the weight of any snapshot tx that was mined in one of them. That reconstructs real coverage for those blocks and lets the retention numerator count the mined snapshot txs, but it adds node/policy plumbing for a fairly narrow case.
For the API, should we pass the MempoolLoadResult (util::Expected) directly into the load completed call instead of unpacking it into bool load_succeeded and snapshot_weight args? The estimator can then measure the post-load weight itself via GetTotalTxWeight()?.
We should also reshape it so FeeRateEstimatorManager receives the mempool-load-completed notification and then tells MemPoolFeeRateEstimator to read the persisted mined-block stats after that. The manager owns the event; the mempool estimator owns the read.

musaHaruna commented at 6:21 AM on September 3, 2026: contributor

Approach ACK

For context, after the original mempool-based fee estimator PR was merged, I continued running experiments. That was how I also noticed similar problem and reproduced the mismatch documented in here.

This PR addresses the underlying problem comprehensively, including the scenario covered by my experimental test, by validating persisted estimator health against the mempool-loading outcome and post-load mempool weight.

I did a light pass through the code and noticed a few things:

Could we call assert_mempool_estimator_unavailable() again after the final self.restart_node(0) in the expired-transactions case? The existing assertion verifies immediate in-memory invalidation, while the post-restart assertion would verify end-to-end that the cleared state survives shutdown and reload and that the old healthy window is not resurrected:

assert_mempool_estimator_unavailable()
self.restart_node(0)
assert_mempool_estimator_unavailable()

Could we also update the -persistmempool help text? The option now controls reading and writing the related mempool-policy estimator data, not only mempool.dat. For example:

"Whether to save and load the mempool and persist related mempool-policy estimator data across restarts"

Could snapshot_weight be represented as std::optional<uint64_t> instead of using bool load_succeeded together with uint64_t snapshot_weight? The states would then be:

std::nullopt                           = load failed
std::optional<uint64_t>{0}             = successfully loaded an empty snapshot
positive value                         = successfully loaded a nonempty snapshot

This would make contradictory states such as (false, 4) unrepresentable. The current production caller cannot generate that combination, although the interface permits it and the unit test constructs it deliberately.

The interface could look like:

void MempoolLoadCompleted(
    std::optional<uint64_t> snapshot_weight,
    uint64_t post_load_mempool_weight);

The caller could translate the load result as follows:

std::optional<uint64_t> snapshot_weight;
if (load_result.has_value()) {
    snapshot_weight = load_result.value();
}

node.fee_estimator_man->MempoolLoadCompleted(
    snapshot_weight,
    post_load_mempool_weight);

The failed-load unit-test case would then become:

estimator.MempoolLoadCompleted(
    /*snapshot_weight=*/std::nullopt,
    /*post_load_mempool_weight=*/4);

in src/policy/fees/mempool_estimator.cpp:328 in 66df06f9de

 319 | @@ -318,6 +320,33 @@ void MemPoolFeeRateEstimator::MempoolTxsRemovedForBlock(const std::shared_ptr<co
 320 |      m_cache.Clear();
 321 |  }
 322 |  
 323 | +void MemPoolFeeRateEstimator::MempoolLoadCompleted(bool load_succeeded,
 324 | +                                                   uint64_t snapshot_weight,
 325 | +                                                   uint64_t post_load_mempool_weight)
 326 | +{
 327 | +    const double retention_ratio{snapshot_weight == 0
 328 | +                                     ? (load_succeeded ? 1.0 : 0.0)

stickies-v commented at 8:19 AM on September 3, 2026:

nit: nested ternary operators are hard to read

w0xlt commented at 1:00 AM on September 4, 2026:

Done. Thanks.

in src/policy/fees/mempool_estimator.cpp:323 in 66df06f9de

 319 | @@ -318,6 +320,33 @@ void MemPoolFeeRateEstimator::MempoolTxsRemovedForBlock(const std::shared_ptr<co
 320 |      m_cache.Clear();
 321 |  }
 322 |  
 323 | +void MemPoolFeeRateEstimator::MempoolLoadCompleted(bool load_succeeded,

stickies-v commented at 9:04 AM on September 3, 2026:

I think it would make more sense to make this a MempoolRestored function that is only called when the mempool is successfully loaded, checks the retention ratio, and then Reads if successful.

w0xlt commented at 1:00 AM on September 4, 2026:

Done. Thanks.

stickies-v commented at 9:08 AM on September 3, 2026: contributor

Concept ACK

node: report mempool snapshot weight

Return a util::Expected result from LoadMempool() containing either a
typed load error or the total transaction weight encoded in the
snapshot.

Co-authored-by: ismaelsadeeq <abubakarsadiqismail@proton.me>
Co-authored-by: Robert Netzke <rob.netzke@gmail.com>

7cfe7c81da

fees: respect -persistmempool for mempool estimator data

Use an empty mempool policy estimator path when mempool persistence is
disabled. Make the estimator read and flush paths no-op for an empty
path, while leaving periodic and shutdown flush scheduling unchanged.

Co-authored-by: ismaelsadeeq <abubakarsadiqismail@proton.me>
Co-authored-by: Robert Netzke <rob.netzke@gmail.com>

e9d7ec115b

w0xlt force-pushed on Sep 4, 2026

fees: validate persisted mempool estimator data using post-load weight

Persisted mempool policy estimator data is only representative when
startup leaves the node with a sufficiently populated mempool. Discard
it when mempool.dat cannot be loaded or post-load mempool weight does
not meet the snapshot retention threshold.

Use final mempool weight as a deliberately permissive proxy.
Transactions added independently before measurement may contribute,
biasing the decision toward retaining potentially usable history
without measuring exact snapshot overlap.

Defer reading until mempool loading completes. Ignore pre-completion
block notifications and suppress estimator flushes until then,
preserving the persisted data if loading is interrupted.

Add unit and functional tests for the retention threshold, load
failure, partial expiry, missing or expired snapshots, and disabled
persistence.

Co-authored-by: ismaelsadeeq <abubakarsadiqismail@proton.me>
Co-authored-by: Robert Netzke <rob.netzke@gmail.com>
Co-authored-by: Musa Haruna <hmusa3962@gmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>

d07cc946cc

fees: log mempool load failure reason

Log the typed load error when persisted mempool-policy estimator
statistics cannot be restored. Keep NO_LOAD_PATH silent because it is
the expected result when mempool persistence is disabled.

6314c03cc4

w0xlt force-pushed on Sep 4, 2026

DrahtBot added the label CI failed on Sep 4, 2026

DrahtBot commented at 12:51 AM on September 4, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/33820078917/job/100860655983 LLM reason (✨ experimental): clang-tidy failed (warnings-as-errors) due to bugprone-argument-comment: mismatched argument comment name snapshot_weight vs parameter name v in mempool_fee_estimator_tests.cpp.

<details><summary>Hints</summary>

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.

</details>

w0xlt commented at 1:00 AM on September 4, 2026: contributor

Update since the previous push:

Rebased onto current master and split the combined persistence change as requested by @ismaelsadeeq. The -persistmempool help was updated per @musaHaruna.
Changed LoadMempool() to return util::Expected, originally suggested by @rustaceanrob, and passed that result directly to the manager as suggested by @ismaelsadeeq.
Deferred reading until mempool loading completes, with the manager owning completion and gating earlier block notifications and flushes, per @ismaelsadeeq. The estimator now exposes a success-only MempoolRestored() entry point as suggested by @stickies-v.
Added the missing post-restart assertion requested by @musaHaruna, expanded unit/functional coverage, and added typed load-failure diagnostics.
Removed the nested ternary per @stickies-v and fixed the argument comments flagged by @DrahtBot.

New co-authors added: Musa Haruna (@musaHaruna) and @stickies-v.

DrahtBot removed the label CI failed on Sep 4, 2026

stickies-v commented at 9:41 AM on September 4, 2026: contributor

First: skip the initial persisted mined-block stats read when -persistmempool=0, and gate the mempool-policy estimator's own read/write paths on persistmempool. .... (I think this is the commit that should be on the milestone for 32.0.)

I agree with this approach, I think we need to narrow it down for the milestone and only include the most important, most straightforward fixes. I think that would be just e9d7ec115b7e07c5fcf83505d68dfb13d66a7d26?, and perhaps also checking the mempool load result (although I don't think that's a frequent occurrence?). The rest can be improved in a follow-up for the next release?

I'm not yet sure what the retention threshold is used for beyond the mechanism we already have where new blocks give us a useful indication of mempool health? I think this fully covers the scenario of a clean, extended shutdown?

w0xlt commented at 2:02 PM on September 4, 2026: contributor

Updated my previous comment to clarify what has changed since the last push and why.

I'm not yet sure what the retention threshold is used for beyond the mechanism we already have where new blocks

The retention check runs once during startup, before restoring the saved recent-block statistics (fees/mempool_policy_estimator.dat).

LoadMempool() can succeed even when most snapshot transactions have expired or were rejected, so without this check the old window could be used until new blocks update it.

The 75% value is borrowed from MEMPOOL_REPRESENTATION_THRESHOLD, but it was suggested above to be a separate constant.

I think that would be just https://github.com/bitcoin/bitcoin/commit/e9d7ec115b7e07c5fcf83505d68dfb13d66a7d26?

Happy to narrow this PR to e9d7ec115b for 32.0 and move the other changes to a follow-up if that is the preference here.

ismaelsadeeq commented at 3:10 PM on September 4, 2026: member

Yep, we should keep this narrowed down. The minimal fix in e9d7ec1 should not discard the persisted mined-block stats completely, because they are still useful. Waiting another 6 blocks ~1 hr is unnecessary IMO.

A better minimal fix when mempool fail to load or don't persist could be:

Keep the persisted mined-block stats, but mark the mempool-policy estimator as cold after startup until the mempool has warmed up for a smaller number of blocks 3?
While the mempool is still cold, do not let the mempool-policy estimator return a min relay tx fee floor caused by a sparse mempool. For the combined estimator if we are healthy before the restart I think we should fall back to the block-policy estimator during the warm-up stage.
Resume serving mempool-policy estimates once the mempool is warm again and the mempool health check succeeds. The numerator of the health ratio can be affected after restart because we lost in-memory mempool state, but the health threshold is intentionally lower to tolerate reduced block coverage in these scenarios.
Apply the same warm-up behavior when mempool loading fails.

I also noticed a separate issue when blocks connect while the mempool is still loading. If MempoolTransactionsRemovedForBlock is emitted against a partially loaded mempool, the removed transaction weight can be undercounted. This affects both the mempool-policy health ratio and the block-policy estimator. A follow-up could preserve the persisted txids during load and account for persisted transactions that were mined before they were loaded, so the removed weight is complete or delay MempoolTransactionsRemovedForBlock notifications till mempool is fully loaded.

After that, I think it is reasonable to assume most persisted transactions return on a clean load. Fully determining whether the missing ones matter would require linearizing the persisted mempool and checking whether much of the top of the mempool disappeared, which is probably an overkill. Instead, serve mempool-policy estimates once the mempool has enough weight to build a block template, and stay in warm-up only when so much is missing that the remaining mempool cannot fill one block template.

Note: the mempool rebroadcast work in #21061 would have sped up the warm-up stage in these scenarios.