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.

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.

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

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.

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?

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.

Oh, I see. I guess if the behavior should be enforced at compile-time, the blocks could be std::moved into the event

Rebased 2233420 -> 42dc172 (txdownloadman_ibd_check_0 -> txdownloadman_ibd_check_1, compare)

• Fixed conflict with refactor: Improve assumeutxo state representation #30214

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?

Concept ACK

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?

given the mixed results from benchcoin maybe we'd need more testing?

The latest runs shows a speedup in both scenarios, but the earlier run showed a slowdown in mainnet-default-uninstrumented, but the only difference was a rebase, so this seems to indicate that the benchcoin run itself is not stable?

(Commit 8da8ff30a57f0fe7f627d121c5e40466f1ecb0b2 was deleted by GitHub, so I am just going after the comment #34054 (comment))