Merge #16034: refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it

9f85e9cb3d scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift)
de9b5dbca3 Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift)
3a809446b3 Move LockAnnotation to make it reflect the truth (practicalswift)
cc2588579c Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift)

Pull request description:

  `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise).

  Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises.

  This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`).

  Issues like the one described in #16028 will be discovered immediately with this PR merged.

  Changes in this PR:
  * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code)
  * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth
  * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`)
  * Rename `LockAnnotation` to `LockAssertion`

ACKs for commit 9f85e9:
  ryanofsky:
    utACK 9f85e9cb3d. No changes at all since last review except clean rebase after base PR #16033 was merged

Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a

src/interfaces/chain.cpp

@@ -41,7 +41,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
 {
     Optional<int> getHeight() override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         int height = ::ChainActive().Height();
         if (height >= 0) {
             return height;
@@ -50,7 +50,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     Optional<int> getBlockHeight(const uint256& hash) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         CBlockIndex* block = LookupBlockIndex(hash);
         if (block && ::ChainActive().Contains(block)) {
             return block->nHeight;
@@ -65,34 +65,34 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     uint256 getBlockHash(int height) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetBlockHash();
     }
     int64_t getBlockTime(int height) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetBlockTime();
     }
     int64_t getBlockMedianTimePast(int height) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetMedianTimePast();
     }
     bool haveBlockOnDisk(int height) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion 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);
+        LockAssertion lock(::cs_main);
         CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
         if (block) {
             if (hash) *hash = block->GetBlockHash();
@@ -102,7 +102,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     Optional<int> findPruned(int start_height, Optional<int> stop_height) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         if (::fPruneMode) {
             CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip();
             while (block && block->nHeight >= start_height) {
@@ -116,7 +116,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     Optional<int> findFork(const uint256& hash, Optional<int>* height) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         const CBlockIndex* block = LookupBlockIndex(hash);
         const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr;
         if (height) {
@@ -133,12 +133,12 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     CBlockLocator getTipLocator() override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         return ::ChainActive().GetLocator();
     }
     Optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
             return fork->nHeight;
         }
@@ -146,12 +146,12 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     bool checkFinalTx(const CTransaction& tx) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         return CheckFinalTx(tx);
     }
     bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override
     {
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
         return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */,
             false /* bypass limits */, absurd_fee);
     }

src/qt/test/wallettests.cpp

@@ -145,7 +145,7 @@ void TestGUI()
     }
     {
         auto locked_chain = wallet->chain().lock();
-        LockAnnotation lock(::cs_main);
+        LockAssertion lock(::cs_main);
 
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();

src/sync.h

@@ -304,4 +304,18 @@ public:
     }
 };
 
+// Utility class for indicating to compiler thread analysis that a mutex is
+// locked (when it couldn't be determined otherwise).
+struct SCOPED_LOCKABLE LockAssertion
+{
+    template <typename Mutex>
+    explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
+    {
+#ifdef DEBUG_LOCKORDER
+        AssertLockHeld(mutex);
+#endif
+    }
+    ~LockAssertion() UNLOCK_FUNCTION() {}
+};
+
 #endif // BITCOIN_SYNC_H

src/threadsafety.h

@@ -54,15 +54,4 @@
 #define ASSERT_EXCLUSIVE_LOCK(...)
 #endif // __GNUC__
 
-// Utility class for indicating to compiler thread analysis that a mutex is
-// locked (when it couldn't be determined otherwise).
-struct SCOPED_LOCKABLE LockAnnotation
-{
-    template <typename Mutex>
-    explicit LockAnnotation(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
-    {
-    }
-    ~LockAnnotation() UNLOCK_FUNCTION() {}
-};
-
 #endif // BITCOIN_THREADSAFETY_H

src/wallet/test/wallet_tests.cpp

@@ -44,7 +44,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
 
     auto chain = interfaces::MakeChain();
     auto locked_chain = chain->lock();
-    LockAnnotation lock(::cs_main);
+    LockAssertion lock(::cs_main);
 
     // Verify ScanForWalletTransactions accommodates a null start block.
     {
@@ -123,7 +123,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
 
     auto chain = interfaces::MakeChain();
     auto locked_chain = chain->lock();
-    LockAnnotation lock(::cs_main);
+    LockAssertion lock(::cs_main);
 
     // Prune the older block file.
     PruneOneBlockFile(oldTip->GetBlockPos().nFile);
@@ -190,7 +190,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 
     auto chain = interfaces::MakeChain();
     auto locked_chain = chain->lock();
-    LockAnnotation lock(::cs_main);
+    LockAssertion lock(::cs_main);
 
     std::string backup_file = (SetDataDir("importwallet_rescan") / "wallet.backup").string();
 
@@ -248,7 +248,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
     CWalletTx wtx(&wallet, m_coinbase_txns.back());
 
     auto locked_chain = chain->lock();
-    LockAnnotation lock(::cs_main);
+    LockAssertion lock(::cs_main);
     LOCK(wallet.cs_wallet);
 
     wtx.hashBlock = ::ChainActive().Tip()->GetBlockHash();
@@ -272,8 +272,8 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64
     SetMockTime(mockTime);
     CBlockIndex* block = nullptr;
     if (blockTime > 0) {
-        LockAnnotation lock(::cs_main);
         auto locked_chain = wallet.chain().lock();
+        LockAssertion lock(::cs_main);
         auto inserted = mapBlockIndex.emplace(GetRandHash(), new CBlockIndex);
         assert(inserted.second);
         const uint256& hash = inserted.first->first;