CBlockIndex::nStatus race when pruning #17161

Disclaimer: I couldn't reproduce this issue in practice on ext4 and ntfs partitions on a x86 CPU, but I think that this issue should be solved in the long-term.

Basically, CBlockIndex::nStatus is used to hold a bunch of flags (e.g. whether the block data is currently on the hard drive, or whether the block data was verified at some point in the past). Most of this access is from validation, which happens to hold the cs_main lock. However, some of the access (e.g. background index threads, or RPC calls) does not take the cs_main lock, so the access to nStatus is not guarded for them. In theory, this could turn out to be racy in combination with pruning.

I am not sure how to solve this. Some ideas:

• Put nStatus under the cs_main lock. --> meh, this will affect validation performance when polling on RPC
• Make a "read-write-lock" for nStatus, similar to ::mempool.cs. I.e. when writing nStatus, the cs_main lock and the read lock is taken and when reading nStatus, only the read lock is taken. Maybe use a std::shared_mutex?
• Any other ideas ... !?

this will affect validation performance when polling on RPC

Where? From a quick look only getrawtransaction reads nStatus without cs_main (I may be missing other cases). For this case we could do somthing like:

    uint32_t block_status;
    {
        LOCK(cs_main);
        blockindex = LookupBlockIndexAndStatus(blockhash, block_status);

As pointed out by @ryanofsky in #13903 (comment), reading the status under a lock is not sufficient, as the block might be deleted as soon as the lock is releases.

What exactly is the problem with nStatus?

1. Torn reads? (e.g. somebody reads it while somebody else is writing it, resulting in read returning garbage - a value that was not in memory before the write, nor after the write)

2. Torn writes? (e.g. two threads write to it concurrently, resulting in garbage being stored in it - a value that was not written by either thread)

3. The access to nStatus is not synced with access to another variable?

All of the above. nStatus is both written to and read from from several threads.

Which are the other variables (for 3.)? Because 1. and 2. alone could be solved by using std::atomic_uint32_t.

atomic can't solve this, because HAVE_DATA needs to be valid until right after you read the block, not the status of it. See my previous comment, which links to #13903 (comment)

Are you saying that even #22932 does not solve the problem because even with that PR the code is:

https://github.com/bitcoin/bitcoin/blob/b1c859abb790a1bbc9c31e3a535e36d1c59085b9/src/node/blockstorage.cpp#L400-L404

and the block file may be removed from disk after GetBlockPos() completes (under cs_main) but before the if? In this case ReadBlockFromDisk() would return false.

cc @jonatack

Thanks, having a look/updating.

Not sure this should have been closed given #22932 doesn't say it actually fixed this, and at least one of the review comments said the same.

If all that is left is the ReadBlockFromDisk race, then that could be fixed with just a shared mutex guarding the reading of blocks and the removal of blocks no? This way cs_main is not blocked and blocks can still be read in parallel via shared locks.

Closing for now, as this is a theoretical issue that isn't (hopefully) exposed to users.