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

1 files changed, 5 insertions, 3 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index 6333dd98d2..512a3619ca 100644
--- a/src/validation.cpp
+++ b/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);