Remove epoch logic from mempool #34384

pull sipa wants to merge 2 commits into bitcoin:master from sipa:202601_no_epochs changing 4 files +9 −131
  1. sipa commented at 10:12 pm on January 22, 2026: member

    Since #33591, the epoch-based graph traversal optimization logic is only used for CTxMempool::GetChildren(), a function that is only used in RPC code and tests. Rewrite it without epochs, and remove util/epochguard.h itself, as that was its last use.

    This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don’t foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.

  2. DrahtBot commented at 10:12 pm on January 22, 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/34384.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, ajtowns, instagibbs

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

  3. in src/txmempool.cpp:65 in 7f17ebf5a4
    66+    const auto& hash = entry.GetTx().GetHash();
    67+    {
    68+        LOCK(cs);
    69+        auto iter = mapNextTx.lower_bound(COutPoint(hash, 0));
    70+        for (; iter != mapNextTx.end() && iter->first->hash == hash; ++iter) {
    71+            ret.push_back(*(iter->second));
    


    l0rinc commented at 10:51 pm on January 22, 2026:

    We expect to have at most 64 children here, right? https://github.com/bitcoin/bitcoin/blob/ade0397f59f2fb59ab0e4ebb39869ac343cc54ee/src/txgraph.h#L17 If that’s the case, wouldn’t a quadratic algo be simpler?

     0std::vector<CTxMemPoolEntry::CTxMemPoolEntryRef> CTxMemPool::GetChildren(const CTxMemPoolEntry& entry) const
     1{
     2    LOCK(cs);
     3    std::vector<CTxMemPoolEntry::CTxMemPoolEntryRef> ret;
     4    const auto hash{entry.GetTx().GetHash()};
     5    for (auto it{mapNextTx.lower_bound(COutPoint(hash, 0))}; it != mapNextTx.end() && it->first->hash == hash; ++it) {
     6        const auto& child{*it->second};
     7        if (std::ranges::none_of(ret, [&](auto& ref) { return &ref.get() == &child; })) {
     8            ret.emplace_back(child);
     9        }
    10    }
    11    return ret;
    12}
    

    This would also fix the MempoolCheck benchmark failure (which indicates no speed regression locally)


    sipa commented at 11:19 pm on January 22, 2026:
    I’ve fixed it another way.

    l0rinc commented at 11:20 pm on January 22, 2026:
    I see, my question still stands

    sipa commented at 11:22 pm on January 22, 2026:

    I think sorting and removing duplicates is more readable than manually checking if newly added elements are already there.

    I agree performance is not a concern.


    l0rinc commented at 11:24 pm on January 22, 2026:
    Didn’t the assertion failure prove the opposite, that’s it’s more complicated than it seems? Not adding elements when we already have them seems as simple as it gets.

    sipa commented at 11:27 pm on January 22, 2026:

    The old code was just using the wrong comparator for std::range::unique, a bug plain and simple that the existing test caught without problems.

    I think this is personal preference, and don’t feel like discussing it further.


    ajtowns commented at 12:01 pm on January 23, 2026:

    We also use GetChildren() in check() on regtest, so avoiding it being too slow might still have some value.

    We might have up to 1009 entries for a tx in mapNext in normal use, I think (1009 p2wsh outputs in the first tx is 31kvB, and 1009 spends of those outputs is 68kvB, so the total stays just under the 400k weight limit), so the vector could be 8kB rather than 512B, which is a little unfortunate? In abnormal usage, we presumably could have up to about double that while staying within the weight limit.

    Yeah, I can’t see avoiding adding duplicates making enough of an improvement here to be worth worrying about even in exceptional cases.


    instagibbs commented at 3:04 pm on January 23, 2026:
    for reference which test caught the regression?

    sipa commented at 3:09 pm on January 23, 2026:

    About a dozen functional tests failed, an assertion inside CTxMemPool::check, in the lines

    0assert(setChildrenCheck.size() == setChildrenStored.size());
    1assert(std::equal(setChildrenCheck.begin(), setChildrenCheck.end(), setChildrenStored.begin(), comp));
    

    maflcko commented at 3:11 pm on January 23, 2026:

    https://github.com/bitcoin/bitcoin/actions/runs/21266749361/job/61207709013 says:

     0...
     12026-01-22T23:08:02.5882649Z feature_rbf.py                                                                   |  Failed  | 3 s
     22026-01-22T23:08:02.5883172Z mempool_package_limits.py                                                        |  Failed  | 2 s
     32026-01-22T23:08:02.5883711Z mempool_packages.py                                                              |  Failed  | 3 s
     42026-01-22T23:08:02.5884219Z mempool_truc.py                                                                  |  Failed  | 3 s
     52026-01-22T23:08:02.5885063Z mempool_updatefromblock.py                                                       |  Failed  | 1 s
     62026-01-22T23:08:02.5885662Z mining_prioritisetransaction.py                                                  |  Failed  | 1 s
     72026-01-22T23:08:02.5886338Z p2p_segwit.py                                                                    |  Failed  | 32 s
     82026-01-22T23:08:02.5886871Z rpc_mempool_info.py                                                              |  Failed  | 1 s
     92026-01-22T23:08:02.5887374Z rpc_packages.py                                                                  |  Failed  | 2 s
    102026-01-22T23:08:02.5887950Z tool_bench_sanity_check.py --bench=MempoolCheck                                  |  Failed  | 1 s
    112026-01-22T23:08:02.5888526Z wallet_labels.py                                                                 |  Failed  | 2 s
    122026-01-22T23:08:02.5889064Z wallet_spend_unconfirmed.py                                                      |  Failed  | 1 s
    132026-01-22T23:08:02.5889596Z wallet_txn_clone.py                                                              |  Failed  | 2 s
    142026-01-22T23:08:02.5890146Z wallet_txn_clone.py --mineblock                                                  |  Failed  | 2 s
    152026-01-22T23:08:02.5890721Z wallet_txn_clone.py --segwit                                                     |  Failed  | 2 s
    162026-01-22T23:08:02.5891270Z wallet_txn_doublespend.py                                                        |  Failed  | 2 s
    172026-01-22T23:08:02.5891850Z wallet_txn_doublespend.py --mineblock                                            |  Failed  | 1 s
    182026-01-22T23:08:02.5892394Z wallet_v3_txs.py                                                                 |  Failed  | 3 s
    192026-01-22T23:08:02.5892667Z 
    202026-01-22T23:08:02.5892935Z ALL                                                                              |  Failed  | 2066 s (accumulated) 
    212026-01-22T23:08:02.5893363Z Runtime: 291 s
    222026-01-22T23:08:02.5893475Z 
    232026-01-22T23:08:02.5893933Z Command '['/usr/bin/python3', './ci_build/test/functional/test_runner.py', '-j', '8', '--combinedlogslen=99999999']' returned non-zero exit status 1.
    

    instagibbs commented at 3:16 pm on January 23, 2026:
    love to see it, thanks
  4. l0rinc changes_requested
  5. sipa force-pushed on Jan 22, 2026
  6. DrahtBot added the label CI failed on Jan 22, 2026
  7. Rework CTxMemPool::GetChildren() to not use epochs
    This is likely slightly slower, but this was the last place we were using
    epochs instead of sets to deduplicate, and this is only used by the RPC code
    and in tests, and should not be CPU-performance critical. Eliminating this
    allows us to save 8 bytes in CTxMemPoolEntry.
    
    Co-Authored-By: Pieter Wuille <bitcoin-dev@wuille.net>
    1a8494d16c
  8. Remove unused epochguard.h 40735450c0
  9. sipa force-pushed on Jan 23, 2026
  10. DrahtBot removed the label CI failed on Jan 23, 2026
  11. fanquake requested review from instagibbs on Jan 23, 2026
  12. l0rinc approved
  13. l0rinc commented at 11:35 am on January 23, 2026: contributor
    code review ACK 40735450c00b10baa03e3a7f1e2bee439077e356
  14. ajtowns commented at 12:01 pm on January 23, 2026: contributor
    ACK 40735450c00b10baa03e3a7f1e2bee439077e356
  15. instagibbs commented at 3:02 pm on January 23, 2026: member

    ACK 40735450c00b10baa03e3a7f1e2bee439077e356

    Confirmed it’s only used in non-performance critical situations.

  16. fanquake merged this on Jan 23, 2026
  17. fanquake closed this on Jan 23, 2026


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-01-27 06:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me