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

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

Pull request description:

  This is an alternative to #14803.

  Refs:
  - #14058
  - #14072
  - bitcoin/bitcoin#14803 (comment) 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?

  - bitcoin/bitcoin#14803 (comment) by @MarcoFalke

Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb

Author: laanwj

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;
-        // Ensure that CheckBlock() passes before calling AcceptBlock, as
-        // belt-and-suspenders.
-        bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
 
+        // 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());
         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