[Consensus] Protect mapBlockIndex with its own mutex

[Zannick]

Problem

tsan identified a data race on CChainState, particularly between AcceptBlock and ActivateBestChain (used often by the miner threads).

Solution

Mark mapBlockIndex as guarded by cs_mapblockindex, and ensure that the lock is taken prior to any use. Some code has been reorganized slightly to reduce the time spent holding the lock and avoid locking a second time for the same information.

Move many lookups to use LookupBlockIndex, which grabs the lock itself, and remove lots of cs_main uses that would have otherwise blocked only for LookupBlockIndex.

Tested

Original: Configured with --with-sanitizers=tsan, built with clang, run on regtest with 2 SHA mining threads enabled.
With cs_mapblockindex: configured --with-sanitizers=tsan, built with clang, run on testnet to catch up and mine 2 sha threads.

[CaveSpectre11]

One thing that bugs me is that cs_main is used as a general lock, when we should be locking individual things. Since this is targeting mapBlockIndex, do you see a reason to not create a lock specifically for mapBlockIndex so we can start separating from this giant super lock that is preventing significant multithreading to happen?

[CaveSpectre11]

This area of code was a major place of inefficiency (primarily later on since IsAncestor took way longer than chainActive.Contains, and it was being done first). It's probably not something I want to lock through the whole thing, since it's looking for stale tips that are old (PRUNE_DEPTH).

I think I would prefer locking to check if mapBlockIndex is bloated; and if so, make a copy, unlock and then let it run with the copy even if mapBlockIndex changes; so we're not locked for the whole check if it's a long check [e.g. on startup].

[Zannick]

I updated to use a separate lock, but missed this section in the subsequent passes. Will get back to this when I have a chance.

[Zannick]

Updated to split this in two critical sections, and not use cs_main at all.

[CaveSpectre11]

great work so far!

[Zannick]

Let me look into using a separate lock. I think at the time it was not especially clear whether cs_main was already locking for mapBlockIndex in some places.

[CaveSpectre11]

Let me look into using a separate lock. I think at the time it was not especially clear whether cs_main was already locking for mapBlockIndex in some places.

it was definitely requiring it (in some places ;))

inline CBlockIndex* LookupBlockIndex(const uint256& hash)
{
    AssertLockHeld(cs_main);
    BlockMap::const_iterator it = mapBlockIndex.find(hash);
    return it == mapBlockIndex.end() ? nullptr : it->second;
}

[Zannick]

tsan reported a data race in GetAncestor, so this is not ready to merge. I will have to look into that.

[Zannick]

Cherry-picked explicit default initialization in CBlockIndex, this quiets the tsan warning about a read conflict with a write in operator::new.

[codeofalltrades]

ACK ad442a4

[CaveSpectre11]

utACK 13dd025