validation: don't re-acquire cs_main during IBD in CChainState::IsInitialBlockDownload() #24220

No longer re-acquire lock cs_main during IBD in CChainState::IsInitialBlockDownload(), as most of its callers already hold the lock. Have these callers invoke non-locking function CChainState::IsIBD() instead. Apply thread safety analysis to the IBD functions. Retains the post-IBD optimization latch for the callers that do not hold the lock. Makes explicit which callers hold cs_main, and which do not.

Noticed while reviewing #24178.

I recall that the reason IsIBD was written like this:

bool CChainState::IsInitialBlockDownload() const
{
    // Optimization: pre-test latch before taking the lock.
    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
        return false;

    LOCK(cs_main);

is so that in the common case -- which is that we've already latched to false -- that we can respond without ever acquiring cs_main inside this function. See #8007

Maybe there's a better refactor that could be done so that we can reduce locking contention on cs_main further, but this patch would have all the call sites grab the lock in order to even call IsIBD(), which would add more cs_main lock acquisitions in general -- and that seems counter to where we want to end up?

Is the motivation for this patch to reduce the uses of cs_main as a recursive mutex, and thereby get to a better design? If so that sounds like a good direction, but I think it'd be better if we avoided adding extra lock acquisitions (and holding cs_main longer) in order to get there. For instance it seems like with this patch some of the validation callbacks will now be invoked while holding cs_main the whole time? That seems undesirable.

Yes, the trade-off this pull makes EDIT: is no longer made, thanks to review feedback. <strike>foregoing the optimization for a half dozen callers that don't already hold cs_main, in favor of avoiding re-taking the cs_main lock for the 18 callers that do already hold it, by moving the lock-taking to the half dozen callers (but just for the call itself, so essentially only the optimization is affected unless I am missing something).</strike>

Thanks for the link to #8007. It looks like the proportion of callers that hold the lock has since increased.

For instance it seems like with this patch some of the validation callbacks will now be invoked while holding cs_main the whole time? That seems undesirable.

Agree with @sdaftuar; IsIBD() is called in a few performance-critical places and at the very least this change should be benchmarked before merge. I'm Concept NACK if this change is essentially just for refactoring's sake; if there's some overarching benefit it would be nice to hear that.

@jamesob I agree; potentially improving performance by not needlessly retaking the cs_main lock is the only point here.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #24178 (p2p: Respond to getheaders if we have sufficient chainwork by sdaftuar)
• #24171 (p2p: Sync chain more readily from inbound peers during IBD by sdaftuar)
• #24008 (assumeutxo: net_processing changes by jamesob)
• #22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)
• #21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
• #20827 (During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr)

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.

potentially improving performance by not needlessly retaking the cs_main lock is the only point here

AFAIU lock acquisition is essentially free if it's already held, so I would be surprised if there's a performance gain from avoiding re-acquisition.

Looking ahead at the assumeutxo work in the pipeline, IsIBD() will see further callers already holding cs_main. If the assumption holds that a thread safety annotation is preferable to retaking a lock, we might have our cake and eat it too: the majority can call the core IsIBD() with an annotation, and the seven callers sans lock can go through a wrapper with optimization and lock.

This may also have a silent merge conflict with #22053?

Pushed a different approach along the lines of #24220 (comment). It does not increase the scope of cs_main and maintains the optimization, while avoiding reacquiring the mutex during IBD for the majority of callers that already hold it.

This may also have a silent merge conflict with #22053?

With the latest push, if MaybeSendAddr no longer holds cs_main it should call IsIBD, otherwise clang provides a thread safety warning.

to reduce the use of cs_main as a recursive mutex.

I think this is a goal that is almost impossible to achieve with a mutex visible in the global scope/namespace. And if someone did it, it would be brittle and easily lead to UB.

See also how for a far simpler and private mutex (m_chainstate_mutex), it took two attempts to get right: #24235.

Personally, I think a better way would be to no longer hold cs_main for non-validation uses and then make cs_main private to chainman.

Yes. This change makes explicit which callers hold cs_main and which do not, now and going forward. Edit: updated the pull description.

Updated with @luke-jr and @PastaPastaPasta feedback (thanks!)

<!--cf906140f33d8803c4a75a2196329ecb-->

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Closing for now due to inactivity. Let us know if this should be reopened.