p2p: avoid traversing blocks (twice) during IBD for no reason

furszy commented at 4:54 pm on June 11, 2025: member

Context: Every time a block is connected, a BlockConnected() event is appended to the validation interface queue. This queue is consumed sequentially by a single worker thread. To avoid excessive memory usage, the queue is limited to 10 events at a time, and we stop processing new blocks until the queued events are handled.

Issue: Within the PeerManager::BlockConnected() listener, we traverse the entire block twice inside the transaction download manager — despite not needing to handle orphans or transaction requests during IBD.

Extra info about the changes: The new arg added to BlockConnected() in the first commit is primarily meant to avoid locking cs_main on the listener side (to avoid calling IsInitialBlockDownload() there). Another way of implementing this could be to add a bool field enabled to the TxDownloadManager class that is only set to true when we are out of IBD, so that we don’t have to include a new arg inside the block connection event. This is open for discussion. Could try implementing this second approach too.

validation: add is_ibd bool to BlockConnected signal

This will later be used to avoid traversing entire blocks during IBD when
unnecessary (e.g. during orphans or transaction requests removals)

59c0f6a0c4

p2p: avoid traversing blocks (twice!) during IBD for no reason

Context:
Every time a block is connected, a 'BlockConnected' event is appended to the validation
interface queue. This queue is consumed sequentially by a single worker thread.
To avoid excessive memory usage, the queue is limited to 10 events at a time, and
we stop processing new blocks until the queued events are handled.

Issue:
Within the 'PeerManager::BlockConnected()' listener, we traverse the entire block twice
inside the transaction download manager — despite not needing to handle orphans or
transaction requests during IBD.
This redundant work adds unnecessary delay to the initial block download process.

e48bfeb58f

DrahtBot commented at 4:54 pm on June 11, 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/32730.

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:

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

DrahtBot added the label P2P on Jun 11, 2025

in src/txorphanage.cpp:250 in e48bfeb58f

245@@ -246,6 +246,9 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
246 
247 void TxOrphanage::EraseForBlock(const CBlock& block)
248 {
249+    // No orphans, nothing to do
250+    if (m_orphans.empty()) return;

l0rinc commented at 5:08 pm on June 11, 2025:

This makes sense on its own I think - it’s similar to another draft PR I had (https://github.com/bitcoin-dev-tools/benchcoin/pull/167). If you think it’s related and adds anything, feel free to cherry-pick it here.

instagibbs commented at 6:00 pm on June 11, 2025:

this is a common-sense change by itself :+1:

furszy commented at 7:20 pm on June 11, 2025:

tiny note: this also skip post-IBD blocks when we don’t have any orphans. –Credits to @mzumsande for pointing it out too– We could do the same for the tx requests tracker loop but.. that wouldn’t change much as we still need to traverse the block for the “recently confirmed” filter anyway.

furszy commented at 7:39 pm on June 11, 2025:

This makes sense on its own I think - it’s similar to another draft PR I had (bitcoin-dev-tools#167). If you think it’s related and adds anything, feel free to cherry-pick it here.

That’s cool, will check it in the coming days. First quick glance it seems that if the mempool is empty; we could avoid calling removeForBlock entirely. Which will save us from adding another callback into the validation queue too, which will leave space for processing another block during ibd. Yet, that was a fast check. Should check who is using the MempoolTransactionsRemovedForBlock event.

in src/txorphanage.cpp:249 in e48bfeb58f

245@@ -246,6 +246,9 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
246 
247 void TxOrphanage::EraseForBlock(const CBlock& block)
248 {
249+    // No orphans, nothing to do

l0rinc commented at 5:29 pm on June 11, 2025:

I find the comment redundant, it states exactly what the code already states clearly, doesn’t add any new info

in src/net_processing.cpp:2000 in e48bfeb58f

1993@@ -1994,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
1994 
1995     // The following task can be skipped since we don't maintain a mempool for
1996     // the ibd/background chainstate.
1997-    if (role == ChainstateRole::BACKGROUND) {
1998+    if (is_ibd || role == ChainstateRole::BACKGROUND) {
1999         return;
2000     }
2001     LOCK(m_tx_download_mutex);

l0rinc commented at 5:35 pm on June 11, 2025:

We can safely skip this because we’re never requesting single transactions during IBD (i.e. AlreadyHaveTx isn’t needed during IBD)?

0for (const auto& ptx : pblock->vtx) {
1    RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
2    if (ptx->HasWitness()) {
3        RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256());
4    }
5    m_txrequest.ForgetTxHash(ptx->GetHash());
6    m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
7}

Can we guard this by some other criteria instead of the IBD boolean, like we’re doing with EraseForBlock, e.g.

0if (!m_txrequest.Size()) return;

furszy commented at 7:30 pm on June 11, 2025:

We can safely skip this because we’re never requesting single transactions during IBD (i.e. AlreadyHaveTx isn’t needed during IBD)?
0for (const auto& ptx : pblock->vtx) {
1    RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
2    if (ptx->HasWitness()) {
3        RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256());
4    }
5    m_txrequest.ForgetTxHash(ptx->GetHash());
6    m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
7}
Can we guard this by some other criteria instead of the IBD boolean, like we’re doing with EraseForBlock, e.g.
0if (!m_txrequest.Size()) return;

I thought about it but didn’t do it on purpose. The reasoning is that we still need to add the txs into the recently confirmed filters post-IBD even when we don’t have any tx requests. In other words, we still need to traverse the block. Could at most skip the last two lines with the emptiness check.

in src/net_processing.cpp:1997 in e48bfeb58f

1993@@ -1994,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
1994 
1995     // The following task can be skipped since we don't maintain a mempool for
1996     // the ibd/background chainstate.
1997-    if (role == ChainstateRole::BACKGROUND) {
1998+    if (is_ibd || role == ChainstateRole::BACKGROUND) {

l0rinc commented at 5:41 pm on June 11, 2025:

is the comment (already mentioning IBD) still accurate?

furszy commented at 7:25 pm on June 11, 2025:

is the comment (already mentioning IBD) still accurate?

The comment was accurate, but the code didn’t behave as described. Now it does.

in src/net_processing.cpp:1979 in e48bfeb58f

1974@@ -1975,7 +1975,8 @@ void PeerManagerImpl::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd)
1975 void PeerManagerImpl::BlockConnected(
1976     ChainstateRole role,
1977     const std::shared_ptr<const CBlock>& pblock,
1978-    const CBlockIndex* pindex)
1979+    const CBlockIndex* pindex,
1980+    bool is_ibd)

l0rinc commented at 5:41 pm on June 11, 2025:

there should be a better way to do this than passing around a boolean over so many layers. I’ll review it properly this week.

furszy commented at 7:24 pm on June 11, 2025:

there should be a better way to do this than passing around a boolean over so many layers. I’ll review it properly this week.

Did you check the second approach proposed in the PR description?

l0rinc commented at 5:57 pm on June 11, 2025: contributor

Only had time for adding a few questions and quickly checking the expected effect this will have on IBD.

A flame graph I did this week for master, showing a -reindex until 900k blocks shows this part of the code to be 0.07% of IBD (~20 seconds). It can still make sense to do it, we should avoid doing unnecessary work, just setting the expectations straight.

Left a few notes, the guard makes sense to me, but the part I passionately disagree with is the boolean argument train - is there a simpler way to avoid that either via a different guard or the locally available m_initial_sync_finished (we’ve been locking before anyway).

in src/net_processing.cpp:1997 in e48bfeb58f

1993@@ -1993,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
1994 
1995     // The following task can be skipped since we don't maintain a mempool for
1996     // the ibd/background chainstate.
1997-    if (role == ChainstateRole::BACKGROUND) {
1998+    if (is_ibd || role == ChainstateRole::BACKGROUND) {

TheCharlatan commented at 12:52 pm on June 12, 2025:

Shouldn’t checking role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload() be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don’t think that really matters here. We also use it similarly in the BlockChecked method.

l0rinc commented at 1:44 pm on June 12, 2025:

That’s why I asked for !m_initial_sync_finished as well. If the race is biased towards reenabling it too early (rather than later), I agree that it should be fine.

TheCharlatan commented at 2:06 pm on June 12, 2025:

furszy reminded me that BlockChecked is run synchronously, so I don’t think that comparison makes sense after all. I guess this comment can be resolved.

furszy commented at 2:07 pm on June 12, 2025:

Shouldn’t checking role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload() be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don’t think that really matters here. We also use it similarly in the BlockChecked method.

The diff between BlockConnected() and BlockChecked() is that the former one runs async while the latter runs synchronously. So the idea was to avoid locking cs_main in a different thread which delays other parts of the node.

I also thought about using the ’m_initial_sync_finished’ bool for this, but that’s not good because the field is calculated at a different time interval in the stale tip check thread (we could compute it more often but that’s a different story).

The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).

l0rinc commented at 3:01 pm on June 12, 2025:

The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).

I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I’d vote for that if we really need this param

furszy commented at 6:05 pm on June 12, 2025:

I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I’d vote for that if we really need this param

Ok, I’m a ~0 on this. I think this field is special enough (reason below) to be signaled as a separate arg or it could be wrapped into a struct — similar to how it’s done in the interfaces::Chain::Notifications class. With this, I’m not saying to not try the TxDownloadManager enabled flag approach, I just think that the current approach is slightly more useful for future listeners usages.

The only thing I’m somewhat convinced of is that, just like listeners are informed about blocks being downloaded through AssumeUTXO background chain validation, it would be nice if they are aware of IBD blocks — otherwise, sooner or later they will end up re-implementing the same racy/locking isIBD() logic across multiple layers (which could be fine for some cases but it doesn’t seem to be generally good).

l0rinc commented at 11:57 am on June 13, 2025:

it would be nice if they are aware of IBD blocks — otherwise […] they will end up re-implementing

Is there a more reusable way we could implement this?

TheCharlatan commented at 10:15 am on June 13, 2025: contributor

One thing that still irks me a bit is that we are communicating more ChainstateManager state through a Chainstate callback. I think ideally the chainstate should not have to know if it is in ibd or not, but that is a difficult thing to achieve with out current code. What does seem non-ideal is that re-indexing is treated the same is_ibd. Maybe the SynchronizationState from the headerTip notification would give us more flexibility in that regard if needed in the future?

p2p: avoid traversing blocks (twice) during IBD for no reason #32730

Code Coverage & Benchmarks

Reviews

Conflicts