Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts()

[jonatack]

Following up on #22932 (comment) by Marco Falke (good observation, thank you), we can replace a cs_main lock in CBlockTreeDB::LoadBlockIndexGuts() with a Clang thread safety annotation/assertion instead. The unlocked code is reverted to its original state before #22932.

[hebasto]

ACK 20276ca, I have reviewed the code and it looks OK, I agree it can be merged.

[PastaPastaPasta]

utACK, I reviewed the code; I agree this is safe, and can be merged.

From a practical perspective, I wonder if this change (or changes like it) would see a performance gain, or if the goal is more about clean code. I also wonder if it is will result in future changes that are sub-optimal. What are the odds that something will want to call this function in the future that is not already locking cs_main?

[jonatack]

Thanks for reviewing!

From a practical perspective, I wonder if this change (or changes like it) would see a performance gain, or if the goal is more about clean code.

I think the goal is to remove any unneeded locks for performance, while using locks where needed for thread safety. If you launch bitcoind with debug=lock (or run bitcoin-cli logging [] '["lock"]' on a bitcoind instance) you'll see how much time is spent in lock contention, and in some cases it is significant. Even in faster or non-hotspot cases, taking a lock, if contended, isn't particularly cheap so it seems best to not do so needlessly.

I also wonder if it is will result in future changes that are sub-optimal. What are the odds that something will want to call this function in the future that is not already locking cs_main?

In this case, thanks to the thread safety annotation the compiler would give a warning, and the run-time assertion may be hit, both of which would serve to warn that a missing lock is needed. At this time, however, LoadBlockIndexGuts() only has one caller in the codebase.

More information about Clang thread safety analysis, if helpful:
- https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

[PastaPastaPasta]

Thanks for the response @jonatack completely agree with minimizing the time spent in critical sections (and doing so saftely)

I have a few questions if you'd be so kind :)

How expensive is locking a RecursiveMutex, if the same thread already has acquired it? Is it the same cost as acquiring an unlocked recursive mutex, or basically a no-op?

My concern is not a future caller would trigger a data-race / thread insafety, the annotations / assertions prevent that. My question is more along the line of, if we introduce a call in the future in some function WITH_LOCK(::cs_main, LoadBlockIndexGuts...); then the scope of the cs_main lock will be broader than needed correct? IE, inside of LoadBlockIndexGuts, only four lines required the cs_main lock, but we were forced to lock cs_main for the entire function call. Does that make sense?

Thanks

[maflcko]

This is only called once in init, which already holds cs_main. It doesn't really make sense to call this anywhere in a hot loop

[jonatack]

@PastaPastaPasta if you'd like to dive deeper, have a look at src/sync.{h,cpp}, https://preshing.com/20111118/locks-arent-slow-lock-contention-is/, and maybe the other links in #22736 (comment). I updated my reply above to mention contention. There are also system calls, memory/cache coherency and fences, and wait queues, for example, in addition to lock contentions.