Merge #14841: consensus: Move CheckBlock() call to critical section

c5ed6e73d Move CheckBlock() call to critical section (Hennadii Stepanov)

Pull request description:

  This is an alternative to #14803.

  Refs:
  - #14058
  - #14072
  - https://github.com/bitcoin/bitcoin/pull/14803#issuecomment-442233211 by @gmaxwell
  > It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here?

  - https://github.com/bitcoin/bitcoin/pull/14803#issuecomment-442237566 by @MarcoFalke

Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb

src/validation.cpp

@@ -3530,12 +3530,14 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
         CBlockIndex *pindex = nullptr;
         if (fNewBlock) *fNewBlock = false;
         CValidationState state;
+
+        // CheckBlock() does not support multi-threaded block validation because CBlock::fChecked can cause data race.
+        // Therefore, the following critical section must include the CheckBlock() call as well.
+        LOCK(cs_main);
+
         // Ensure that CheckBlock() passes before calling AcceptBlock, as
         // belt-and-suspenders.
         bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
-
-        LOCK(cs_main);
-
         if (ret) {
             // Store to disk
             ret = g_chainstate.AcceptBlock(pblock, state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);

test/sanitizer_suppressions/tsan

@@ -1,9 +1,6 @@
 # ThreadSanitizer suppressions
 # ============================
 
-# fChecked is theoretically racy, practically only in unit tests
-race:CheckBlock
-
 # WalletBatch (unidentified deadlock)
 deadlock:WalletBatch