Merge bitcoin/bitcoin#19238: refactor: Make CAddrMan::cs non-recursive

ae98aec9c0 refactor: Make CAddrMan::cs non-recursive (Hennadii Stepanov)
f5d1c7fac7 Add AssertLockHeld to CAddrMan private functions (Hennadii Stepanov)
5ef1d0b698 Add thread safety annotations to CAddrMan public functions (Hennadii Stepanov)
b138973a8b refactor: Avoid recursive locking in CAddrMan::Clear (Hennadii Stepanov)
f79a664314 refactor: Apply consistent pattern for CAddrMan::Check usage (Hennadii Stepanov)
187b7d2bb3 refactor: Avoid recursive locking in CAddrMan::Check (Hennadii Stepanov)
f77d9c79aa refactor: Fix CAddrMan::Check style (Hennadii Stepanov)
06703973c7 Make CAddrMan::Check private (Hennadii Stepanov)
efc6fac951 refactor: Avoid recursive locking in CAddrMan::size (Hennadii Stepanov)
2da95545ea test: Drop excessive locking in CAddrManTest::SimConnFail (Hennadii Stepanov)

Pull request description:

  This PR replaces `RecursiveMutex CAddrMan::cs` with `Mutex CAddrMan::cs`.

  All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident.

  Related to #19303.

  Based on #22025, and first three commits belong to it.

ACKs for top commit:
  vasild:
    ACK ae98aec9c0

Tree-SHA512: c3a2d3d955a75befd7e497a802b8c10730e393be9111ca263ad0464d32fae6c7edf9bd173ffb6bc9bb61c4b39073a74eba12979d47f26b0b7b4a861d100942df

src/addrman.cpp

@@ -79,6 +79,8 @@ double CAddrInfo::GetChance(int64_t nNow) const
 
 CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId)
 {
+    AssertLockHeld(cs);
+
     const auto it = mapAddr.find(addr);
     if (it == mapAddr.end())
         return nullptr;
@@ -92,6 +94,8 @@ CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId)
 
 CAddrInfo* CAddrMan::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId)
 {
+    AssertLockHeld(cs);
+
     int nId = nIdCount++;
     mapInfo[nId] = CAddrInfo(addr, addrSource);
     mapAddr[addr] = nId;
@@ -104,6 +108,8 @@ CAddrInfo* CAddrMan::Create(const CAddress& addr, const CNetAddr& addrSource, in
 
 void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2)
 {
+    AssertLockHeld(cs);
+
     if (nRndPos1 == nRndPos2)
         return;
 
@@ -124,6 +130,8 @@ void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2)
 
 void CAddrMan::Delete(int nId)
 {
+    AssertLockHeld(cs);
+
     assert(mapInfo.count(nId) != 0);
     CAddrInfo& info = mapInfo[nId];
     assert(!info.fInTried);
@@ -138,6 +146,8 @@ void CAddrMan::Delete(int nId)
 
 void CAddrMan::ClearNew(int nUBucket, int nUBucketPos)
 {
+    AssertLockHeld(cs);
+
     // if there is an entry in the specified bucket, delete it.
     if (vvNew[nUBucket][nUBucketPos] != -1) {
         int nIdDelete = vvNew[nUBucket][nUBucketPos];
@@ -153,6 +163,8 @@ void CAddrMan::ClearNew(int nUBucket, int nUBucketPos)
 
 void CAddrMan::MakeTried(CAddrInfo& info, int nId)
 {
+    AssertLockHeld(cs);
+
     // remove the entry from all new buckets
     for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; bucket++) {
         int pos = info.GetBucketPosition(nKey, true, bucket);
@@ -201,6 +213,8 @@ void CAddrMan::MakeTried(CAddrInfo& info, int nId)
 
 void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
 {
+    AssertLockHeld(cs);
+
     int nId;
 
     nLastGood = nTime;
@@ -267,6 +281,8 @@ void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime
 
 bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
 {
+    AssertLockHeld(cs);
+
     if (!addr.IsRoutable())
         return false;
 
@@ -340,6 +356,8 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP
 
 void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime)
 {
+    AssertLockHeld(cs);
+
     CAddrInfo* pinfo = Find(addr);
 
     // if not found, bail out
@@ -362,7 +380,9 @@ void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime)
 
 CAddrInfo CAddrMan::Select_(bool newOnly)
 {
-    if (size() == 0)
+    AssertLockHeld(cs);
+
+    if (vRandom.empty())
         return CAddrInfo();
 
     if (newOnly && nNew == 0)
@@ -410,6 +430,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
 #ifdef DEBUG_ADDRMAN
 int CAddrMan::Check_()
 {
+    AssertLockHeld(cs);
+
     std::unordered_set<int> setTried;
     std::unordered_map<int, int> mapNew;
 
@@ -487,6 +509,8 @@ int CAddrMan::Check_()
 
 void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network)
 {
+    AssertLockHeld(cs);
+
     size_t nNodes = vRandom.size();
     if (max_pct != 0) {
         nNodes = max_pct * nNodes / 100;
@@ -519,6 +543,8 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
 
 void CAddrMan::Connected_(const CService& addr, int64_t nTime)
 {
+    AssertLockHeld(cs);
+
     CAddrInfo* pinfo = Find(addr);
 
     // if not found, bail out
@@ -539,6 +565,8 @@ void CAddrMan::Connected_(const CService& addr, int64_t nTime)
 
 void CAddrMan::SetServices_(const CService& addr, ServiceFlags nServices)
 {
+    AssertLockHeld(cs);
+
     CAddrInfo* pinfo = Find(addr);
 
     // if not found, bail out
@@ -557,6 +585,8 @@ void CAddrMan::SetServices_(const CService& addr, ServiceFlags nServices)
 
 void CAddrMan::ResolveCollisions_()
 {
+    AssertLockHeld(cs);
+
     for (std::set<int>::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) {
         int id_new = *it;
 
@@ -616,6 +646,8 @@ void CAddrMan::ResolveCollisions_()
 
 CAddrInfo CAddrMan::SelectTriedCollision_()
 {
+    AssertLockHeld(cs);
+
     if (m_tried_collisions.size() == 0) return CAddrInfo();
 
     std::set<int>::iterator it = m_tried_collisions.begin();

src/addrman.h

@@ -231,6 +231,7 @@ public:
      */
     template <typename Stream>
     void Serialize(Stream& s_) const
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
 
@@ -296,10 +297,11 @@ public:
 
     template <typename Stream>
     void Unserialize(Stream& s_)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
 
-        Clear();
+        assert(vRandom.empty());
 
         Format format;
         s_ >> Using<CustomUintFormatter<1>>(format);
@@ -452,6 +454,7 @@ public:
     }
 
     void Clear()
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         std::vector<int>().swap(vRandom);
@@ -487,26 +490,15 @@ public:
 
     //! Return the number of (unique) addresses in all tables.
     size_t size() const
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs); // TODO: Cache this in an atomic to avoid this overhead
         return vRandom.size();
     }
 
-    //! Consistency check
-    void Check()
-    {
-#ifdef DEBUG_ADDRMAN
-        {
-            LOCK(cs);
-            int err;
-            if ((err=Check_()))
-                LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
-        }
-#endif
-    }
-
     //! Add a single address.
     bool Add(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty = 0)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         bool fRet = false;
@@ -521,6 +513,7 @@ public:
 
     //! Add multiple addresses.
     bool Add(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty = 0)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         int nAdd = 0;
@@ -536,6 +529,7 @@ public:
 
     //! Mark an entry as accessible.
     void Good(const CService &addr, bool test_before_evict = true, int64_t nTime = GetAdjustedTime())
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         Check();
@@ -545,6 +539,7 @@ public:
 
     //! Mark an entry as connection attempted to.
     void Attempt(const CService &addr, bool fCountFailure, int64_t nTime = GetAdjustedTime())
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         Check();
@@ -554,6 +549,7 @@ public:
 
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
     void ResolveCollisions()
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         Check();
@@ -563,14 +559,12 @@ public:
 
     //! Randomly select an address in tried that another address is attempting to evict.
     CAddrInfo SelectTriedCollision()
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
-        CAddrInfo ret;
+        LOCK(cs);
-        {
+        Check();
-            LOCK(cs);
+        const CAddrInfo ret = SelectTriedCollision_();
-            Check();
+        Check();
-            ret = SelectTriedCollision_();
-            Check();
-        }
         return ret;
     }
 
@@ -578,14 +572,12 @@ public:
      * Choose an address to connect to.
      */
     CAddrInfo Select(bool newOnly = false)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
-        CAddrInfo addrRet;
+        LOCK(cs);
-        {
+        Check();
-            LOCK(cs);
+        const CAddrInfo addrRet = Select_(newOnly);
-            Check();
+        Check();
-            addrRet = Select_(newOnly);
-            Check();
-        }
         return addrRet;
     }
 
@@ -597,19 +589,19 @@ public:
      * @param[in] network        Select only addresses of this network (nullopt = all).
      */
     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
+        LOCK(cs);
         Check();
         std::vector<CAddress> vAddr;
-        {
+        GetAddr_(vAddr, max_addresses, max_pct, network);
-            LOCK(cs);
-            GetAddr_(vAddr, max_addresses, max_pct, network);
-        }
         Check();
         return vAddr;
     }
 
     //! Outer function for Connected_()
     void Connected(const CService &addr, int64_t nTime = GetAdjustedTime())
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         Check();
@@ -618,6 +610,7 @@ public:
     }
 
     void SetServices(const CService &addr, ServiceFlags nServices)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         Check();
@@ -633,8 +626,8 @@ protected:
     FastRandomContext insecure_rand;
 
 private:
-    //! critical section to protect the inner data structures
+    //! A mutex to protect the inner data structures.
-    mutable RecursiveMutex cs;
+    mutable Mutex cs;
 
     //! Serialization versions.
     enum Format : uint8_t {
@@ -725,6 +718,19 @@ private:
     //! Return a random to-be-evicted tried table address.
     CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
+    //! Consistency check
+    void Check()
+        EXCLUSIVE_LOCKS_REQUIRED(cs)
+    {
+#ifdef DEBUG_ADDRMAN
+        AssertLockHeld(cs);
+        const int err = Check_();
+        if (err) {
+            LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
+        }
+#endif
+    }
+
 #ifdef DEBUG_ADDRMAN
     //! Perform consistency check. Returns an error code or zero.
     int Check_() EXCLUSIVE_LOCKS_REQUIRED(cs);

src/test/addrman_tests.cpp

@@ -74,9 +74,9 @@ public:
     // Simulates connection failure so that we can test eviction of offline nodes
     void SimConnFail(const CService& addr)
     {
-         LOCK(cs);
          int64_t nLastSuccess = 1;
-         Good_(addr, true, nLastSuccess); // Set last good connection in the deep past.
+         // Set last good connection in the deep past.
+         Good(addr, true, nLastSuccess);
 
          bool count_failure = false;
          int64_t nLastTry = GetAdjustedTime()-61;

src/test/fuzz/addrman.cpp

@@ -107,7 +107,6 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
         /* max_addresses */ fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
         /* max_pct */ fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
         /* network */ std::nullopt);
-    (void)/*const_*/addr_man.Check();
     (void)/*const_*/addr_man.Select(fuzzed_data_provider.ConsumeBool());
     (void)const_addr_man.size();
     CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);