Merge bitcoin/bitcoin#24931: Strengthen thread safety assertions

ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns)
d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37e1b2d19e5a053dbfefa30306c1d41a net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)

Pull request description:

  This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

  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; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex`  to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).

ACKs for top commit:
  MarcoFalke:
    review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦
  hebasto:
    ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e

Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529

16 files changed, 49 insertions, 23 deletions

diff --git a/src/init.cpp b/src/init.cpp
index d0fd6074b1..f6e6f7cf43 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -599,7 +599,7 @@ void SetupServerArgs(ArgsManager& argsman)
 }
 
 static bool fHaveGenesis = false;
-static Mutex g_genesis_wait_mutex;
+static GlobalMutex g_genesis_wait_mutex;
 static std::condition_variable g_genesis_wait_cv;
 
 static void BlockNotifyGenesisWait(const CBlockIndex* pBlockIndex)
diff --git a/src/net.cpp b/src/net.cpp
index 82b5a69eb5..0cdf98c40f 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -114,7 +114,7 @@ static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256
 //
 bool fDiscover = true;
 bool fListen = true;
-Mutex g_maplocalhost_mutex;
+GlobalMutex g_maplocalhost_mutex;
 std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(g_maplocalhost_mutex);
 static bool vfLimited[NET_MAX] GUARDED_BY(g_maplocalhost_mutex) = {};
 std::string strSubVersion;
diff --git a/src/net.h b/src/net.h
index 3d1a2658c7..d6b605652b 100644
--- a/src/net.h
+++ b/src/net.h
@@ -248,7 +248,7 @@ struct LocalServiceInfo {
     uint16_t nPort;
 };
 
-extern Mutex g_maplocalhost_mutex;
+extern GlobalMutex g_maplocalhost_mutex;
 extern std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(g_maplocalhost_mutex);
 
 extern const std::string NET_MESSAGE_TYPE_OTHER;
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 30d5548385..1d69975f55 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -292,7 +292,7 @@ struct Peer {
         return m_tx_relay.get();
     };
 
-    TxRelay* GetTxRelay()
+    TxRelay* GetTxRelay() EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex)
     {
         return WITH_LOCK(m_tx_relay_mutex, return m_tx_relay.get());
     };
diff --git a/src/netbase.cpp b/src/netbase.cpp
index 8ff3b7a68c..030f462ed9 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -30,7 +30,7 @@
 #endif
 
 // Settings
-static Mutex g_proxyinfo_mutex;
+static GlobalMutex g_proxyinfo_mutex;
 static Proxy proxyInfo[NET_MAX] GUARDED_BY(g_proxyinfo_mutex);
 static Proxy nameProxy GUARDED_BY(g_proxyinfo_mutex);
 int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h
index b10ffb4523..81f03a58ec 100644
--- a/src/qt/clientmodel.h
+++ b/src/qt/clientmodel.h
@@ -105,7 +105,7 @@ private:
     //! A thread to interact with m_node asynchronously
     QThread* const m_thread;
 
-    void TipChanged(SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress, bool header);
+    void TipChanged(SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress, bool header) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_tip_mutex);
     void subscribeToCoreSignals();
     void unsubscribeFromCoreSignals();
 
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index d682a7d3e8..bf4bfbb95f 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -66,7 +66,7 @@ struct CUpdatedBlock
     int height;
 };
 
-static Mutex cs_blockchange;
+static GlobalMutex cs_blockchange;
 static std::condition_variable cond_blockchange;
 static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange);
 
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index af00acdc9f..66ed18045e 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -19,14 +19,14 @@
 #include <mutex>
 #include <unordered_map>
 
-static Mutex g_rpc_warmup_mutex;
+static GlobalMutex g_rpc_warmup_mutex;
 static std::atomic<bool> g_rpc_running{false};
 static bool fRPCInWarmup GUARDED_BY(g_rpc_warmup_mutex) = true;
 static std::string rpcWarmupStatus GUARDED_BY(g_rpc_warmup_mutex) = "RPC server started";
 /* Timer-creating functions */
 static RPCTimerInterface* timerInterface = nullptr;
 /* Map of name to timer. */
-static Mutex g_deadline_timers_mutex;
+static GlobalMutex g_deadline_timers_mutex;
 static std::map<std::string, std::unique_ptr<RPCTimerBase> > deadlineTimers GUARDED_BY(g_deadline_timers_mutex);
 static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler);
 
diff --git a/src/sync.h b/src/sync.h
index a175926113..7ec4b668ac 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -129,10 +129,22 @@ using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;
 /** Wrapped mutex: supports waiting but not recursive locking */
 using Mutex = AnnotatedMixin<std::mutex>;
 
+/** Different type to mark Mutex at global scope
+ *
+ * Thread safety analysis can't handle negative assertions about mutexes
+ * with global scope well, so mark them with a separate type, and
+ * eventually move all the mutexes into classes so they are not globally
+ * visible.
+ *
+ * See: https://github.com/bitcoin/bitcoin/pull/20272#issuecomment-720755781
+ */
+class GlobalMutex : public Mutex { };
+
 #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
 
 inline void AssertLockNotHeldInline(const char* name, const char* file, int line, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(name, file, line, cs); }
 inline void AssertLockNotHeldInline(const char* name, const char* file, int line, RecursiveMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); }
+inline void AssertLockNotHeldInline(const char* name, const char* file, int line, GlobalMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); }
 #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs)
 
 /** Wrapper around std::unique_lock style lock for Mutex. */
@@ -232,12 +244,26 @@ public:
 template<typename MutexArg>
 using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;
 
-#define LOCK(cs) DebugLock<decltype(cs)> UNIQUE_NAME(criticalblock)(cs, #cs, __FILE__, __LINE__)
+// When locking a Mutex, require negative capability to ensure the lock
+// is not already held
+inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
+inline Mutex* MaybeCheckNotHeld(Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
+
+// When locking a GlobalMutex, just check it is not locked in the surrounding scope
+inline GlobalMutex& MaybeCheckNotHeld(GlobalMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; }
+inline GlobalMutex* MaybeCheckNotHeld(GlobalMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; }
+
+// When locking a RecursiveMutex, it's okay to already hold the lock
+// but check that it is not known to be locked in the surrounding scope anyway
+inline RecursiveMutex& MaybeCheckNotHeld(RecursiveMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; }
+inline RecursiveMutex* MaybeCheckNotHeld(RecursiveMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; }
+
+#define LOCK(cs) DebugLock<decltype(cs)> UNIQUE_NAME(criticalblock)(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__)
 #define LOCK2(cs1, cs2)                                               \
-    DebugLock<decltype(cs1)> criticalblock1(cs1, #cs1, __FILE__, __LINE__); \
-    DebugLock<decltype(cs2)> criticalblock2(cs2, #cs2, __FILE__, __LINE__);
-#define TRY_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__, true)
-#define WAIT_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__)
+    DebugLock<decltype(cs1)> criticalblock1(MaybeCheckNotHeld(cs1), #cs1, __FILE__, __LINE__); \
+    DebugLock<decltype(cs2)> criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__);
+#define TRY_LOCK(cs, name) DebugLock<decltype(cs)> name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true)
+#define WAIT_LOCK(cs, name) DebugLock<decltype(cs)> name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__)
 
 #define ENTER_CRITICAL_SECTION(cs)                            \
     {                                                         \
@@ -276,7 +302,7 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
 //!
 //! The above is detectable at compile-time with the -Wreturn-local-addr flag in
 //! gcc and the -Wreturn-stack-address flag in clang, both enabled by default.
-#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
+#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())
 
 class CSemaphore
 {
diff --git a/src/timedata.cpp b/src/timedata.cpp
index 7faf22aba0..d0ca609a48 100644
--- a/src/timedata.cpp
+++ b/src/timedata.cpp
@@ -16,7 +16,7 @@
 #include <util/translation.h>
 #include <warnings.h>
 
-static Mutex g_timeoffset_mutex;
+static GlobalMutex g_timeoffset_mutex;
 static int64_t nTimeOffset GUARDED_BY(g_timeoffset_mutex) = 0;
 
 /**
diff --git a/src/util/system.cpp b/src/util/system.cpp
index f88b0fac77..6e2638a5d6 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -96,7 +96,7 @@ const char * const BITCOIN_SETTINGS_FILENAME = "settings.json";
 ArgsManager gArgs;
 
 /** Mutex to protect dir_locks. */
-static Mutex cs_dir_locks;
+static GlobalMutex cs_dir_locks;
 /** A map that contains all the currently held directory locks. After
  * successful locking, these will be held here until the global destructor
  * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks
diff --git a/src/validation.cpp b/src/validation.cpp
index 23ad221ffe..1a8f501863 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -122,7 +122,7 @@ static constexpr int PRUNE_LOCK_BUFFER{10};
  */
 RecursiveMutex cs_main;
 
-Mutex g_best_block_mutex;
+GlobalMutex g_best_block_mutex;
 std::condition_variable g_best_block_cv;
 uint256 g_best_block;
 bool g_parallel_script_checks{false};
diff --git a/src/validation.h b/src/validation.h
index cc94add3cb..70b6c072e6 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -113,7 +113,7 @@ enum class SynchronizationState {
 };
 
 extern RecursiveMutex cs_main;
-extern Mutex g_best_block_mutex;
+extern GlobalMutex g_best_block_mutex;
 extern std::condition_variable g_best_block_cv;
 /** Used to notify getblocktemplate RPC of new tips. */
 extern uint256 g_best_block;
diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp
index 2515df3177..053fb8f983 100644
--- a/src/wallet/sqlite.cpp
+++ b/src/wallet/sqlite.cpp
@@ -23,7 +23,7 @@
 namespace wallet {
 static constexpr int32_t WALLET_SCHEMA_VERSION = 0;
 
-static Mutex g_sqlite_mutex;
+static GlobalMutex g_sqlite_mutex;
 static int g_sqlite_count GUARDED_BY(g_sqlite_mutex) = 0;
 
 static void ErrorLogCallback(void* arg, int code, const char* msg)
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 0c83c3d954..910562e669 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -173,8 +173,8 @@ void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr<CWallet>&
     }
 }
 
-static Mutex g_loading_wallet_mutex;
-static Mutex g_wallet_release_mutex;
+static GlobalMutex g_loading_wallet_mutex;
+static GlobalMutex g_wallet_release_mutex;
 static std::condition_variable g_wallet_release_cv;
 static std::set<std::string> g_loading_wallet_set GUARDED_BY(g_loading_wallet_mutex);
 static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex);
diff --git a/src/warnings.cpp b/src/warnings.cpp
index 60388cc713..dabb194ce1 100644
--- a/src/warnings.cpp
+++ b/src/warnings.cpp
@@ -12,7 +12,7 @@
 
 #include <vector>
 
-static Mutex g_warnings_mutex;
+static GlobalMutex g_warnings_mutex;
 static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex);
 static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false;