Merge #15855: [refactor] interfaces: Add missing LockAnnotation for cs_main

fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke)

Pull request description:

  This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and  benchmarks)

ACKs for commit fa3c65:
  practicalswift:
    utACK fa3c651
  ryanofsky:
    utACK fa3c651

Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a

Author: MarcoFalke

src/bench/bench.cpp

@@ -114,6 +114,7 @@ void benchmark::BenchRunner::RunAll(Printer& printer, uint64_t num_evals, double
     for (const auto& p : benchmarks()) {
         TestingSetup test{CBaseChainParams::REGTEST};
         {
+            LOCK(cs_main);
             assert(::ChainActive().Height() == 0);
             const bool witness_enabled{IsWitnessEnabled(::ChainActive().Tip(), Params().GetConsensus())};
             assert(witness_enabled);

src/bench/duplicate_inputs.cpp

@@ -29,6 +29,7 @@ static void DuplicateInputs(benchmark::State& state)
     CMutableTransaction coinbaseTx{};
     CMutableTransaction naughtyTx{};
 
+    LOCK(cs_main);
     CBlockIndex* pindexPrev = ::ChainActive().Tip();
     assert(pindexPrev != nullptr);
     block.nBits = GetNextWorkRequired(pindexPrev, &block, chainparams.GetConsensus());

src/interfaces/chain.cpp

@@ -41,6 +41,7 @@ class LockImpl : public Chain::Lock
 {
     Optional<int> getHeight() override
     {
+        LockAnnotation lock(::cs_main);
         int height = ::ChainActive().Height();
         if (height >= 0) {
             return height;
@@ -49,6 +50,7 @@ class LockImpl : public Chain::Lock
     }
     Optional<int> getBlockHeight(const uint256& hash) override
     {
+        LockAnnotation lock(::cs_main);
         CBlockIndex* block = LookupBlockIndex(hash);
         if (block && ::ChainActive().Contains(block)) {
             return block->nHeight;
@@ -63,29 +65,34 @@ class LockImpl : public Chain::Lock
     }
     uint256 getBlockHash(int height) override
     {
+        LockAnnotation lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetBlockHash();
     }
     int64_t getBlockTime(int height) override
     {
+        LockAnnotation lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetBlockTime();
     }
     int64_t getBlockMedianTimePast(int height) override
     {
+        LockAnnotation lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetMedianTimePast();
     }
     bool haveBlockOnDisk(int height) override
     {
+        LockAnnotation lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
     }
     Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
     {
+        LockAnnotation lock(::cs_main);
         CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
         if (block) {
             if (hash) *hash = block->GetBlockHash();
@@ -95,6 +102,7 @@ class LockImpl : public Chain::Lock
     }
     Optional<int> findPruned(int start_height, Optional<int> stop_height) override
     {
+        LockAnnotation lock(::cs_main);
         if (::fPruneMode) {
             CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip();
             while (block && block->nHeight >= start_height) {
@@ -108,6 +116,7 @@ class LockImpl : public Chain::Lock
     }
     Optional<int> findFork(const uint256& hash, Optional<int>* height) override
     {
+        LockAnnotation lock(::cs_main);
         const CBlockIndex* block = LookupBlockIndex(hash);
         const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr;
         if (height) {
@@ -122,7 +131,11 @@ class LockImpl : public Chain::Lock
         }
         return nullopt;
     }
-    CBlockLocator getTipLocator() override { return ::ChainActive().GetLocator(); }
+    CBlockLocator getTipLocator() override
+    {
+        LockAnnotation lock(::cs_main);
+        return ::ChainActive().GetLocator();
+    }
     Optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
         LockAnnotation lock(::cs_main);

src/qt/test/wallettests.cpp

@@ -145,6 +145,8 @@ void TestGUI()
     }
     {
         auto locked_chain = wallet->chain().lock();
+        LockAnnotation lock(::cs_main);
+
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
         CWallet::ScanResult result = wallet->ScanForWalletTransactions(locked_chain->getBlockHash(0), {} /* stop_block */, reserver, true /* fUpdate */);

src/test/miner_tests.cpp

@@ -8,15 +8,15 @@
 #include <consensus/merkle.h>
 #include <consensus/tx_verify.h>
 #include <consensus/validation.h>
-#include <validation.h>
 #include <miner.h>
 #include <policy/policy.h>
 #include <pubkey.h>
 #include <script/standard.h>
 #include <txmempool.h>
 #include <uint256.h>
-#include <util/system.h>
 #include <util/strencodings.h>
+#include <util/system.h>
+#include <validation.h>
 
 #include <test/setup_common.h>
 
@@ -82,7 +82,7 @@ struct {
     {2, 0xbbbeb305}, {2, 0xfe1c810a},
 };
 
-static CBlockIndex CreateBlockIndex(int nHeight)
+static CBlockIndex CreateBlockIndex(int nHeight) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     CBlockIndex index;
     index.nHeight = nHeight;

src/test/txvalidationcache_tests.cpp

@@ -66,26 +66,38 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
 
     // Test 1: block with both of those transactions should be rejected.
     block = CreateAndProcessBlock(spends, scriptPubKey);
-    BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
+    }
 
     // Test 2: ... and should be rejected if spend1 is in the memory pool
     BOOST_CHECK(ToMemPool(spends[0]));
     block = CreateAndProcessBlock(spends, scriptPubKey);
-    BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
+    }
     mempool.clear();
 
     // Test 3: ... and should be rejected if spend2 is in the memory pool
     BOOST_CHECK(ToMemPool(spends[1]));
     block = CreateAndProcessBlock(spends, scriptPubKey);
-    BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
+    }
     mempool.clear();
 
     // Final sanity test: first spend in mempool, second in block, that's OK:
     std::vector<CMutableTransaction> oneSpend;
     oneSpend.push_back(spends[0]);
     BOOST_CHECK(ToMemPool(spends[1]));
     block = CreateAndProcessBlock(oneSpend, scriptPubKey);
-    BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() == block.GetHash());
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() == block.GetHash());
+    }
     // spends[1] should have been removed from the mempool when the
     // block with spends[0] is accepted:
     BOOST_CHECK_EQUAL(mempool.size(), 0U);

src/test/util.cpp

@@ -84,6 +84,7 @@ std::shared_ptr<CBlock> PrepareBlock(const CScript& coinbase_scriptPubKey)
             .CreateNewBlock(coinbase_scriptPubKey)
             ->block);
 
+    LOCK(cs_main);
     block->nTime = ::ChainActive().Tip()->GetMedianTimePast() + 1;
     block->hashMerkleRoot = BlockMerkleRoot(*block);
 

src/test/validation_block_tests.cpp

@@ -181,6 +181,7 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
 
     UnregisterValidationInterface(&sub);
 
+    LOCK(cs_main);
     BOOST_CHECK_EQUAL(sub.m_expected_tip, ::ChainActive().Tip()->GetBlockHash());
 }
 

src/validation.cpp

@@ -3224,7 +3224,7 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
 }
 
 //! Returns last CBlockIndex* that is a checkpoint
-static CBlockIndex* GetLastCheckpoint(const CCheckpointData& data)
+static CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     const MapCheckpoints& checkpoints = data.mapCheckpoints;
 
@@ -3248,7 +3248,7 @@ static CBlockIndex* GetLastCheckpoint(const CCheckpointData& data)
  *  in ConnectBlock().
  *  Note that -reindex-chainstate skips the validation that happens here!
  */
-static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime)
+static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     assert(pindexPrev != nullptr);
     const int nHeight = pindexPrev->nHeight + 1;

src/validation.h

@@ -412,7 +412,7 @@ class CVerifyDB {
 /** Replay blocks that aren't fully applied to the database. */
 bool ReplayBlocks(const CChainParams& params, CCoinsView* view);
 
-inline CBlockIndex* LookupBlockIndex(const uint256& hash)
+inline CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     AssertLockHeld(cs_main);
     BlockMap::const_iterator it = mapBlockIndex.find(hash);