Cache m_cached_finished_ibd where SetTip is called. #32885

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, l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

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. <sub>Task tidy: https://github.com/bitcoin/bitcoin/runs/45440913265</sub> <sub>LLM reason (✨ experimental): The CI failure is caused by compilation errors due to missing mutex lock assertions in validation.cpp.</sub>

<details><summary>Hints</summary>

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.

</details>

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 12: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 12: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.

in src/test/util/validation.cpp:24 in 184030ea12 outdated

  20 | @@ -21,6 +21,7 @@ void TestChainstateManager::DisableNextWrite()
  21 |  void TestChainstateManager::ResetIbd()
  22 |  {
  23 |      m_cached_finished_ibd = false;
  24 | +    m_cached_chaintip_recent = false;

l0rinc commented at 1:11 PM on December 2, 2025:

I find the existence of his whole method very hacky, we're testing something that cannot happen in reality so if the test passes or fails after this, it won't increase my confidence in the product. But if you insist on updating it (which we likely have to), we should update JumpOutOfIbd as well for symmetry.

in src/validation.cpp:2043 in 184030ea12 outdated

2039 | @@ -2040,6 +2040,17 @@ bool ChainstateManager::IsInitialBlockDownload() const
2040 |      return false;
2041 |  }
2042 |  
2043 | +void ChainstateManager::UpdateCachedChaintipRecent()

l0rinc commented at 1:32 PM on December 2, 2025:

We're introducing dead code in the first commit without context about where these values are coming from. What if instead we extract the internal checks from IsInitialBlockDownload and slowly migrate that behavior away from there.

Note also that ActiveTip() already returns the tip we need. I'm also not exactly sure why we're calling the current state "cached". And we're already in ChainstateManager, simply referring to "tip" is already unambiguous.

The first commit could lay the groundwork by extracting-and-reusing the recency check only, the second commit could route active chain SetTip through ChainstateManager to make sure each state change updates this as well, the third commit could cache the locked recency calculations, and the last one could finally eliminate the lock from the reader side.

in src/validation.h:1146 in 184030ea12 outdated

1142 | @@ -1142,6 +1143,7 @@ class ChainstateManager
1143 |  
1144 |      /** Check whether we are doing an initial block download (synchronizing from disk or network) */
1145 |      bool IsInitialBlockDownload() const;
1146 | +    void UpdateCachedChaintipRecent() EXCLUSIVE_LOCKS_REQUIRED(cs_main);

l0rinc commented at 1:36 PM on December 2, 2025:

This could be const getter instead and it could use some comment (and I'd specialize it to just return the value instead of mutating the state, we can do that in the SetTip method instead)

    /** Check whether the active chain tip exists, has enough work, and is recent. */
    bool IsTipRecent() const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

in src/validation.cpp:3102 in e11f608f45 outdated

3099 | @@ -3100,6 +3100,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
3100 |      }
3101 |  
3102 |      m_chain.SetTip(*pindexDelete->pprev);

l0rinc commented at 1:39 PM on December 2, 2025:

what's the reason for separating this work from SetTip, if it's related to it? We could move it to the manager which would call both method, the tip update, followed by the IBD state update

in src/validation.cpp:2051 in 184030ea12 outdated

2046 | +    CChain& chain{ActiveChain()};
2047 | +    if (chain.Tip() == nullptr) return;
2048 | +    if (chain.Tip()->nChainWork < MinimumChainWork()) return;
2049 | +    if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) return;
2050 | +
2051 | +    m_cached_chaintip_recent = true;

l0rinc commented at 3:07 PM on December 2, 2025:

shouldn't we guard this method with this being false?

in src/validation.h:1032 in 99df6bfdb7

1028 | @@ -1029,6 +1029,7 @@ class ChainstateManager
1029 |       * const, which latches this for caching purposes.
1030 |       */
1031 |      mutable std::atomic<bool> m_cached_finished_ibd{false};
1032 | +    mutable std::atomic<bool> m_cached_chaintip_recent{false};

l0rinc commented at 4:06 PM on December 2, 2025:

no need to mention chain here and we can use std::atomic_bool instead and should add some description to it

l0rinc changes_requested

l0rinc commented at 4:08 PM on December 2, 2025: contributor

Concept ACK, makes sense to push the burden to the writer instead of the reader. But we need to restructure it slightly so that it tells a story of what we're extracting, delegating and caching exactly.

I have implemented an example in https://github.com/l0rinc/bitcoin/pull/60 (prototype, may not pass all tests yet).

l0rinc commented at 9:02 PM on December 29, 2025: contributor

@pstratem, are you still working on this or would you like me to take over?

sedited commented at 1:48 PM on January 11, 2026: contributor

There hasn't been progress here in many months. Maybe time to re-open it?

l0rinc commented at 1:50 PM on January 11, 2026: contributor

I will open an alternative PR for this today Edit: pushed https://github.com/bitcoin/bitcoin/pull/34253

maflcko closed this on Jan 12, 2026