mempool: Avoid needless vtx iteration during IBD

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.

DrahtBot added the label Mempool on Jun 29, 2025

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.

l0rinc marked this as a draft on Jun 29, 2025

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.

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.

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 …

when the mempool is empty, or
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)

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?

l0rinc force-pushed on Jun 30, 2025

l0rinc renamed this:
~~mempool: Avoid needless vtx iteration in `removeForBlock` during IBD~~
mempool: Avoid needless vtx iteration during IBD
on Jun 30, 2025

l0rinc force-pushed on Jun 30, 2025

l0rinc marked this as ready for review on Jul 1, 2025

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?

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

luke-jr commented at 7:13 pm on July 8, 2025: member

crACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50

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==

luke-jr referenced this in commit 8990a80618 on Jul 17, 2025

luke-jr referenced this in commit fc5361a515 on Jul 17, 2025

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.

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.

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.

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.

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~~.

ismaelsadeeq approved

ismaelsadeeq commented at 10:07 am on July 18, 2025: member

Code review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50

DrahtBot added the label Needs rebase on Jul 18, 2025

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

orphanage: avoid vtx iteration when no orphans 249889bee6

l0rinc force-pushed on Jul 18, 2025

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.

DrahtBot removed the label Needs rebase on Jul 19, 2025

l0rinc requested review from maflcko on Jul 19, 2025

l0rinc requested review from mzumsande on Jul 19, 2025

l0rinc requested review from ismaelsadeeq on Jul 19, 2025

l0rinc requested review from optout21 on Jul 19, 2025

optout21 commented at 5:09 am on July 20, 2025: none

ACK 249889bee6b88eb9814eb969e6fb108f86a4bf98

DrahtBot requested review from luke-jr on Jul 20, 2025

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”

ismaelsadeeq commented at 11:19 am on July 21, 2025: member

reACK 249889bee6b88eb9814eb969e6fb108f86a4bf98

glozow commented at 2:55 pm on July 21, 2025: member

ACK 249889bee6b88eb9814eb969e6fb108f86a4bf98

glozow merged this on Jul 21, 2025

glozow closed this on Jul 21, 2025

l0rinc deleted the branch on Jul 21, 2025

mempool: Avoid needless vtx iteration during IBD #32827

Code Coverage & Benchmarks

Reviews

Conflicts