Strengthen AssertLockNotHeld assertions #25109

1. 
   ajtowns commented at 5:15 pm on May 11, 2022: member

   This changes AssertLockNotHeld so that it is annotated with the negative capability for the mutex it refers to. clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

   Note that this can’t reasonably be used for globals, because clang would require every function to be annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex) for each global mutex. At present, the only global mutexes that use AssertLockNotHeld are RecursiveMutex so we treat that as an exception in order to avoid having to add an excessive number of negative annotations.

2. Increase threadsafety annotation coverage 7d73f58e9c
3. sync.h: strengthen AssertLockNotHeld assertion 436ce0233c
4. 
   ajtowns commented at 5:17 pm on May 11, 2022: member

   cc @vasild @w0xlt @jonatack @hebasto

   This is the first two commits from #24931 so we can hopefully make some progress..

5. ajtowns added the label Refactoring on May 11, 2022
6. 
   DrahtBot commented at 7:24 am on May 12, 2022: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #24970 (net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer by jnewbery)
   • #24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
   • #24356 (refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() by vasild)
   • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
   • #24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
   • #24122 (refactor: replace RecursiveMutex cs_vProcessMsg with Mutex (and rename) by theStack)
   • #21878 (Make all networking code mockable by vasild)
   • #21527 (net_processing: lock clean up by ajtowns)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

7. vasild approved
8. 
   vasild commented at 2:11 pm on May 13, 2022: member

   ACK 436ce0233c276e263dcb441255dc0b881cb39cfb

   Looks like replacing LOCKS_EXCLUDED(::cs_main) with EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) or adding the latter where missing is not too many changes and is in line with the direction of this PR. Consider the diff below, then we don’t need to distinguish between Mutex and RecursiveMutex in AssertLockNotHeld().

     0diff --git i/src/index/base.h w/src/index/base.h
     1index a8f6a18c8d..989cbcf824 100644
     2--- i/src/index/base.h
     3+++ w/src/index/base.h
     4@@ -115,13 +115,13 @@ public:
     5 
     6     /// Blocks the current thread until the index is caught up to the current
     7     /// state of the block chain. This only blocks if the index has gotten in
     8     /// sync once and only needs to process blocks in the ValidationInterface
     9     /// queue. If the index is catching up from far behind, this method does
    10     /// not block and immediately returns false.
    11-    bool BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main);
    12+    bool BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
    13 
    14     void Interrupt();
    15 
    16     /// Start initializes the sync state and registers the instance as a
    17     /// ValidationInterface so that it stays in sync with blockchain updates.
    18     [[nodiscard]] bool Start(CChainState& active_chainstate);
    19diff --git i/src/net_processing.cpp w/src/net_processing.cpp
    20index 46a5e54e32..d436b63d9b 100644
    21--- i/src/net_processing.cpp
    22+++ w/src/net_processing.cpp
    23@@ -451,13 +451,13 @@ public:
    24     void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override;
    25 
    26     /** Implement NetEventsInterface */
    27     void InitializeNode(CNode* pnode) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    28     void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    29     bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
    30-        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex);
    31+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !m_peer_mutex, !m_recent_confirmed_transactions_mutex);
    32     bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing)
    33         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex);
    34 
    35     /** Implement PeerManager */
    36     void StartScheduledTasks(CScheduler& scheduler) override;
    37     void CheckForStaleTipAndEvictPeers() override;
    38@@ -467,13 +467,13 @@ public:
    39     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    40     void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    41     void SetBestHeight(int height) override { m_best_height = height; };
    42     void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    43     void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
    44                         const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
    45-        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex);
    46+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !m_peer_mutex, !m_recent_confirmed_transactions_mutex);
    47     void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override;
    48 
    49 private:
    50     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
    51     void ConsiderEviction(CNode& pto, std::chrono::seconds time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    52 
    53@@ -732,16 +732,16 @@ private:
    54     /** When our tip was last updated. */
    55     std::atomic<std::chrono::seconds> m_last_tip_update{0s};
    56 
    57     /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
    58     CTransactionRef FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main);
    59 
    60-    void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);
    61+    void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, peer.m_getdata_requests_mutex);
    62 
    63     /** Process a new block. Perform any post-processing housekeeping */
    64-    void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing);
    65+    void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
    66 
    67     /** Relay map (txid or wtxid -> CTransactionRef) */
    68     typedef std::map<uint256, CTransactionRef> MapRelay;
    69     MapRelay mapRelay GUARDED_BY(cs_main);
    70     /** Expiration-time ordered list of (expire time, relay map entry) pairs. */
    71     std::deque<std::pair<std::chrono::microseconds, MapRelay::iterator>> g_relay_expiration GUARDED_BY(cs_main);
    72diff --git i/src/qt/test/wallettests.cpp w/src/qt/test/wallettests.cpp
    73index c4cd0f4cd1..5624125121 100644
    74--- i/src/qt/test/wallettests.cpp
    75+++ w/src/qt/test/wallettests.cpp
    76@@ -139,13 +139,13 @@ void BumpFee(TransactionView& view, const uint256& txid, bool expectDisabled, st
    77 //
    78 // This also requires overriding the default minimal Qt platform:
    79 //
    80 //     QT_QPA_PLATFORM=xcb     src/qt/test/test_bitcoin-qt  # Linux
    81 //     QT_QPA_PLATFORM=windows src/qt/test/test_bitcoin-qt  # Windows
    82 //     QT_QPA_PLATFORM=cocoa   src/qt/test/test_bitcoin-qt  # macOS
    83-void TestGUI(interfaces::Node& node)
    84+void TestGUI(interfaces::Node& node) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
    85 {
    86     // Set up wallet and chain with 105 blocks (5 mature blocks for spending).
    87     TestChain100Setup test;
    88     for (int i = 0; i < 5; ++i) {
    89         test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
    90     }
    91diff --git i/src/qt/test/wallettests.h w/src/qt/test/wallettests.h
    92index 6044bedb1d..a819ad5127 100644
    93--- i/src/qt/test/wallettests.h
    94+++ w/src/qt/test/wallettests.h
    95@@ -2,12 +2,15 @@
    96 // Distributed under the MIT software license, see the accompanying
    97 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
    98 
    99 #ifndef BITCOIN_QT_TEST_WALLETTESTS_H
   100 #define BITCOIN_QT_TEST_WALLETTESTS_H
   101 
   102+#include <chain.h>
   103+#include <sync.h>
   104+
   105 #include <QObject>
   106 #include <QTest>
   107 
   108 namespace interfaces {
   109 class Node;
   110 } // namespace interfaces
   111@@ -18,10 +21,10 @@ class WalletTests : public QObject
   112     explicit WalletTests(interfaces::Node& node) : m_node(node) {}
   113     interfaces::Node& m_node;
   114 
   115     Q_OBJECT
   116 
   117 private Q_SLOTS:
   118-    void walletTests();
   119+    void walletTests() EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   120 };
   121 
   122 #endif // BITCOIN_QT_TEST_WALLETTESTS_H
   123diff --git i/src/rest.cpp w/src/rest.cpp
   124index 22b5d2e074..f26d4e1d3b 100644
   125--- i/src/rest.cpp
   126+++ w/src/rest.cpp
   127@@ -635,12 +635,13 @@ static bool rest_mempool_contents(const std::any& context, HTTPRequest* req, con
   128         return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
   129     }
   130     }
   131 }
   132 
   133 static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
   134+    EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   135 {
   136     if (!CheckWarmup(req))
   137         return false;
   138     std::string hashStr;
   139     const RESTResponseFormat rf = ParseDataFormat(hashStr, strURIPart);
   140 
   141diff --git i/src/rpc/blockchain.cpp w/src/rpc/blockchain.cpp
   142index 50bf764e53..354a391da7 100644
   143--- i/src/rpc/blockchain.cpp
   144+++ w/src/rpc/blockchain.cpp
   145@@ -130,12 +130,13 @@ static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateMan
   146 
   147         return pindex;
   148     }
   149 }
   150 
   151 UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex)
   152+    EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   153 {
   154     // Serialize passed information without accessing chain state of the active chain!
   155     AssertLockNotHeld(cs_main); // For performance reasons
   156 
   157     UniValue result(UniValue::VOBJ);
   158     result.pushKV("hash", blockindex->GetBlockHash().GetHex());
   159@@ -158,13 +159,13 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex
   160         result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex());
   161     if (pnext)
   162         result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex());
   163     return result;
   164 }
   165 
   166-UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity)
   167+UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   168 {
   169     UniValue result = blockheaderToJSON(tip, blockindex);
   170 
   171     result.pushKV("strippedsize", (int)::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS));
   172     result.pushKV("size", (int)::GetSerializeSize(block, PROTOCOL_VERSION));
   173     result.pushKV("weight", (int)::GetBlockWeight(block));
   174diff --git i/src/rpc/mempool.cpp w/src/rpc/mempool.cpp
   175index 27080d3881..d6f897b0ca 100644
   176--- i/src/rpc/mempool.cpp
   177+++ w/src/rpc/mempool.cpp
   178@@ -45,13 +45,13 @@ static RPCHelpMan sendrawtransaction()
   179             + HelpExampleCli("signrawtransactionwithwallet", "\"myhex\"") +
   180             "\nSend the transaction (signed hex)\n"
   181             + HelpExampleCli("sendrawtransaction", "\"signedhex\"") +
   182             "\nAs a JSON-RPC call\n"
   183             + HelpExampleRpc("sendrawtransaction", "\"signedhex\"")
   184                 },
   185-        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
   186+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) -> UniValue
   187         {
   188             RPCTypeCheck(request.params, {
   189                 UniValue::VSTR,
   190                 UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
   191             });
   192 
   193diff --git i/src/rpc/mining.cpp w/src/rpc/mining.cpp
   194index b552528951..fddfb04be6 100644
   195--- i/src/rpc/mining.cpp
   196+++ w/src/rpc/mining.cpp
   197@@ -111,13 +111,13 @@ static RPCHelpMan getnetworkhashps()
   198     LOCK(cs_main);
   199     return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1, chainman.ActiveChain());
   200 },
   201     };
   202 }
   203 
   204-static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t& max_tries, uint256& block_hash)
   205+static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t& max_tries, uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   206 {
   207     block_hash.SetNull();
   208     block.hashMerkleRoot = BlockMerkleRoot(block);
   209 
   210     CChainParams chainparams(Params());
   211 
   212@@ -138,13 +138,13 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
   213     }
   214 
   215     block_hash = block.GetHash();
   216     return true;
   217 }
   218 
   219-static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& mempool, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
   220+static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& mempool, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   221 {
   222     UniValue blockHashes(UniValue::VARR);
   223     while (nGenerate > 0 && !ShutdownRequested()) {
   224         std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), mempool, Params()).CreateNewBlock(coinbase_script));
   225         if (!pblocktemplate.get())
   226             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
   227@@ -212,13 +212,13 @@ static RPCHelpMan generatetodescriptor()
   228             {
   229                 {RPCResult::Type::STR_HEX, "", "blockhash"},
   230             }
   231         },
   232         RPCExamples{
   233             "\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")},
   234-        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
   235+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) -> UniValue
   236 {
   237     const int num_blocks{request.params[0].get_int()};
   238     const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].get_int()};
   239 
   240     CScript coinbase_script;
   241     std::string error;
   242@@ -259,13 +259,13 @@ static RPCHelpMan generatetoaddress()
   243          RPCExamples{
   244             "\nGenerate 11 blocks to myaddress\n"
   245             + HelpExampleCli("generatetoaddress", "11 \"myaddress\"")
   246             + "If you are using the " PACKAGE_NAME " wallet, you can get a new address to send the newly generated bitcoin to with:\n"
   247             + HelpExampleCli("getnewaddress", "")
   248                 },
   249-        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
   250+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) -> UniValue
   251 {
   252     const int num_blocks{request.params[0].get_int()};
   253     const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].get_int()};
   254 
   255     CTxDestination destination = DecodeDestination(request.params[1].get_str());
   256     if (!IsValidDestination(destination)) {
   257diff --git i/src/rpc/rawtransaction.cpp w/src/rpc/rawtransaction.cpp
   258index e8713fbd2e..1b82795952 100644
   259--- i/src/rpc/rawtransaction.cpp
   260+++ w/src/rpc/rawtransaction.cpp
   261@@ -205,13 +205,13 @@ static RPCHelpMan getrawtransaction()
   262                     HelpExampleCli("getrawtransaction", "\"mytxid\"")
   263             + HelpExampleCli("getrawtransaction", "\"mytxid\" true")
   264             + HelpExampleRpc("getrawtransaction", "\"mytxid\", true")
   265             + HelpExampleCli("getrawtransaction", "\"mytxid\" false \"myblockhash\"")
   266             + HelpExampleCli("getrawtransaction", "\"mytxid\" true \"myblockhash\"")
   267                 },
   268-        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
   269+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) -> UniValue
   270 {
   271     const NodeContext& node = EnsureAnyNodeContext(request.context);
   272     ChainstateManager& chainman = EnsureChainman(node);
   273 
   274     bool in_active_chain = true;
   275     uint256 hash = ParseHashV(request.params[0], "parameter 1");
   276diff --git i/src/test/blockfilter_index_tests.cpp w/src/test/blockfilter_index_tests.cpp
   277index 82b9617384..785ef6f852 100644
   278--- i/src/test/blockfilter_index_tests.cpp
   279+++ w/src/test/blockfilter_index_tests.cpp
   280@@ -21,13 +21,13 @@ using node::BlockAssembler;
   281 using node::CBlockTemplate;
   282 
   283 BOOST_AUTO_TEST_SUITE(blockfilter_index_tests)
   284 
   285 struct BuildChainTestingSetup : public TestChain100Setup {
   286     CBlock CreateBlock(const CBlockIndex* prev, const std::vector<CMutableTransaction>& txns, const CScript& scriptPubKey);
   287-    bool BuildChain(const CBlockIndex* pindex, const CScript& coinbase_script_pub_key, size_t length, std::vector<std::shared_ptr<CBlock>>& chain);
   288+    bool BuildChain(const CBlockIndex* pindex, const CScript& coinbase_script_pub_key, size_t length, std::vector<std::shared_ptr<CBlock>>& chain) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   289 };
   290 
   291 static bool CheckFilterLookups(BlockFilterIndex& filter_index, const CBlockIndex* block_index,
   292                                uint256& last_header)
   293 {
   294     BlockFilter expected_filter;
   295diff --git i/src/test/coinstatsindex_tests.cpp w/src/test/coinstatsindex_tests.cpp
   296index 5b73481bc1..59caa7cdde 100644
   297--- i/src/test/coinstatsindex_tests.cpp
   298+++ w/src/test/coinstatsindex_tests.cpp
   299@@ -15,13 +15,13 @@
   300 
   301 using node::CCoinsStats;
   302 using node::CoinStatsHashType;
   303 
   304 BOOST_AUTO_TEST_SUITE(coinstatsindex_tests)
   305 
   306-static void IndexWaitSynced(BaseIndex& index)
   307+static void IndexWaitSynced(BaseIndex& index) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   308 {
   309     // Allow the CoinStatsIndex to catch up with the block index that is syncing
   310     // in a background thread.
   311     const auto timeout = GetTime<std::chrono::seconds>() + 120s;
   312     while (!index.BlockUntilSyncedToCurrentChain()) {
   313         BOOST_REQUIRE(timeout > GetTime<std::chrono::milliseconds>());
   314@@ -84,13 +84,13 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
   315 
   316     // Rest of shutdown sequence and destructors happen in ~TestingSetup()
   317 }
   318 
   319 // Test shutdown between BlockConnected and ChainStateFlushed notifications,
   320 // make sure index is not corrupted and is able to reload.
   321-BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup)
   322+BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   323 {
   324     CChainState& chainstate = Assert(m_node.chainman)->ActiveChainstate();
   325     const CChainParams& params = Params();
   326     {
   327         CoinStatsIndex index{1 << 20};
   328         BOOST_REQUIRE(index.Start(chainstate));
   329diff --git i/src/test/fuzz/utxo_snapshot.cpp w/src/test/fuzz/utxo_snapshot.cpp
   330index e513f1883c..3c9b2fb76c 100644
   331--- i/src/test/fuzz/utxo_snapshot.cpp
   332+++ w/src/test/fuzz/utxo_snapshot.cpp
   333@@ -24,13 +24,13 @@ void initialize_chain()
   334 {
   335     const auto params{CreateChainParams(ArgsManager{}, CBaseChainParams::REGTEST)};
   336     static const auto chain{CreateBlockChain(2 * COINBASE_MATURITY, *params)};
   337     g_chain = &chain;
   338 }
   339 
   340-FUZZ_TARGET_INIT(utxo_snapshot, initialize_chain)
   341+FUZZ_TARGET_INIT(utxo_snapshot, initialize_chain) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   342 {
   343     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
   344     std::unique_ptr<const TestingSetup> setup{MakeNoLogFileContext<const TestingSetup>()};
   345     const auto& node = setup->m_node;
   346     auto& chainman{*node.chainman};
   347 
   348diff --git i/src/test/interfaces_tests.cpp w/src/test/interfaces_tests.cpp
   349index 49b7d2003b..a6288d8ee6 100644
   350--- i/src/test/interfaces_tests.cpp
   351+++ w/src/test/interfaces_tests.cpp
   352@@ -88,13 +88,13 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash)
   353     int height = -1;
   354     BOOST_CHECK(chain->findAncestorByHash(active[20]->GetBlockHash(), active[10]->GetBlockHash(), FoundBlock().height(height)));
   355     BOOST_CHECK_EQUAL(height, 10);
   356     BOOST_CHECK(!chain->findAncestorByHash(active[10]->GetBlockHash(), active[20]->GetBlockHash()));
   357 }
   358 
   359-BOOST_AUTO_TEST_CASE(findCommonAncestor)
   360+BOOST_AUTO_TEST_CASE(findCommonAncestor) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   361 {
   362     auto& chain = m_node.chain;
   363     const CChain& active = Assert(m_node.chainman)->ActiveChain();
   364     auto* orig_tip = active.Tip();
   365     for (int i = 0; i < 10; ++i) {
   366         BlockValidationState state;
   367diff --git i/src/test/txindex_tests.cpp w/src/test/txindex_tests.cpp
   368index 15213f826b..bce6d8b297 100644
   369--- i/src/test/txindex_tests.cpp
   370+++ w/src/test/txindex_tests.cpp
   371@@ -10,13 +10,13 @@
   372 #include <validation.h>
   373 
   374 #include <boost/test/unit_test.hpp>
   375 
   376 BOOST_AUTO_TEST_SUITE(txindex_tests)
   377 
   378-BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
   379+BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   380 {
   381     TxIndex txindex(1 << 20, true);
   382 
   383     CTransactionRef tx_disk;
   384     uint256 block_hash;
   385 
   386diff --git i/src/test/txpackage_tests.cpp w/src/test/txpackage_tests.cpp
   387index 079b753304..b08aec63f3 100644
   388--- i/src/test/txpackage_tests.cpp
   389+++ w/src/test/txpackage_tests.cpp
   390@@ -339,13 +339,13 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
   391         BOOST_CHECK(submit_deduped.m_package_feerate == std::nullopt);
   392     }
   393 }
   394 
   395 // Tests for packages containing transactions that have same-txid-different-witness equivalents in
   396 // the mempool.
   397-BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
   398+BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   399 {
   400     // Mine blocks to mature coinbases.
   401     mineBlocks(5);
   402     LOCK(cs_main);
   403 
   404     // Transactions with a same-txid-different-witness transaction in the mempool should be ignored,
   405@@ -595,13 +595,13 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
   406         BOOST_CHECK_MESSAGE(mixed_result.m_package_feerate.value() == expected_feerate,
   407                             strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(),
   408                                       mixed_result.m_package_feerate.value().ToString()));
   409     }
   410 }
   411 
   412-BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
   413+BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   414 {
   415     mineBlocks(5);
   416     LOCK(::cs_main);
   417     size_t expected_pool_size = m_node.mempool->size();
   418     CKey child_key;
   419     child_key.MakeNewKey(true);
   420diff --git i/src/test/txvalidationcache_tests.cpp w/src/test/txvalidationcache_tests.cpp
   421index d41b54af20..d8bd91cace 100644
   422--- i/src/test/txvalidationcache_tests.cpp
   423+++ w/src/test/txvalidationcache_tests.cpp
   424@@ -22,13 +22,13 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
   425                        const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
   426                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
   427                        std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
   428 
   429 BOOST_AUTO_TEST_SUITE(txvalidationcache_tests)
   430 
   431-BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup)
   432+BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   433 {
   434     // Make sure skipping validation of transactions that were
   435     // validated going into the memory pool does not allow
   436     // double-spends in blocks to pass validation when they should not.
   437 
   438     CScript scriptPubKey = CScript() <<  ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
   439diff --git i/src/test/util/mining.cpp w/src/test/util/mining.cpp
   440index 5ed8598e8e..8699c19622 100644
   441--- i/src/test/util/mining.cpp
   442+++ w/src/test/util/mining.cpp
   443@@ -57,12 +57,13 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
   444         }
   445     }
   446     return ret;
   447 }
   448 
   449 CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
   450+    EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   451 {
   452     auto block = PrepareBlock(node, coinbase_scriptPubKey);
   453 
   454     while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) {
   455         ++block->nNonce;
   456         assert(block->nNonce);
   457diff --git i/src/test/util/setup_common.h w/src/test/util/setup_common.h
   458index a1b7525cf4..69abd9437f 100644
   459--- i/src/test/util/setup_common.h
   460+++ w/src/test/util/setup_common.h
   461@@ -128,25 +128,26 @@ struct TestChain100Setup : public TestingSetup {
   462      * Create a new block with just given transactions, coinbase paying to
   463      * scriptPubKey, and try to add it to the current chain.
   464      * If no chainstate is specified, default to the active.
   465      */
   466     CBlock CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns,
   467                                  const CScript& scriptPubKey,
   468-                                 CChainState* chainstate = nullptr);
   469+                                 CChainState* chainstate = nullptr)
   470+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   471 
   472     /**
   473      * Create a new block with just given transactions, coinbase paying to
   474      * scriptPubKey.
   475      */
   476     CBlock CreateBlock(
   477         const std::vector<CMutableTransaction>& txns,
   478         const CScript& scriptPubKey,
   479         CChainState& chainstate);
   480 
   481     //! Mine a series of new blocks on the active chain.
   482-    void mineBlocks(int num_blocks);
   483+    void mineBlocks(int num_blocks) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   484 
   485     /**
   486      * Create a transaction and submit to the mempool.
   487      *
   488      * [@param](/bitcoin-bitcoin/contributor/param/) input_transaction  The transaction to spend
   489      * [@param](/bitcoin-bitcoin/contributor/param/) input_vout         The vout to spend from the input_transaction
   490diff --git i/src/test/validation_block_tests.cpp w/src/test/validation_block_tests.cpp
   491index c5b1dabcb7..7fd265b74c 100644
   492--- i/src/test/validation_block_tests.cpp
   493+++ w/src/test/validation_block_tests.cpp
   494@@ -21,16 +21,20 @@
   495 
   496 using node::BlockAssembler;
   497 
   498 namespace validation_block_tests {
   499 struct MinerTestingSetup : public RegTestingSetup {
   500     std::shared_ptr<CBlock> Block(const uint256& prev_hash);
   501-    std::shared_ptr<const CBlock> GoodBlock(const uint256& prev_hash);
   502-    std::shared_ptr<const CBlock> BadBlock(const uint256& prev_hash);
   503-    std::shared_ptr<CBlock> FinalizeBlock(std::shared_ptr<CBlock> pblock);
   504-    void BuildChain(const uint256& root, int height, const unsigned int invalid_rate, const unsigned int branch_rate, const unsigned int max_size, std::vector<std::shared_ptr<const CBlock>>& blocks);
   505+    std::shared_ptr<const CBlock> GoodBlock(const uint256& prev_hash)
   506+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   507+    std::shared_ptr<const CBlock> BadBlock(const uint256& prev_hash)
   508+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   509+    std::shared_ptr<CBlock> FinalizeBlock(std::shared_ptr<CBlock> pblock)
   510+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   511+    void BuildChain(const uint256& root, int height, const unsigned int invalid_rate, const unsigned int branch_rate, const unsigned int max_size, std::vector<std::shared_ptr<const CBlock>>& blocks)
   512+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   513 };
   514 } // namespace validation_block_tests
   515 
   516 BOOST_FIXTURE_TEST_SUITE(validation_block_tests, MinerTestingSetup)
   517 
   518 struct TestSubscriber final : public CValidationInterface {
   519@@ -143,13 +147,13 @@ void MinerTestingSetup::BuildChain(const uint256& root, int height, const unsign
   520     if (gen_fork) {
   521         blocks.push_back(GoodBlock(root));
   522         BuildChain(blocks.back()->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks);
   523     }
   524 }
   525 
   526-BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
   527+BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   528 {
   529     // build a large-ish chain that's likely to have some forks
   530     std::vector<std::shared_ptr<const CBlock>> blocks;
   531     while (blocks.size() < 50) {
   532         blocks.clear();
   533         BuildChain(Params().GenesisBlock().GetHash(), 100, 15, 10, 500, blocks);
   534@@ -171,13 +175,13 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
   535 
   536     // create a bunch of threads that repeatedly process a block generated above at random
   537     // this will create parallelism and randomness inside validation - the ValidationInterface
   538     // will subscribe to events generated during block validation and assert on ordering invariance
   539     std::vector<std::thread> threads;
   540     for (int i = 0; i < 10; i++) {
   541-        threads.emplace_back([&]() {
   542+        threads.emplace_back([&]() EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) {
   543             bool ignored;
   544             FastRandomContext insecure;
   545             for (int i = 0; i < 1000; i++) {
   546                 auto block = blocks[insecure.randrange(blocks.size() - 1)];
   547                 Assert(m_node.chainman)->ProcessNewBlock(Params(), block, true, &ignored);
   548             }
   549@@ -217,16 +221,16 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
   550  * from another thread during the reorg and checking that its size only changes
   551  * once. The size changing exactly once indicates that the polling thread's
   552  * view of the mempool is either consistent with the chain state before reorg,
   553  * or consistent with the chain state after the reorg, and not just consistent
   554  * with some intermediate state during the reorg.
   555  */
   556-BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
   557+BOOST_AUTO_TEST_CASE(mempool_locks_reorg) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   558 {
   559     bool ignored;
   560-    auto ProcessBlock = [&](std::shared_ptr<const CBlock> block) -> bool {
   561+    auto ProcessBlock = [&](std::shared_ptr<const CBlock> block) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) -> bool {
   562         return Assert(m_node.chainman)->ProcessNewBlock(Params(), block, /*force_processing=*/true, /*new_block=*/&ignored);
   563     };
   564 
   565     // Process all mined blocks
   566     BOOST_REQUIRE(ProcessBlock(std::make_shared<CBlock>(Params().GenesisBlock())));
   567     auto last_mined = GoodBlock(Params().GenesisBlock().GetHash());
   568diff --git i/src/test/validation_chainstate_tests.cpp w/src/test/validation_chainstate_tests.cpp
   569index 2a3990bb7c..12ad0fd485 100644
   570--- i/src/test/validation_chainstate_tests.cpp
   571+++ w/src/test/validation_chainstate_tests.cpp
   572@@ -75,13 +75,13 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
   573 }
   574 
   575 //! Test UpdateTip behavior for both active and background chainstates.
   576 //!
   577 //! When run on the background chainstate, UpdateTip should do a subset
   578 //! of what it does for the active chainstate.
   579-BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
   580+BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   581 {
   582     ChainstateManager& chainman = *Assert(m_node.chainman);
   583     uint256 curr_tip = ::g_best_block;
   584 
   585     // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can
   586     // be found.
   587diff --git i/src/validation.cpp w/src/validation.cpp
   588index b5d6a66088..6c8cf79649 100644
   589--- i/src/validation.cpp
   590+++ w/src/validation.cpp
   591@@ -2951,21 +2951,22 @@ static bool NotifyHeaderTip(CChainState& chainstate) LOCKS_EXCLUDED(cs_main) {
   592     if (fNotify) {
   593         uiInterface.NotifyHeaderTip(GetSynchronizationState(fInitialBlockDownload), pindexHeader);
   594     }
   595     return fNotify;
   596 }
   597 
   598-static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
   599+static void LimitValidationInterfaceQueue() EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) {
   600     AssertLockNotHeld(cs_main);
   601 
   602     if (GetMainSignals().CallbacksPending() > 10) {
   603         SyncWithValidationInterfaceQueue();
   604     }
   605 }
   606 
   607 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
   608+    EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   609 {
   610     AssertLockNotHeld(m_chainstate_mutex);
   611 
   612     // Note that while we're often called here from ProcessNewBlock, this is
   613     // far from a guarantee. Things in the P2P/RPC will often end up calling
   614     // us in the middle of ProcessNewBlock - do not assume pblock is set
   615diff --git i/src/validation.h w/src/validation.h
   616index 42e41502f9..fa1cfd778f 100644
   617--- i/src/validation.h
   618+++ w/src/validation.h
   619@@ -660,19 +660,17 @@ public:
   620     // Manual block validity manipulation:
   621     /** Mark a block as precious and reorganize.
   622      *
   623      * May not be called in a validationinterface callback.
   624      */
   625     bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
   626-        EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
   627-        LOCKS_EXCLUDED(::cs_main);
   628+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !m_chainstate_mutex);
   629 
   630     /** Mark a block as invalid. */
   631     bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
   632-        EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
   633-        LOCKS_EXCLUDED(::cs_main);
   634+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !m_chainstate_mutex);
   635 
   636     /** Remove invalidity status from a block and its descendants. */
   637     void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
   638 
   639     /** Replay blocks that aren't fully applied to the database. */
   640     bool ReplayBlocks();
   641@@ -957,26 +955,26 @@ public:
   642      *
   643      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   block The block we want to process.
   644      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   force_processing Process this block even if unrequested; used for non-network block sources.
   645      * [@param](/bitcoin-bitcoin/contributor/param/)[out]  new_block A boolean which is set to indicate if the block was first received via this call
   646      * [@returns](/bitcoin-bitcoin/contributor/returns/)     If the block was processed, independently of block validity
   647      */
   648-    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock>& block, bool force_processing, bool* new_block) LOCKS_EXCLUDED(cs_main);
   649+    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock>& block, bool force_processing, bool* new_block) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   650 
   651     /**
   652      * Process incoming block headers.
   653      *
   654      * May not be called in a
   655      * validationinterface callback.
   656      *
   657      * [@param](/bitcoin-bitcoin/contributor/param/)[in]  block The block headers themselves
   658      * [@param](/bitcoin-bitcoin/contributor/param/)[out] state This may be set to an Error state if any error occurred processing them
   659      * [@param](/bitcoin-bitcoin/contributor/param/)[in]  chainparams The params for the chain we want to connect to
   660      * [@param](/bitcoin-bitcoin/contributor/param/)[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
   661      */
   662-    bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
   663+    bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
   664 
   665     /**
   666      * Try to add a transaction to the memory pool.
   667      *
   668      * [@param](/bitcoin-bitcoin/contributor/param/)[in]  tx              The transaction to submit for mempool acceptance.
   669      * [@param](/bitcoin-bitcoin/contributor/param/)[in]  test_accept     When true, run validation checks but don't submit to mempool.
   670diff --git i/src/validationinterface.cpp w/src/validationinterface.cpp
   671index 3f7fad3f87..a1d094164e 100644
   672--- i/src/validationinterface.cpp
   673+++ w/src/validationinterface.cpp
   674@@ -154,13 +154,13 @@ void UnregisterAllValidationInterfaces()
   675 
   676 void CallFunctionInValidationInterfaceQueue(std::function<void()> func)
   677 {
   678     g_signals.m_internals->m_schedulerClient.AddToProcessQueue(std::move(func));
   679 }
   680 
   681-void SyncWithValidationInterfaceQueue()
   682+void SyncWithValidationInterfaceQueue() EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   683 {
   684     AssertLockNotHeld(cs_main);
   685     // Block until the validation queue drains
   686     std::promise<void> promise;
   687     CallFunctionInValidationInterfaceQueue([&promise] {
   688         promise.set_value();
   689diff --git i/src/wallet/bdb.cpp w/src/wallet/bdb.cpp
   690index 1aa0339445..459c64aeab 100644
   691--- i/src/wallet/bdb.cpp
   692+++ w/src/wallet/bdb.cpp
   693@@ -421,13 +421,13 @@ void BerkeleyEnvironment::CloseDb(const fs::path& filename)
   694             database.m_db->close(0);
   695             database.m_db.reset();
   696         }
   697     }
   698 }
   699 
   700-void BerkeleyEnvironment::ReloadDbEnv()
   701+void BerkeleyEnvironment::ReloadDbEnv() EXCLUSIVE_LOCKS_REQUIRED(!cs_db)
   702 {
   703     // Make sure that no Db's are in use
   704     AssertLockNotHeld(cs_db);
   705     std::unique_lock<RecursiveMutex> lock(cs_db);
   706     m_db_in_use.wait(lock, [this](){
   707         for (auto& db : m_databases) {
   708@@ -653,13 +653,13 @@ void BerkeleyDatabase::Flush()
   709 
   710 void BerkeleyDatabase::Close()
   711 {
   712     env->Flush(true);
   713 }
   714 
   715-void BerkeleyDatabase::ReloadDbEnv()
   716+void BerkeleyDatabase::ReloadDbEnv() EXCLUSIVE_LOCKS_REQUIRED(!cs_db)
   717 {
   718     env->ReloadDbEnv();
   719 }
   720 
   721 bool BerkeleyBatch::StartCursor()
   722 {
   723diff --git i/src/wallet/test/spend_tests.cpp w/src/wallet/test/spend_tests.cpp
   724index 334bd5b8bc..d10c4f1644 100644
   725--- i/src/wallet/test/spend_tests.cpp
   726+++ w/src/wallet/test/spend_tests.cpp
   727@@ -12,13 +12,13 @@
   728 
   729 #include <boost/test/unit_test.hpp>
   730 
   731 namespace wallet {
   732 BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
   733 
   734-BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
   735+BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   736 {
   737     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
   738     auto wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), m_args, coinbaseKey);
   739 
   740     // Check that a subtract-from-recipient transaction slightly less than the
   741     // coinbase input amount does not create a change output (because it would
   742diff --git i/src/wallet/test/wallet_tests.cpp w/src/wallet/test/wallet_tests.cpp
   743index 683f0eb327..06eefaf2b7 100644
   744--- i/src/wallet/test/wallet_tests.cpp
   745+++ w/src/wallet/test/wallet_tests.cpp
   746@@ -90,13 +90,13 @@ static void AddKey(CWallet& wallet, const CKey& key)
   747     std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);
   748     assert(desc);
   749     WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
   750     if (!wallet.AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
   751 }
   752 
   753-BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
   754+BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   755 {
   756     // Cap last block file size, and mine new block in a new block file.
   757     CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
   758     WITH_LOCK(::cs_main, m_node.chainman->m_blockman.GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
   759     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
   760     CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
   761@@ -194,13 +194,13 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
   762         BOOST_CHECK(result.last_scanned_block.IsNull());
   763         BOOST_CHECK(!result.last_scanned_height);
   764         BOOST_CHECK_EQUAL(GetBalance(wallet).m_mine_immature, 0);
   765     }
   766 }
   767 
   768-BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
   769+BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   770 {
   771     // Cap last block file size, and mine new block in a new block file.
   772     CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
   773     WITH_LOCK(::cs_main, m_node.chainman->m_blockman.GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
   774     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
   775     CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
   776@@ -260,13 +260,13 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
   777 }
   778 
   779 // Verify importwallet RPC starts rescan at earliest block with timestamp
   780 // greater or equal than key birthday. Previously there was a bug where
   781 // importwallet RPC would start the scan at the latest block with timestamp less
   782 // than or equal to key birthday.
   783-BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
   784+BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   785 {
   786     // Create two blocks with same timestamp to verify that importwallet rescan
   787     // will pick up both blocks, not just the first.
   788     const int64_t BLOCK_TIME = m_node.chainman->ActiveChain().Tip()->GetBlockTimeMax() + 5;
   789     SetMockTime(BLOCK_TIME);
   790     m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
   791@@ -515,13 +515,13 @@ public:
   792 
   793     ~ListCoinsTestingSetup()
   794     {
   795         wallet.reset();
   796     }
   797 
   798-    CWalletTx& AddTx(CRecipient recipient)
   799+    CWalletTx& AddTx(CRecipient recipient) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   800     {
   801         CTransactionRef tx;
   802         CAmount fee;
   803         int changePos = -1;
   804         bilingual_str error;
   805         CCoinControl dummy;
   806@@ -545,13 +545,13 @@ public:
   807         return it->second;
   808     }
   809 
   810     std::unique_ptr<CWallet> wallet;
   811 };
   812 
   813-BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
   814+BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   815 {
   816     std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString();
   817 
   818     // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey
   819     // address.
   820     std::map<CTxDestination, std::vector<COutput>> list;
   821@@ -712,13 +712,13 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup)
   822 //! the test verifies the transactions are detected before they arrive.
   823 //!
   824 //! In the second case, block and mempool transactions are created after the
   825 //! wallet rescan and notifications are immediately synced, to verify the wallet
   826 //! must already have a handler in place for them, and there's no gap after
   827 //! rescanning where new transactions in new blocks could be lost.
   828-BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
   829+BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   830 {
   831     m_args.ForceSetArg("-unsafesqlitesync", "1");
   832     // Create new wallet with known key and unload it.
   833     WalletContext context;
   834     context.args = &m_args;
   835     context.chain = m_node.chain.get();
   836@@ -786,13 +786,13 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
   837     // paying to the wallet as the wallet finishes loading and syncing the
   838     // queue so the events have to be handled immediately. Releasing the wallet
   839     // lock during the sync is a little artificial but is needed to avoid a
   840     // deadlock during the sync and simulates a new block notification happening
   841     // as soon as possible.
   842     addtx_count = 0;
   843-    auto handler = HandleLoadWallet(context, [&](std::unique_ptr<interfaces::Wallet> wallet) {
   844+    auto handler = HandleLoadWallet(context, [&](std::unique_ptr<interfaces::Wallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) {
   845             BOOST_CHECK(rescan_completed);
   846             m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
   847             block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
   848             m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
   849             mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
   850             BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
   851@@ -816,13 +816,13 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup)
   852     context.args = &m_args;
   853     auto wallet = TestLoadWallet(context);
   854     BOOST_CHECK(wallet);
   855     UnloadWallet(std::move(wallet));
   856 }
   857 
   858-BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
   859+BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main)
   860 {
   861     m_args.ForceSetArg("-unsafesqlitesync", "1");
   862     WalletContext context;
   863     context.args = &m_args;
   864     context.chain = m_node.chain.get();
   865     auto wallet = TestLoadWallet(context);
   

9. 
   MarcoFalke commented at 2:19 pm on May 13, 2022: member

   diff …

   I don’t think it is meaningful or useful to annotate lambdas, as annotations are stripped as soon as they are assigned to a std::function<...> type.

10. 
    vasild commented at 2:44 pm on May 13, 2022: member

    annotations are stripped

    Hmm:

    0void f()
    1{
    2    auto g = []() EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) { AssertLockNotHeld(::cs_main); };
    3    g(); // error: calling function 'operator()' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
    4}
    

    (clang 15)

11. 
    ajtowns commented at 3:08 pm on May 13, 2022: member

    diff …

    I think that’s large enough and independent enough that it warrants its own PR…

    I don’t think it is meaningful or useful to annotate lambdas, as annotations are stripped as soon as they are assigned to a std::function<...> type.

    That means they’re still useful when passed around via auto or templated types though. It gets pretty cumbersome anytime the lambda is passed to some generic function that wants a callback though.

12. 
    MarcoFalke commented at 3:25 pm on May 13, 2022: member

    Hmm auto

    auto in this context is not a std::function, so it may still have the annotations attached.

    That means they’re still useful when passed around via auto or templated types though.

    Eh, isn’t this what we want to avoid? For example, annotating an rpc method lambda with lock annotations will either:

    • get them stripped away (as they are assigned to a std::function)
    • pollute the rpc server code (if they are somehow templated) with global lock annotations
    • might result in a compile failure if the calling function can’t be annotated (for example it is from an external library)

    None of this sounds useful or like an improvement. The diff that @vasild posted above looks similar to the diff that I NACKed two years ago #20272#pullrequestreview-520824413

13. 
    vasild commented at 3:58 pm on May 13, 2022: member

    Well, that diff above is required if we want to avoid the two AssertLockNotHeldInline() introduced in this PR and use just the first one:

    0inline void AssertLockNotHeldInline(const char* name, const char* file, int line, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(name, file, line, cs); }
    1inline void AssertLockNotHeldInline(const char* name, const char* file, int line, RecursiveMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); }
    

    Or, in other words, to avoid this (from this PR description):

    Note that this can’t reasonably be used for globals, because clang would require every function to be annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex) for each global mutex.

    Anyway - I am ok with this PR as it is. It’s an improvement from the current situation IMO, even with the two AssertLockNotHeldInline()s.

14. 
    ajtowns commented at 5:05 pm on May 13, 2022: member

    That means they’re still useful when passed around via auto or templated types though. It gets pretty cumbersome anytime the lambda is passed to some generic function that wants a callback though.

    Eh, isn’t this what we want to avoid? For example, annotating an rpc method lambda with lock annotations will …

    Yeah, that’s what I meant by cumbersome. Could be worthwhile in some cases maybe though (I know I’ve added annotations to some lambdas which didn’t seem too bad).

15. in src/net.h:799 in 7d73f58e9c outdated

    794@@ -795,7 +795,8 @@ class CConnman
    795              bool network_active = true);
    796 
    797     ~CConnman();
    798-    bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
    799+
    800+    bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !m_added_nodes_mutex, !m_addr_fetches_mutex, !mutexMsgProc);
    

    ---

    ---

    

    MarcoFalke commented at 11:47 am on May 16, 2022:

    Why is !m_added_nodes_mutex needed? Recall that starting a thread that takes a lock has no lock annotation implications on the caller.

    ---

    

    ajtowns commented at 1:16 pm on May 16, 2022:

    It was an extra assertion needed for the subsequent commits in #24931.

    ---

    

    MarcoFalke commented at 2:40 pm on May 16, 2022:

    Ah. For reference:

    0net.cpp:2594:5: warning: calling function 'Init' requires negative capability '!m_added_nodes_mutex' [-Wthread-safety-analysis]
    1    Init(connOptions);
    2    ^
    31 warning generated.
    

16. in src/net_processing.cpp:470 in 7d73f58e9c outdated

    477-    void RelayTransaction(const uint256& txid, const uint256& wtxid) override;
    478+    void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    479+    void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    480     void SetBestHeight(int height) override { m_best_height = height; };
    481-    void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) override;
    482+    void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    

    ---

    ---

    

    MarcoFalke commented at 11:58 am on May 16, 2022:

    unrelated: This should probably be removed

    ---

    

    MarcoFalke commented at 2:25 pm on May 16, 2022:

    Attempt in #25144
17. in src/net_processing.cpp:506 in 7d73f58e9c outdated

    501@@ -495,14 +502,16 @@ class PeerManagerImpl final : public PeerManager
    502      * @return Returns true if the peer was punished (probably disconnected)
    503      */
    504     bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state,
    505-                                 bool via_compact_block, const std::string& message = "");
    506+                                 bool via_compact_block, const std::string& message = "")
    507+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    

    ---

    ---

    

    MarcoFalke commented at 12:07 pm on May 16, 2022:

    Same here, as it would be better to just pass the Peer&

    ---

    

    MarcoFalke commented at 2:12 pm on May 16, 2022:

    Tried this, but didn’t work, as the Peer might be non-existent (disconnected) at this point.
18. in src/net_processing.cpp:514 in 7d73f58e9c outdated

    511      *
    512      * @return Returns true if the peer was punished (probably disconnected)
    513      */
    514-    bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message = "");
    515+    bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message = "")
    516+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    

    ---

    ---

    

    MarcoFalke commented at 12:08 pm on May 16, 2022:

    Same

    ---

    

    MarcoFalke commented at 2:12 pm on May 16, 2022:

    Tried this, but didn’t work, as the Peer might be non-existent (disconnected) at this point.
19. in src/net_processing.cpp:530 in 7d73f58e9c outdated

    527     /** Process a single headers message from a peer. */
    528     void ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
    529                                const std::vector<CBlockHeader>& headers,
    530-                               bool via_compact_block);
    531+                               bool via_compact_block)
    532+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    

    ---

    ---

    

    MarcoFalke commented at 12:10 pm on May 16, 2022:

    Same

    ---

    

    MarcoFalke commented at 2:24 pm on May 16, 2022:

    Attempt in #25144
20. in src/net_processing.cpp:525 in 7d73f58e9c outdated

    520@@ -512,13 +521,16 @@ class PeerManagerImpl final : public PeerManager
    521      */
    522     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
    523 
    524-    void ProcessOrphanTx(std::set<uint256>& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans);
    525+    void ProcessOrphanTx(std::set<uint256>& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans)
    526+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    

    ---

    ---

    

    MarcoFalke commented at 12:10 pm on May 16, 2022:

    Same

    ---

    

    MarcoFalke commented at 2:13 pm on May 16, 2022:

    Tried this, but didn’t work, as the Peer might be non-existent (disconnected) at this point.
21. MarcoFalke approved
22. 
    MarcoFalke commented at 12:17 pm on May 16, 2022: member

    lgtm. I didn’t get the !m_added_nodes_mutex thing in Start().

    Also left some unrelated comments.

    review ACK 436ce0233c276e263dcb441255dc0b881cb39cfb 🌺

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK 436ce0233c276e263dcb441255dc0b881cb39cfb 🌺
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUj/3gv+LfVwadPBlDSmCSWLuam73ROet6lfw7CgJRFosylwqX2ePcHTQLbzDlmG
     8qaSsa3tHEuWD4wjbUbq6cNF8st78YS4W/7geDhn8yeos9US0JeuXv9XaEWxGXmjU
     9O9oDIQgJRB4bsc8lUBZ1W6do7zaQXsFepA0avd+DvJJ50EHeIMsnJ3e1TabGk2EM
    10X53qeT50vMwK0SH3UCaVVF+yEH5Hcrt1TDddzy8QlP2kKoKX80AhIlCdxa+L1nAy
    110zzzDCFNuxopDECXzBv0XizGRC2EoklCi1ajnjzmVWctLygeEWOzYAx5izL2C8qC
    12OCBjmVSRzgW4kTy5pkYO8z5KZDHb8zCoDOwicBSbaFz5n0RYaQKrFSxouhuNlkmE
    13p59l2jJN829m+jI6kI3wOWsmreRNwnm5NYUWJS2gty7J86PVHQ+Jsds63mKBdMcz
    14ZOoQ+HffljeKFIkqvp2jmTqUjxvgrPdRBpWg9kGswHVtHoRXwb9Zm8iG/GDenx5T
    15O3C5661p
    16=YVtl
    17-----END PGP SIGNATURE-----
    

23. MarcoFalke merged this on May 16, 2022
24. MarcoFalke closed this on May 16, 2022

    ---

25. 
    hebasto commented at 2:29 pm on May 16, 2022: member

    Post-merge ACK 436ce0233c276e263dcb441255dc0b881cb39cfb.
26. 
    jonatack commented at 2:33 pm on May 16, 2022: member

    Post-merge Approach ACK

    LGTM (modulo run-time assertions may be missing for the added annotations)

27. 
    w0xlt commented at 2:50 pm on May 16, 2022: contributor

    Post-merge ACK https://github.com/bitcoin/bitcoin/pull/25109/commits/436ce0233c276e263dcb441255dc0b881cb39cfb
28. sidhujag referenced this in commit 43aeec7e63 on May 28, 2022
29. DrahtBot locked this on May 16, 2023

Contributors
ajtowns DrahtBot vasild MarcoFalke hebasto jonatack w0xlt

Labels
Refactoring