net processing: Add ibd check before processing block for txdownloadman #34054

1. 
   sedited commented at 9:06 am on December 12, 2025: contributor

   Calculating the rolling bloom filters for the txorphanage takes some CPU time from the scheduler thread. This can be observed for example in this flamegraph, where handling the filter takes about 2.6% of total time (and most of the scheduler thread’s time).

   During ibd the entries in the tx download bloom filter are just continuously rolled over and aren’t consumed, since no mempool entries are created by incoming transactions from peers during ibd. The mempool does accept transactions via RPC, or the wallet at the time, however these don’t interact with the orphanage and the txdownloadman, because adding anything to those is guarded by IsInitialBlockDownload() checks as well.

   We’re usually latching ibd to false a few blocks before catching up to the tip, so this should also not significantly degrade the performance of the filter once fully caught up.

2. DrahtBot added the label P2P on Dec 12, 2025
3. 
   DrahtBot commented at 9:06 am on December 12, 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/34054.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	mzumsande, fjahr

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

   Conflicts

   No conflicts as of last run.

4. 
   maflcko commented at 10:17 am on December 12, 2025: member

   During ibd the entries in the tx download bloom filter are just continuously rolled over, and aren’t consumed, since no mempool is maintained at the time.

   I think this could be clarified a bit, why this is safe. My understanding is that a mempool does exist and is maintained (albeit it will normally be empty). The mempool will also happily accept transactions via RPC, or the wallet. However, this is fine, because they don’t interact with the orphanage and the txdownloadman, because adding anything to those is guarded by IsInitialBlockDownload() checks as well?

   Ref:

   https://github.com/bitcoin/bitcoin/blob/8da8ff30a57f0fe7f627d121c5e40466f1ecb0b2/src/net_processing.cpp#L4258-L4268

   and

   https://github.com/bitcoin/bitcoin/blob/8da8ff30a57f0fe7f627d121c5e40466f1ecb0b2/src/net_processing.cpp#L3987-L3990

5. 
   sedited commented at 8:21 pm on December 15, 2025: contributor

   Re #34054 (comment)

   I think this could be clarified a bit, why this is safe.

   Thanks, that was not precise. Slightly reworded your explanation and added it to the pull request description. I was curious if the ibd check might cause a performance regression, since we’re probably triggering fewer de-allocs on the scheduler thread. I tried to get some results in benchcoin. The results were kind of mixed, but there is clearly no more work being done through the txdownloadman during ibd.

6. 
   maflcko commented at 12:32 pm on December 16, 2025: member

   fewer de-allocs on the scheduler thread.

   I think this patch does not change whether the event is run on the scheduler thread, or not, but rather if the event runs faster in the scheduler thread with the early return. So I think this should not move destructor calls between threads. Or am I missing something?

7. 
   sedited commented at 12:52 pm on December 16, 2025: contributor

   Re #34054 (comment)

   but rather if the event runs faster in the scheduler thread with the early return.

   Yes, and I was curious if this might change where the final decrement for the block’s shared pointer happens.

8. 
   maflcko commented at 1:31 pm on December 16, 2025: member

   Oh, I see. I guess if the behavior should be enforced at compile-time, the blocks could be std::moved into the event
9. net processing: Check if we are in ibd before processing block for txdownloadman 42dc1725b9
10. DrahtBot added the label Needs rebase on Dec 16, 2025
11. sedited force-pushed on Dec 16, 2025
12. 
    sedited commented at 3:26 pm on December 16, 2025: contributor

    Rebased 2233420fcff7c9c3d7d5727880a8ae1e6173eb89 -> 42dc1725b978329e0808db5e2c3a64c038c35de3 (txdownloadman_ibd_check_0 -> txdownloadman_ibd_check_1, compare)

    • Fixed conflict with #30214
13. DrahtBot removed the label Needs rebase on Dec 16, 2025
14. in src/net_processing.cpp:1969 in 42dc1725b9

    1966@@ -1967,7 +1967,7 @@ void PeerManagerImpl::BlockConnected(
    1967 
    1968     // The following task can be skipped since we don't maintain a mempool for
    1969     // the historical chainstate.
    

    ---

    ---

    

    mzumsande commented at 8:46 pm on December 23, 2025:

    comment could be adjusted for why we skip it during IBD.
15. 
    mzumsande commented at 9:16 pm on December 23, 2025: contributor

    Concept ACK

    In addition to RPC/wallet mentioned above, the mempool could also contain transactions from disk - downloaded at a time when we weren’t in IBD. This is very common for nodes that aren’t online 24/7 , so that they will go back into IBD at each subsequent start. But those shouldn’t really be a problem either I think.

    Also, this introduces a cs_main lock in the scheduler thread, which could cause some lock contention with msghand - not sure how important this it, but it might go well together with #32885.

16. 
    sedited commented at 9:32 am on December 24, 2025: contributor

    Also, this introduces a cs_main lock in the scheduler thread, which could cause some lock contention with msghand - not sure how important this it, but it might go well together with #32885.

    Yes, the original PR explicitly worked around this by introducing a variable to track the ibd state: https://github.com/bitcoin/bitcoin/pull/32730/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dL1911-L1912 . A prior approach of that PR passes the ibd state directly in the BlockConnected callback. I think blocking here might be fine (which is why it opened this), but it’s not great either. What do you think of drafting this for now, and we try to make progress with #32885, or do you prefer one of the other approaches?

17. 
    fjahr commented at 9:28 pm on December 25, 2025: contributor

    Concept ACK
18. 
    mzumsande commented at 6:51 pm on December 26, 2025: contributor

    What do you think of drafting this for now, and we try to make progress with #32885, or do you prefer one of the other approaches?

    I think I like the approach from #32885 because it addresses the issue at its root (so that many IBD checks would benefit, not just this one). As to whether this should be drafted I’m unsure - it’s probably an improvement even with the added locking due to the saved work, but given the mixed results from benchcoin maybe we’d need more testing?

Contributors
sedited DrahtBot maflcko mzumsande fjahr

Labels
P2P