Merge bitcoin/bitcoin#32592: threading: remove ancient CRITICAL_SECTION macros

46ca771 threading: remove unused template instantiations (Cory Fields)
b537a6a threading: remove obsolete critsect macros (Cory Fields)
0d0e0a3 threading: use a reverse lock rather than manual critsect macros (Cory Fields)
3ddd554 tests: Add Assertions in reverse_lock tests to exercise thread-safety annotations (Cory Fields)
c88b1cb tests: get rid of remaining manual critsect usage (Cory Fields)

Pull request description:

  Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.

  This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.

  ~While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang still believes that the mutex is locked. #32465 fixes this problem. Without that fix, this PR would potentially allow a false-negative if code were added in the future to this chunk of `getblocktemplate` which required `cs_main` to be locked.~

  ~I added a test for the reverse lock here in the form of a compiler warning in `reverselock_tests.cpp` to simulate that possibility. This PR will therefore cause a new warning (and should fail a warnings-as-errors ci check) until #32465 is merged and this is rebased on top of it.~

  Edit: Rebased on top of #32465, so this should now pass tests.

ACKs for top commit:
  maflcko:
    review ACK 46ca771 📌
  fjahr:
    Code review ACK 46ca771
  TheCharlatan:
    ACK 46ca771
  furszy:
    ACK 46ca771

Tree-SHA512: 5e423c8539ed5ddd784f5c3657bbd63be509d54942c25149f04e3764bcdf897bebf655553338d5af7b8c4f546fc1d4dd4176c2bce6f4683e76ae4bb91ba2ec80

Author: fanquake

src/rpc/mining.cpp

@@ -704,7 +704,7 @@ static RPCHelpMan getblocktemplate()
     NodeContext& node = EnsureAnyNodeContext(request.context);
     ChainstateManager& chainman = EnsureChainman(node);
     Mining& miner = EnsureMining(node);
-    LOCK(cs_main);
+    WAIT_LOCK(cs_main, csmain_lock);
     uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash};
 
     std::string strMode = "template";
@@ -810,8 +810,8 @@ static RPCHelpMan getblocktemplate()
         }
 
         // Release lock while waiting
-        LEAVE_CRITICAL_SECTION(cs_main);
         {
+            REVERSE_LOCK(csmain_lock, cs_main);
             MillisecondsDouble checktxtime{std::chrono::minutes(1)};
             while (IsRPCRunning()) {
                 // If hashWatchedChain is not a real block hash, this will
@@ -830,8 +830,6 @@ static RPCHelpMan getblocktemplate()
                 checktxtime = std::chrono::seconds(10);
             }
         }
-        ENTER_CRITICAL_SECTION(cs_main);
-
         tip = CHECK_NONFATAL(miner.getTip()).value().hash;
 
         if (!IsRPCRunning())

src/sync.cpp

@@ -206,8 +206,6 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexTyp
 {
     push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName()));
 }
-template void EnterCritical(const char*, const char*, int, Mutex*, bool);
-template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool);
 template void EnterCritical(const char*, const char*, int, std::mutex*, bool);
 template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);
 

src/sync.h

@@ -39,12 +39,6 @@ LOCK2(mutex1, mutex2);
 
 TRY_LOCK(mutex, name);
     std::unique_lock<std::recursive_mutex> name(mutex, std::try_to_lock_t);
-
-ENTER_CRITICAL_SECTION(mutex); // no RAII
-    mutex.lock();
-
-LEAVE_CRITICAL_SECTION(mutex); // no RAII
-    mutex.unlock();
  */
 
 ///////////////////////////////
@@ -270,20 +264,6 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE
 #define TRY_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs), true)
 #define WAIT_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs))
 
-#define ENTER_CRITICAL_SECTION(cs)                            \
-    {                                                         \
-        EnterCritical(#cs, __FILE__, __LINE__, &cs); \
-        (cs).lock();                                          \
-    }
-
-#define LEAVE_CRITICAL_SECTION(cs)                                          \
-    {                                                                       \
-        std::string lockname;                                               \
-        CheckLastCritical((void*)(&cs), lockname, #cs, __FILE__, __LINE__); \
-        (cs).unlock();                                                      \
-        LeaveCritical();                                                    \
-    }
-
 //! Run code while locking a mutex.
 //!
 //! Examples:

src/test/reverselock_tests.cpp

@@ -17,8 +17,10 @@ BOOST_AUTO_TEST_CASE(reverselock_basics)
     WAIT_LOCK(mutex, lock);
 
     BOOST_CHECK(lock.owns_lock());
+    AssertLockHeld(mutex);
     {
         REVERSE_LOCK(lock, mutex);
+        AssertLockNotHeld(mutex);
         BOOST_CHECK(!lock.owns_lock());
     }
     BOOST_CHECK(lock.owns_lock());

src/test/sync_tests.cpp

@@ -37,8 +37,7 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
 template <typename MutexType>
 void TestDoubleLock2(MutexType& m)
 {
-    ENTER_CRITICAL_SECTION(m);
-    LEAVE_CRITICAL_SECTION(m);
+    LOCK(m);
 }
 
 template <typename MutexType>
@@ -48,31 +47,31 @@ void TestDoubleLock(bool should_throw)
     g_debug_lockorder_abort = false;
 
     MutexType m;
-    ENTER_CRITICAL_SECTION(m);
-    if (should_throw) {
-        BOOST_CHECK_EXCEPTION(TestDoubleLock2(m), std::logic_error,
+    {
+        LOCK(m);
+        if (should_throw) {
+            BOOST_CHECK_EXCEPTION(TestDoubleLock2(m), std::logic_error,
                               HasReason("double lock detected"));
-    } else {
-        BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
+        } else {
+            BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
+        }
     }
-    LEAVE_CRITICAL_SECTION(m);
-
     BOOST_CHECK(LockStackEmpty());
 
     g_debug_lockorder_abort = prev;
 }
 #endif /* DEBUG_LOCKORDER */
 
 template <typename MutexType>
-void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_THREAD_SAFETY_ANALYSIS
+void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2)
 {
-    ENTER_CRITICAL_SECTION(mutex1);
-    ENTER_CRITICAL_SECTION(mutex2);
+    {
+        WAIT_LOCK(mutex1, lock1);
+        LOCK(mutex2);
 #ifdef DEBUG_LOCKORDER
-    BOOST_CHECK_EXCEPTION(LEAVE_CRITICAL_SECTION(mutex1), std::logic_error, HasReason("mutex1 was not most recent critical section locked"));
+        BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock1, mutex1), std::logic_error, HasReason("mutex1 was not most recent critical section locked"));
 #endif // DEBUG_LOCKORDER
-    LEAVE_CRITICAL_SECTION(mutex2);
-    LEAVE_CRITICAL_SECTION(mutex1);
+    }
     BOOST_CHECK(LockStackEmpty());
 }
 } // namespace