Require no cs_main lock for ProcessNewBlock/ActivateBestChain

This requires the removal of some very liberal (incorrect) cs_mains
sprinkled in some tests. It adds some chainActive.Tip() races, but
the tests are all single-threaded anyway.

src/test/miner_tests.cpp

@@ -210,7 +210,6 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     entry.dPriority = 111.0;
     entry.nHeight = 11;
 
-    LOCK(cs_main);
     fCheckpointsEnabled = false;
 
     // Simple block creation, nothing special yet:
@@ -224,28 +223,30 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     auto createAndProcessEmptyBlock = [&]() {
         int i = chainActive.Height();
         CBlock *pblock = &pemptyblocktemplate->block; // pointer for convenience
-        pblock->nVersion = 2;
+        {
-        pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1;
+            LOCK(cs_main);
-        CMutableTransaction txCoinbase(*pblock->vtx[0]);
+            pblock->nVersion = 2;
-        txCoinbase.nVersion = 1;
+            pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1;
-        txCoinbase.vin[0].scriptSig = CScript() << (chainActive.Height() + 1);
+            CMutableTransaction txCoinbase(*pblock->vtx[0]);
-        txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce);
+            txCoinbase.nVersion = 1;
-        txCoinbase.vin[0].scriptSig.push_back(chainActive.Height());
+            txCoinbase.vin[0].scriptSig = CScript() << (chainActive.Height() + 1);
-        txCoinbase.vout[0].scriptPubKey = CScript();
+            txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce);
-        pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase));
+            txCoinbase.vin[0].scriptSig.push_back(chainActive.Height());
-        if (txFirst.size() == 0)
+            txCoinbase.vout[0].scriptPubKey = CScript();
-            baseheight = chainActive.Height();
+            pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase));
-        if (txFirst.size() < 4)
+            if (txFirst.size() == 0)
-            txFirst.push_back(pblock->vtx[0]);
+                baseheight = chainActive.Height();
-        pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
+            if (txFirst.size() < 4)
-        pblock->nNonce = blockinfo[i].nonce;
+                txFirst.push_back(pblock->vtx[0]);
+            pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
+            pblock->nNonce = blockinfo[i].nonce;
 
-        // This will usually succeed in the first round as we take the nonce from blockinfo
+            // This will usually succeed in the first round as we take the nonce from blockinfo
-        // It's however usefull when adding new blocks with unknown nonces (you should add the found block to blockinfo)
+            // It's however usefull when adding new blocks with unknown nonces (you should add the found block to blockinfo)
-        while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits, chainparams.GetConsensus())) {
+            while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits, chainparams.GetConsensus())) {
-            pblock->nNonce++;
+                pblock->nNonce++;
+            }
         }
-
         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
         BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, true, NULL));
         pblock->hashPrevBlock = pblock->GetHash();
@@ -256,6 +257,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
         createAndProcessEmptyBlock();
     }
 
+    LOCK(cs_main);
+
     // Just to make sure we can still make simple blocks
     BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey));
 

src/validation.cpp

@@ -2886,6 +2886,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
     // far from a guarantee. Things in the P2P/RPC will often end up calling
     // us in the middle of ProcessNewBlock - do not assume pblock is set
     // sanely for performance or correctness!
+    AssertLockNotHeld(cs_main);
 
     CBlockIndex *pindexMostWork = NULL;
     CBlockIndex *pindexNewTip = NULL;
@@ -3599,6 +3600,8 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
 
 bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool *fNewBlock)
 {
+    AssertLockNotHeld(cs_main);
+
     {
         CBlockIndex *pindex = NULL;
         if (fNewBlock) *fNewBlock = false;

src/wallet/test/wallet_tests.cpp

@@ -363,14 +363,14 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
 
 BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
 {
-    LOCK(cs_main);
-
     // Cap last block file size, and mine new block in a new block file.
     CBlockIndex* oldTip = chainActive.Tip();
     GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE;
     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
     CBlockIndex* newTip = chainActive.Tip();
 
+    LOCK(cs_main);
+
     // Verify ScanForWalletTransactions picks up transactions in both the old
     // and new block files.
     {
@@ -435,8 +435,6 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
 BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 {
     CWallet *pwalletMainBackup = ::pwalletMain;
-    LOCK(cs_main);
-
     // Create two blocks with same timestamp to verify that importwallet rescan
     // will pick up both blocks, not just the first.
     const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 5;
@@ -450,6 +448,8 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
     SetMockTime(KEY_TIME);
     coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
 
+    LOCK(cs_main);
+
     // Import key into wallet and call dumpwallet to create backup file.
     {
         CWallet wallet;