Cache m_cached_finished_ibd where SetTip is called. #32885

pstratem · 2025-07-06T09:29:26Z

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 · 2025-07-06T09:29:30Z

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  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 · 2025-07-07T01:58:25Z

🚧 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.}

Hints

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.

stickies-v

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

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 · 2025-07-08T00:09:14Z

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 · 2025-07-08T00:43:43Z

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 · 2025-07-08T05:53:06Z

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

maflcko · 2025-07-08T10:01:23Z

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

luke-jr

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 · 2025-07-10T05:28:21Z

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 · 2025-07-24T17:28:46Z

@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

mzumsande

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().

mzumsande · 2025-07-24T21:07:41Z

src/validation.cpp

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?!

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.

mzumsande · 2025-07-24T21:22:58Z

src/validation.cpp

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?

pstratem · 2025-07-26T20:02:44Z

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

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

…ntipRecent. 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.

…n forward.

DrahtBot · 2025-10-27T17:09:09Z

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

l0rinc

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 l0rinc#60 (prototype, may not pass all tests yet).

l0rinc · 2025-12-02T13:11:19Z

src/test/util/validation.cpp

 void TestChainstateManager::ResetIbd()
 {
    m_cached_finished_ibd = false;
+    m_cached_chaintip_recent = false;


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.

l0rinc · 2025-12-02T13:32:21Z

src/validation.cpp

    return false;
 }

+void ChainstateManager::UpdateCachedChaintipRecent()


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.

l0rinc · 2025-12-02T13:36:07Z

src/validation.h


    /** Check whether we are doing an initial block download (synchronizing from disk or network) */
    bool IsInitialBlockDownload() const;
+    void UpdateCachedChaintipRecent() EXCLUSIVE_LOCKS_REQUIRED(cs_main);


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)

Suggested change

void UpdateCachedChaintipRecent() EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/** Check whether the active chain tip exists, has enough work, and is recent. */

bool IsTipRecent() const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

l0rinc · 2025-12-02T13:39:22Z

src/validation.cpp

        }
    }

    m_chain.SetTip(*pindexDelete->pprev);


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

l0rinc · 2025-12-02T15:07:22Z

src/validation.cpp

+    if (chain.Tip()->nChainWork < MinimumChainWork()) return;
+    if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) return;
+
+    m_cached_chaintip_recent = true;


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

l0rinc · 2025-12-02T16:06:03Z

src/validation.h

     * const, which latches this for caching purposes.
     */
    mutable std::atomic<bool> m_cached_finished_ibd{false};
+    mutable std::atomic<bool> m_cached_chaintip_recent{false};


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

l0rinc · 2025-12-29T21:02:12Z

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

sedited · 2026-01-11T13:48:30Z

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

l0rinc · 2026-01-11T13:50:36Z

I will open an alternative PR for this today
Edit: pushed #34253

DrahtBot added the CI failed label Jul 6, 2025

pstratem force-pushed the 2025-07-05-lockless-isibd branch 4 times, most recently from c5084ac to 5e85698 Compare July 6, 2025 23:52

DrahtBot removed the CI failed label Jul 7, 2025

pstratem force-pushed the 2025-07-05-lockless-isibd branch 2 times, most recently from 0709758 to f3ee281 Compare July 7, 2025 01:58

DrahtBot added the CI failed label Jul 7, 2025

pstratem force-pushed the 2025-07-05-lockless-isibd branch from f3ee281 to 2e3fefb Compare July 7, 2025 02:18

pstratem changed the title ~~Cache m_cached_finished_ibd in ActivateBestChain.~~ Cache m_cached_finished_ibd where SetTip is called. Jul 7, 2025

DrahtBot changed the title ~~Cache m_cached_finished_ibd where SetTip is called.~~ Cache m_cached_finished_ibd where SetTip is called. Jul 7, 2025

DrahtBot removed the CI failed label Jul 7, 2025

DrahtBot mentioned this pull request Jul 7, 2025

Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) #29640

Merged

stickies-v reviewed Jul 7, 2025

View reviewed changes

furszy reviewed Jul 7, 2025

View reviewed changes

luke-jr suggested changes Jul 8, 2025

View reviewed changes

mzumsande reviewed Jul 24, 2025

View reviewed changes

pstratem force-pushed the 2025-07-05-lockless-isibd branch from 2e3fefb to 116bdc7 Compare July 26, 2025 20:01

pstratem added 2 commits July 26, 2025 16:05

Add m_cached_chaintip_recent and method to update it UpdateCachedChai…

184030e

…ntipRecent. 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.

Call UpdateCachedChaintipRecent when SetTip is moving the active chai…

e11f608

…n forward.

Use m_cached_chaintip_recent in IsIBD removing the cs_main lock.

99df6bf

pstratem force-pushed the 2025-07-05-lockless-isibd branch from 116bdc7 to 99df6bf Compare July 26, 2025 20:06

pstratem requested a review from luke-jr July 26, 2025 23:34

This was referenced Oct 16, 2025

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

achow101 requested review from l0rinc and sipa October 22, 2025 15:51

DrahtBot added the Needs rebase label Oct 27, 2025

l0rinc mentioned this pull request Dec 2, 2025

validation: cache tip recency in m_cached_tip_recent for IsInitialBlockDownload l0rinc/bitcoin#60

Closed

l0rinc suggested changes Dec 2, 2025

View reviewed changes

mzumsande mentioned this pull request Dec 23, 2025

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

Open

l0rinc mentioned this pull request Jan 11, 2026

validation: cache tip recency for lock-free IsInitialBlockDownload() #34253

Open

maflcko closed this Jan 12, 2026

l0rinc mentioned this pull request Jan 14, 2026

Coins Cache Cleanup checklist #34280

Open

23 tasks

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

Cache m_cached_finished_ibd where SetTip is called. #32885

Cache m_cached_finished_ibd where SetTip is called. #32885

Uh oh!

Conversation

pstratem commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jul 7, 2025

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

pstratem commented Jul 8, 2025

Uh oh!

pstratem commented Jul 8, 2025

Uh oh!

l0rinc commented Jul 8, 2025

Uh oh!

maflcko commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

pstratem commented Jul 10, 2025

Uh oh!

pstratem commented Jul 24, 2025

Uh oh!

mzumsande left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstratem Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

mzumsande Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

pstratem commented Jul 26, 2025

Uh oh!

DrahtBot commented Oct 27, 2025

Uh oh!

l0rinc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 2, 2025

Choose a reason for hiding this comment

pstratem commented Jul 6, 2025 •

edited

Loading

DrahtBot commented Jul 6, 2025 •

edited

Loading

maflcko commented Jul 8, 2025 •

edited

Loading

mzumsande left a comment •

edited

Loading

mzumsande Jul 24, 2025 •

edited

Loading

l0rinc left a comment •

edited

Loading

l0rinc Dec 2, 2025 •

edited

Loading

l0rinc Dec 2, 2025 •

edited

Loading

l0rinc Dec 2, 2025 •

edited

Loading

l0rinc Dec 2, 2025 •

edited

Loading

l0rinc commented Jan 11, 2026 •

edited

Loading