Cache m_cached_finished_ibd where SetTip is called.

pstratem commented at 9:29 am on July 6, 2025: contributor

As IsInitialBlockDownload latches to false only once the Tip is sufficiently advanced there is no need to check the Tip everytime IsIBD is called.

By caching this in advance we can avoid extra work and more importantly a lock.

DrahtBot commented at 9:29 am on July 6, 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/32885.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	luke-jr

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:

#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29640 (Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) by sr-gi)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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 CI failed on Jul 6, 2025

pstratem force-pushed on Jul 6, 2025

DrahtBot removed the label CI failed on Jul 7, 2025

pstratem force-pushed on Jul 7, 2025

DrahtBot added the label CI failed on Jul 7, 2025

DrahtBot commented at 1:58 am on July 7, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/45440913265 LLM reason (✨ experimental): The CI failure is caused by compilation errors due to missing mutex lock assertions in validation.cpp.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

pstratem force-pushed on Jul 7, 2025

pstratem renamed this:
~~Cache m_cached_finished_ibd in ActivateBestChain.~~
Cache m_cached_finished_ibd where SetTip is called.
on Jul 7, 2025

DrahtBot renamed this:
~~Cache m_cached_finished_ibd where SetTip is called.~~
Cache m_cached_finished_ibd where SetTip is called.
on Jul 7, 2025

DrahtBot removed the label CI failed on Jul 7, 2025

stickies-v commented at 5:17 pm on July 7, 2025: contributor

Conceptually not a bad idea to cache and lock less, but imo this makes the code more brittle (and harder to understand), e.g. if any tip updates happen without the cache being updated separately.

Do you have any data as to the actual performance improvements from this PR?

furszy commented at 6:10 pm on July 7, 2025: member

I think something like this, if properly implemented (I haven’t thought much about the code yet), would reduce the GUI freezes during IBD in a noticeable manner.

pstratem commented at 0:09 am on July 8, 2025: contributor

Conceptually not a bad idea to cache and lock less, but imo this makes the code more brittle (and harder to understand), e.g. if any tip updates happen without the cache being updated separately.

Do you have any data as to the actual performance improvements from this PR?

I’m (very) open to suggestions on how to make the caching call more robust. (Indeed I expected some.)

There’s no performance improvement from this PR, it’s the first in a series of proposed changes I’ll be making to remove locking where it’s not necessary, with the end goal being some form of concurrency being possible in message processing.

pstratem commented at 0:43 am on July 8, 2025: contributor

I think something like this, if properly implemented (I haven’t thought much about the code yet), would reduce the GUI freezes during IBD in a noticeable manner.

I hadn’t even considered that, but certainly that’s a possible direct improvement.

l0rinc commented at 5:53 am on July 8, 2025: contributor

@pstratem, not sure if you saw this, but could be helpful: https://github.com/bitcoin/bitcoin/pull/25081

maflcko commented at 10:01 am on July 8, 2025: member

tight polling of is_ibd seems like a mistake in the first place, so i am not sure if this is something to optimize for.

Looking at the remaining call sites of the ibd check, most have cs_main already, so they won’t be affected by this? The remaining ones (I only found MaybeSendFeefilter), if they are relevant, could either re-order their code to call it less often, or cache the bool themselves? For the gui, see also https://github.com/bitcoin/bitcoin/issues/17145

luke-jr changes_requested

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

Concept ACK, but I’m not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it’s not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.

pstratem commented at 5:28 am on July 10, 2025: contributor

Concept ACK, but I’m not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it’s not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.

This made me revisit the function and consider what we’re trying to achieve.

The function is only interesting when it can latch to the IBD finished state.

That’s only possible when all four conditions are met, which can only happen when the tip is updated.

The final time based condition can only change when the tip changes as it gets further away with time, not closer.

pstratem commented at 5:28 pm on July 24, 2025: contributor

@pstratem, not sure if you saw this, but could be helpful: #25081

That seems like it would be useful, but for now I’m just parsing the lock contention messages out of debug.log

in src/validation.cpp:3082 in 2e3fefbaaa outdated

3078@@ -3089,6 +3079,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
3079     }
3080 
3081     m_chain.SetTip(*pindexDelete->pprev);
3082+    m_chainman.CacheIsInitialBlockDownload();

mzumsande commented at 9:07 pm on July 24, 2025:

First I thought this was not necessary because disconnecting a block shouldn’t usually get you out of IBD, but I guess there are edge cases (starting up, with the old tip having a lower timestamp than it’s parent block) where this could lead to get us out of IBD?!

pstratem commented at 8:08 pm on July 26, 2025:

It’s technically possible for disconnecting a block to get us out of IBD, though I really don’t think that particular edge case is super important.

I was just trying to be thorough.

in src/validation.cpp:2025 in 2e3fefbaaa outdated

2043+    if (m_cached_finished_ibd.load(std::memory_order_relaxed)) return;
2044+
2045+    if (m_blockman.LoadingBlocks()) return;
2046+
2047+    {
2048+        AssertLockHeld(cs_main);

mzumsande commented at 9:22 pm on July 24, 2025:

since the function is annotated with EXCLUSIVE_LOCKS_REQUIRED(cs_main) anyway, why not put it to the beginning of the function, as it is done in most other places?

mzumsande commented at 9:41 pm on July 24, 2025: contributor

The function is only interesting when it can latch to the IBD finished state.

That’s only possible when all four conditions are met, which can only happen when the tip is updated.

The final time based condition can only change when the tip changes as it gets further away with time, not closer.

I think that @luke-jr is right. If we reindex, we set m_importing to true in ImportBlocks, so any blocks we connect there can never result in getting out of IBD due to the m_blockman.LoadingBlocks() early return. Therefore we need a call to CacheIsInitialBlockDownload() after ImportingNow goes out of scope in ImportBlocks().

pstratem force-pushed on Jul 26, 2025

pstratem commented at 8:02 pm on July 26, 2025: contributor

Ok I thought about it and it just wasn’t obviously correct enough.

So I’ve rewritten into three commits to be simpler.

Add m_cached_chaintip_recent and method to update it UpdateCachedChaintipRecent.

On systems with sane clocks the chain tip checks can only change when the tip
changes. The gap between the chain tip and the current time only grows.

184030ea12

Call UpdateCachedChaintipRecent when SetTip is moving the active chain forward. e11f608f45

Use m_cached_chaintip_recent in IsIBD removing the cs_main lock. 99df6bfdb7

pstratem force-pushed on Jul 26, 2025

pstratem requested review from luke-jr on Jul 26, 2025

achow101 requested review from sipa on Oct 22, 2025

achow101 requested review from l0rinc on Oct 22, 2025

DrahtBot added the label Needs rebase on Oct 27, 2025

DrahtBot commented at 5:09 pm on October 27, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Cache m_cached_finished_ibd where SetTip is called. #32885

Code Coverage & Benchmarks

Reviews

Conflicts