Once a block is fully validated, it is saved to disk in one of the blk<nFile>.dat files in your datadir.
Blocks are received, validated and stored in an unpredictable order (and not sequentially based on block height), so we need to keep track of which file each block is stored in, in order to be able to access it quickly. This is tracked in CBlockIndex by its membersnFilenDataPos and nUndoPos. In master, all of these members are guarded by the ::cs_main mutex. We have discussed how blocks are downloaded and stored in previous meetings #24858 and #25880.
::cs_mainis a recursive mutex which is used to ensure that validation is carried out in an atomic way. Although in recent years a lot of effort has been made to reduce usage of ::cs_main, it is still heavily used across the codebase.
Having a single (global) mutex can allow for reduced code complexity and simplify reasoning about race conditions. However, it often also leads to (sometimes significant) performance issues when multiple threads are waiting for the same mutex even though they don’t need synchronization and are not accessing any of the same variables.
SharedLock is added as a new mutex type to complement the UniqueLock we already have. Why does a UniqueLock not suffice here? How are the implementations of UniqueLock and SharedLock different?
Do you expect this PR to have any visible impact on performance? If so, for which process(es) (in a very general sense) and by how much (order of magnitude)? Were you able to verify/benchmark this in practice?
This PR changes CBlockIndex::nIndex to default to -1 instead of 0. How can/did you verify that this change is safe?
nFile, nDataPos and nUndoPoschange from being guarded by ::cs_main to being guarded by g_cs_blockindex_data. Why is it that we lock exactly these 3 variables with g_cs_blockindex_data? What would be the concerns and benefits of using a different mutex for each of those 3 variables?
Are there any other ways to ensure the data integrity of nFile, nDataPos and nUndoPos?
With this PR, does the number of times that a mutex is acquired increase, stay constant, or decrease - or does it depend on the program flow?
<pakaro> tACK - compiled, ran bitcoind, unit tests, functional tests without bdb nor usdt-trace. All passed. I got a setup error when I tried to run sharedlock_tests on its own, i didn't spend much/anytime figuring that part out though.
<DaveBeer> concept ACK, for code ACK I would be concerned with global variables usage, but I'm not familiar with bitcoin core codebase much as of yet and maybe global variables are reasonable here (I understand the PR doesn't change this approach and is as good as prior solution)
<LarryRuane_> a mutex is the lowest-level syncronization primitive, just prevents two threads from running the protected ranges of code at the same time ... locks are higher-level constructs that _use_ a mutex
<stickies-v> in simpler terms: a mutex is an object that we use to help control access to one or multiple resources. a mutex can have one or multiple locks, and whoever has the lock(s) can access the underlying resources
<LarryRuane_> DaveBeer: "mutex can have any number of locks associated with it" -- I think you're right, multiple threads can be inside a `LOCK(::cs_main)` for example, and all of those are separate locks, but all refer to a single mutex
<stickies-v> so then the next conceptual question is: if the whole point of a mutex is to prevent multiple threads from accessing the same data and messing everything up for everyone... what's the point of allowing a mutex to have shared locks?!
<LarryRuane_> stickies-v: yes i was thinking that also, this PR really combines two things that could have been done separately: having a separate lock (from cs_main), and making it a shared lock ... but it's good to do both
<stickies-v> `nFile`, `nDataPos` and `nUndoPos` change from being guarded by `::cs_main` to being guarded by `g_cs_blockindex_data`. Why is it that we lock exactly these 3 variables with `g_cs_blockindex_data`? What would be the concerns and benefits of using a different mutex for each of those 3 variables?
<stickies-v> Do you expect this PR to have any visible impact on performance? If so, for which process(es) (in a very general sense) and by how much (order of magnitude)? Were you able to verify/benchmark this in practice?
<LarryRuane_> there are certain RPCs that read the block files (if the node isn't pruned), like if `txindex` is enabled, and the RPC server is multi-threaded, so those could proceed in parallel (better performance)?
<LarryRuane_> pakaro: but one thing to note is that if a particular block is in `blk00123.dat`, then its undo data is in `rev00123.dat` (for example) .. and I think the blocks are in the same order across those 2 files (but the block and undo data are different sizes, blocks are much bigger usually)
<stickies-v> LarryRuane_: yeah RPC (well, and REST) is the first thing that came to my mind too because the HTTP server uses multiple workers. for example, I think the `getblock` method could now really benefit from batch RPC requests since *most* (not all) of that logic is now no longer exclusively locked to cs_main
<stickies-v> jbes: this PR does not introduce any new threads, I think! but it does allow existing multithreaded processes that were previously contended with cs_main to become more efficient. you weren't wrong, just wanted to challenge that a bit :-)
<LarryRuane_> stickies-v: i did have a question if you don't mind... I noticed that there are "getters" for these `CBlockIndex` variables you mentioned, but they are still public instead of private.. just wondering why the PR doesn't make them private?
<stickies-v> I didn't count, but my impression is that it goes up? previously we could (and often did) just acquire a single cs_main lock for a bunch of operations, whereas now for example in `MakeBlockInfo()` we acquire a lock twice, first by calling `index->GetFileNum();` and then again by calling `index->GetFileNum();` directly after
<stickies-v> of course, even though there's a cost associated with acquiring a lock on a mutex, it can pale very very quickly in comparison to having multiple threads run concurrently, so this need not necessarily be a huge concern
<stickies-v> thank you everyone for attending and for the discussion - hope our collective understanding about locks, mutexes, concurrency and cs_main has improved a bit - and hope that furszy gets some helpful feedback on his PR!