mempool: Avoid needless vtx iteration during IBD #32827

pull l0rinc wants to merge 2 commits into bitcoin:master from l0rinc:l0rinc/empty-mempool-IBD changing 2 files +15 āˆ’14
  1. l0rinc commented at 6:23 pm on June 29, 2025: contributor

    During Initial Block Download, the mempool is usually empty, but CTxMemPool::removeForBlock is still called for every connected block where we:

    • iterate over every transaction in the block even though none will be found in the empty mapTx, always leaving txs_removed_for_block empty…
    • which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty.

    Similarly to #32730 (review), this change introduces a minor performance & memory optimization by only executing the loop if any of the affected mempool maps have any contents. The second commit is cherry-picked from there since it’s related to this change as well.

  2. DrahtBot added the label Mempool on Jun 29, 2025
  3. DrahtBot commented at 6:23 pm on June 29, 2025: 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/32827.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK optout21, ismaelsadeeq, glozow
    Stale ACK luke-jr, maflcko, mzumsande

    If your review is incorrectly listed, please react with šŸ‘Ž to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28676 ([WIP] Cluster mempool implementation by sdaftuar)

    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.

  4. l0rinc marked this as a draft on Jun 29, 2025
  5. mzumsande commented at 11:11 pm on June 29, 2025: contributor
    “known to be empty” / “always empty” is too strong: It’s quite common for nodes that are not online 24/7 to be in IBD (because they have to catch up a few weeks/months) but have a non-empty mempool. This doesn’t affect the approach, just the PR / commit message description.
  6. l0rinc commented at 5:48 am on June 30, 2025: contributor
    Thanks, updated - though I don’t think that’s “Initial”, just regular Block Download.
  7. in src/txmempool.cpp:685 in a5f8f16719 outdated
    694         }
    695-        removeConflicts(*tx);
    696-        ClearPrioritisation(tx->GetHash());
    697     }
    698     if (m_opts.signals) {
    699         m_opts.signals->MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight);
    


    optout21 commented at 8:53 am on June 30, 2025:
    Couldn’t the MempoolTransactionsRemovedForBlock call also go inside the above if? Can this call be omitted if txs_removed_for_block is empty? If yes, the declaration of txs_removed_for_block could also go inside the if.

    l0rinc commented at 10:51 am on June 30, 2025:

    That’s the question, this is what I was referring to in the PR description:

    Draft until I get feedback on whether MempoolTransactionsRemovedForBlock should still be called for empty vectors, given that feature_fee_estimation is failing if we skip it… Would be cool if we could avoid adding another callback into the validation queue…


    optout21 commented at 11:01 am on June 30, 2025:

    I think it’s a different question: does it need to be called …

    1. when the mempool is empty, or
    2. when the list of transactions to be removed is empty.

    maflcko commented at 11:03 am on June 30, 2025:
    Probably only if firstRecordedHeight is zero, which would need to be checked inside.

    l0rinc commented at 5:52 pm on June 30, 2025:

    which would need to be checked inside

    Wouldn’t that be after it’s added to the queue already - which would kinda’ defeat the purpose as far as I can tell :/


    maflcko commented at 5:42 am on July 1, 2025:
    Yeah, not sure. It would be good to see some flamegraphs or IBD speedup, to see what matters?

    l0rinc commented at 2:23 pm on July 1, 2025:
    That change seems riskier than the rest, we can do it in a follow-up. The PR is ready for review.

    l0rinc commented at 6:56 pm on July 5, 2025:

    @furszy and @ismaelsadeeq have opined on it in the IRC meeting:

    18:38 l0rinc: I just had a quick talk with abubakarsadiq and It seems we still need to send the signal just to update the best seen height inside the fee estimation class, but we can skip some of the calculations on the event processing side (all the stats objects are initialized but contain only the default values, so we could avoid going through them while computing some mod operations). Still, not sure how much performance gain we will get from this. Matter of checking it. 18:58 yeah working on it šŸ‘šŸ¾


    ismaelsadeeq commented at 4:53 pm on July 7, 2025:
    Yeah, the call should be outside the if statement because the fee estimator decays previously tracked data points after block connection in order to make stale data points less relevant and recent data points more relevant over time.

    ismaelsadeeq commented at 4:55 pm on July 7, 2025:
    On what I discussed with @furszy I posted comment here #32748 (review)
  8. maflcko commented at 11:03 am on June 30, 2025: member

    Similarly to #32730 (comment),

    Could make sense to include that change here as well?

  9. l0rinc force-pushed on Jun 30, 2025
  10. l0rinc force-pushed on Jun 30, 2025
  11. l0rinc force-pushed on Jun 30, 2025
  12. l0rinc renamed this:
    mempool: Avoid needless vtx iteration in `removeForBlock` during IBD
    mempool: Avoid needless vtx iteration during IBD
    on Jun 30, 2025
  13. l0rinc force-pushed on Jun 30, 2025
  14. l0rinc marked this as ready for review on Jul 1, 2025
  15. maflcko commented at 9:19 am on July 7, 2025: member
    Just to clarify this is just a refactor/cleanup and doesn’t affect end-to-end IBD performance in a measurable way?
  16. l0rinc commented at 1:51 pm on July 7, 2025: contributor

    That’s my expectation, yes. Would you like me to measure it to sure?

    Edit: measured a full reindex, the effect is <1%, as expected

  17. luke-jr commented at 7:13 pm on July 8, 2025: member
    crACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
  18. maflcko commented at 1:57 pm on July 9, 2025: member

    lgtm ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50 🌌

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: lgtm ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50 🌌
    3Wlk4PL+rrh0WOZRZ2h/TJZ1bw7tQq2rihW0c4tE3QXFO7MY7GBGSVVYcDT6sjjyMwDFbnFqFOFNUkrQTmSOICg==
    
  19. luke-jr referenced this in commit 8990a80618 on Jul 17, 2025
  20. luke-jr referenced this in commit fc5361a515 on Jul 17, 2025
  21. in src/txmempool.cpp:670 in 1c6b5032cf outdated
    677-        if (it != mapTx.end()) {
    678-            setEntries stage;
    679-            stage.insert(it);
    680-            txs_removed_for_block.emplace_back(*it);
    681-            RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
    682+    if (mapTx.size() || mapNextTx.size() || mapDeltas.size()) {
    


    mzumsande commented at 6:50 pm on July 17, 2025:
    nit: If mapTx is empty, mapNextTx must necessarily be empty as well, so strictly speaking it doesn’t need to be checked here.

    l0rinc commented at 9:52 pm on July 17, 2025:
    Yes, I’m aware, but I have listed here every value that is read or written to - seems safer this way.
  22. mzumsande commented at 7:25 pm on July 17, 2025: contributor

    Code Review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50

    I think it makes sense not to do these iterations over block txns if these can never result in any action - even if it doesn’t increase performance measurably.

  23. in src/txmempool.cpp:665 in 1c6b5032cf outdated
    660@@ -661,26 +661,25 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
    661     }
    662 }
    663 
    664-/**
    665- * Called when a block is connected. Removes from mempool.
    


    ismaelsadeeq commented at 8:44 pm on July 17, 2025:
    Why delete this comment?

    l0rinc commented at 9:59 pm on July 17, 2025:
    Thanks for the review. I’m not a big fan of dead comments, especially when they’re duplicating information that the code can tell better. The name already states that it removes from mempool. And it’s called from ConnectTip in validation.cpp. I have instead added a comment inside the method without repeating the exact same - please see the commit message.

    ismaelsadeeq commented at 10:11 pm on July 17, 2025:
    This is a valid point, might be better for such removals to be in it’s own commit with this explanation as rationale.
  24. in src/txmempool.cpp:666 in 1c6b5032cf outdated
    660@@ -661,26 +661,25 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
    661     }
    662 }
    663 
    664-/**
    665- * Called when a block is connected. Removes from mempool.
    666- */
    667 void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight)
    668 {
    669+    // Remove confirmed txs and conflicts when a new block is connected, updating the fee logic
    


    ismaelsadeeq commented at 8:46 pm on July 17, 2025:

    Explicitly stating we update fee logic is unnecessary, just state that we fire the MempoolTransactionsRemovedForBlock notification or something like that.

    0    // Remove confirmed txs and conflicts when a new block is connected, and fire MempoolTransactionsRemovedForBlock notification
    

    l0rinc commented at 10:01 pm on July 17, 2025:
    The code already states clearly that MempoolTransactionsRemovedForBlock is called.

    ismaelsadeeq commented at 10:08 pm on July 17, 2025:
    Fair enough, It should be removed then because of the reason here #32827#pullrequestreview-3030963395 I’ve come across multiple places where such comments get stale and not updated.

    l0rinc commented at 10:40 pm on July 17, 2025:
    Since the method is about mempool removal, I found it surprising that it is needed for fee estimation - hence the comment

    ismaelsadeeq commented at 9:54 am on July 18, 2025:
    This is not surprising at all you can easily see that by looking at the subscribers of this notification. Fwiw, I like the change, but I think the last comment in the sentence should be removed. We’re not updating the fee logic inside the subroutine; it’s done in the scheduler thread in the background. Hence, I’m approach ~0 as-is. Based on https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review

    maflcko commented at 10:01 am on July 18, 2025:
    It is also updating the fee logic (lastRollingFee), so the comment isn’t wrong. No strong opinion, though šŸ˜…

    ismaelsadeeq commented at 10:06 am on July 18, 2025:
    Okay, I see that :) Yeah the comment matches that update, I guess that ~0 does not apply any more, can be resolved.

    optout21 commented at 4:54 am on July 20, 2025:

    Very low relevance, but “updating the fee logic” is a nit confusing to me:

    • why “updating” and not “update”? “remove …, updating …” suggests it’s a side effect in the same step as opposed to a separate step
    • is it (updating the fee) logic or updating the (fee logic)? What logic is updated?

    My suggestion: also update fee stats or just also update fees. Otherwise LGTM.


    l0rinc commented at 4:57 am on July 20, 2025:

    “Remove… while also update” -> “Remove…, updating”.

    If you agree with the change please comment an uppercase ACK with the latest commit hash to your message.

  25. ismaelsadeeq commented at 8:49 pm on July 17, 2025: member
    Concept ACK. edit I think the comment about updating the fee logic should be removed, as it can easily become stale.
  26. ismaelsadeeq approved
  27. ismaelsadeeq commented at 10:07 am on July 18, 2025: member
    Code review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
  28. DrahtBot added the label Needs rebase on Jul 18, 2025
  29. mempool: Avoid expensive loop in `removeForBlock` during IBD
    During Initial Block Download, the mempool is usually empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
    * iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
    * which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty.
    
    This change introduces a minor performance optimization by only executing the loop if any of the core mempool maps have any contents.
    
    The call to `MempoolTransactionsRemovedForBlock` and the updates to the rolling fee logic remain unchanged.
    
    The `removeForBlock` was also updated stylistically to match the surrounding methods and a clarification was added to clarify that it affects fee estimation as well.
    41ad2be434
  30. orphanage: avoid vtx iteration when no orphans 249889bee6
  31. l0rinc force-pushed on Jul 18, 2025
  32. l0rinc commented at 11:58 pm on July 18, 2025: contributor
    Had to rebase after https://github.com/bitcoin/bitcoin/commit/067365d2a8a421a074bb54394118beccb3f775c2#diff-e6100361fa0e9e25478f808ca084e5f681d4dddbbee7b3bea0f9d5bcd29db3aaR39-R563, src/txorphanage.cpp was moved to src/node/txorphanage.cpp, the change is exactly the same otherwise, would appreciate re-reviews.
  33. DrahtBot removed the label Needs rebase on Jul 19, 2025
  34. l0rinc requested review from maflcko on Jul 19, 2025
  35. l0rinc requested review from mzumsande on Jul 19, 2025
  36. l0rinc requested review from ismaelsadeeq on Jul 19, 2025
  37. l0rinc requested review from optout21 on Jul 19, 2025
  38. optout21 commented at 5:09 am on July 20, 2025: none
    ACK 249889bee6b88eb9814eb969e6fb108f86a4bf98
  39. DrahtBot requested review from luke-jr on Jul 20, 2025
  40. in src/txmempool.cpp:661 in 41ad2be434 outdated
    660@@ -661,26 +661,25 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
    661     }
    


    ismaelsadeeq commented at 11:18 am on July 21, 2025:

    In “mempool: Avoid expensive loop in removeForBlock during IBD” 41ad2be4340d7b18a4669aa9becef8936597f000

    nit: change commit title to “mempool: avoid looping blk txs when mempool is empty in removeForBlock

  41. ismaelsadeeq commented at 11:19 am on July 21, 2025: member
    reACK 249889bee6b88eb9814eb969e6fb108f86a4bf98
  42. glozow commented at 2:55 pm on July 21, 2025: member
    ACK 249889bee6b88eb9814eb969e6fb108f86a4bf98
  43. glozow merged this on Jul 21, 2025
  44. glozow closed this on Jul 21, 2025

  45. l0rinc deleted the branch on Jul 21, 2025

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: 2025-07-26 21:13 UTC

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