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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32730 (p2p: avoid traversing blocks (twice) during IBD by furszy)
#31829 (p2p: improve TxOrphanage denial of service bounds by glozow)

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 👍🏾

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

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.

1c6b5032cf

orphanage: avoid vtx iteration when no orphans 54f9cb85c4

l0rinc force-pushed on Jun 30, 2025

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

mempool: Avoid needless vtx iteration during IBD #32827

Code Coverage & Benchmarks

Reviews

Conflicts