Merge bitcoin/bitcoin#21940: refactor: Mark CAddrMan::Select and GetAddr const

fae108c Fix incorrect whitespace in addrman (MarcoFalke)
fa32024 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b fuzz: Actually use const addrman (MarcoFalke)
fae0c79 refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934 refactor: Mark CAddrMan::Select const (MarcoFalke)

Pull request description:

  To clarify that a call to this only changes the random state and nothing else.

ACKs for top commit:
  jnewbery:
    Code review ACK fae108c
  theStack:
    re-ACK fae108c 🍦

Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a

Author: MarcoFalke

src/addrman.cpp

@@ -138,7 +138,7 @@ CAddrInfo* CAddrMan::Create(const CAddress& addr, const CNetAddr& addrSource, in
     return &mapInfo[nId];
 }
 
-void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2)
+void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2) const
 {
     AssertLockHeld(cs);
 
@@ -150,11 +150,13 @@ void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2)
     int nId1 = vRandom[nRndPos1];
     int nId2 = vRandom[nRndPos2];
 
-    assert(mapInfo.count(nId1) == 1);
-    assert(mapInfo.count(nId2) == 1);
+    const auto it_1{mapInfo.find(nId1)};
+    const auto it_2{mapInfo.find(nId2)};
+    assert(it_1 != mapInfo.end());
+    assert(it_2 != mapInfo.end());
 
-    mapInfo[nId1].nRandomPos = nRndPos2;
-    mapInfo[nId2].nRandomPos = nRndPos1;
+    it_1->second.nRandomPos = nRndPos2;
+    it_2->second.nRandomPos = nRndPos1;
 
     vRandom[nRndPos1] = nId2;
     vRandom[nRndPos2] = nId1;
@@ -410,7 +412,7 @@ void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime)
     }
 }
 
-CAddrInfo CAddrMan::Select_(bool newOnly)
+CAddrInfo CAddrMan::Select_(bool newOnly) const
 {
     AssertLockHeld(cs);
 
@@ -433,8 +435,9 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
                 nKBucketPos = (nKBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE;
             }
             int nId = vvTried[nKBucket][nKBucketPos];
-            assert(mapInfo.count(nId) == 1);
-            CAddrInfo& info = mapInfo[nId];
+            const auto it_found{mapInfo.find(nId)};
+            assert(it_found != mapInfo.end());
+            const CAddrInfo& info{it_found->second};
             if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30))
                 return info;
             fChanceFactor *= 1.2;
@@ -450,8 +453,9 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
                 nUBucketPos = (nUBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE;
             }
             int nId = vvNew[nUBucket][nUBucketPos];
-            assert(mapInfo.count(nId) == 1);
-            CAddrInfo& info = mapInfo[nId];
+            const auto it_found{mapInfo.find(nId)};
+            assert(it_found != mapInfo.end());
+            const CAddrInfo& info{it_found->second};
             if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30))
                 return info;
             fChanceFactor *= 1.2;
@@ -503,15 +507,15 @@ int CAddrMan::Check_()
 
     for (int n = 0; n < ADDRMAN_TRIED_BUCKET_COUNT; n++) {
         for (int i = 0; i < ADDRMAN_BUCKET_SIZE; i++) {
-             if (vvTried[n][i] != -1) {
-                 if (!setTried.count(vvTried[n][i]))
-                     return -11;
-                 if (mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) != n)
-                     return -17;
-                 if (mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) != i)
-                     return -18;
-                 setTried.erase(vvTried[n][i]);
-             }
+            if (vvTried[n][i] != -1) {
+                if (!setTried.count(vvTried[n][i]))
+                    return -11;
+                if (mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) != n)
+                    return -17;
+                if (mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) != i)
+                    return -18;
+                setTried.erase(vvTried[n][i]);
+            }
         }
     }
 
@@ -539,7 +543,7 @@ int CAddrMan::Check_()
 }
 #endif
 
-void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network)
+void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) const
 {
     AssertLockHeld(cs);
 
@@ -559,9 +563,10 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
 
         int nRndPos = insecure_rand.randrange(vRandom.size() - n) + n;
         SwapRandom(n, nRndPos);
-        assert(mapInfo.count(vRandom[n]) == 1);
+        const auto it{mapInfo.find(vRandom[n])};
+        assert(it != mapInfo.end());
 
-        const CAddrInfo& ai = mapInfo[vRandom[n]];
+        const CAddrInfo& ai{it->second};
 
         // Filter by network (optional)
         if (network != std::nullopt && ai.GetNetClass() != network) continue;

src/addrman.h

@@ -55,7 +55,7 @@ class CAddrInfo : public CAddress
     bool fInTried{false};
 
     //! position in vRandom
-    int nRandomPos{-1};
+    mutable int nRandomPos{-1};
 
     friend class CAddrMan;
 
@@ -579,7 +579,7 @@ class CAddrMan
     /**
      * Choose an address to connect to.
      */
-    CAddrInfo Select(bool newOnly = false)
+    CAddrInfo Select(bool newOnly = false) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
@@ -596,7 +596,7 @@ class CAddrMan
      * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
      * @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)
+    std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
@@ -631,12 +631,12 @@ class CAddrMan
     uint256 nKey;
 
     //! Source of random numbers for randomization in inner loops
-    FastRandomContext insecure_rand;
+    mutable FastRandomContext insecure_rand GUARDED_BY(cs);
 
-private:
     //! A mutex to protect the inner data structures.
     mutable Mutex cs;
 
+private:
     //! Serialization versions.
     enum Format : uint8_t {
         V0_HISTORICAL = 0,    //!< historic format, before commit e6b343d88
@@ -669,7 +669,9 @@ class CAddrMan
     std::unordered_map<CNetAddr, int, CNetAddrHash> mapAddr GUARDED_BY(cs);
 
     //! randomly-ordered vector of all nIds
-    std::vector<int> vRandom GUARDED_BY(cs);
+    //! This is mutable because it is unobservable outside the class, so any
+    //! changes to it (even in const methods) are also unobservable.
+    mutable std::vector<int> vRandom GUARDED_BY(cs);
 
     // number of "tried" entries
     int nTried GUARDED_BY(cs);
@@ -697,7 +699,7 @@ class CAddrMan
     CAddrInfo* Create(const CAddress &addr, const CNetAddr &addrSource, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Swap two elements in vRandom.
-    void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Move an entry from the "new" table(s) to the "tried" table
     void MakeTried(CAddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -718,7 +720,7 @@ class CAddrMan
     void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Select an address to connect to, if newOnly is set to true, only the new table is selected from.
-    CAddrInfo Select_(bool newOnly) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    CAddrInfo Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
     void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -727,7 +729,7 @@ class CAddrMan
     CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Consistency check
-    void Check()
+    void Check() const
         EXCLUSIVE_LOCKS_REQUIRED(cs)
     {
 #ifdef DEBUG_ADDRMAN
@@ -741,7 +743,7 @@ class CAddrMan
 
 #ifdef DEBUG_ADDRMAN
     //! Perform consistency check. Returns an error code or zero.
-    int Check_() EXCLUSIVE_LOCKS_REQUIRED(cs);
+    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 #endif
 
     /**
@@ -752,7 +754,7 @@ class CAddrMan
      * @param[in] max_pct        Maximum percentage of addresses to return (0 = all).
      * @param[in] network        Select only addresses of this network (nullopt = all).
      */
-    void GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /** We have successfully connected to this peer. Calling this function
      *  updates the CAddress's nTime, which is used in our IsTerrible()

src/test/addrman_tests.cpp

@@ -34,6 +34,7 @@ class CAddrManTest : public CAddrMan
     //! Ensure that bucket placement is always the same for testing purposes.
     void MakeDeterministic()
     {
+        LOCK(cs);
         nKey.SetNull();
         insecure_rand = FastRandomContext(true);
     }
@@ -87,11 +88,11 @@ class CAddrManTest : public CAddrMan
     {
         CAddrMan::Clear();
         if (deterministic) {
+            LOCK(cs);
             nKey.SetNull();
             insecure_rand = FastRandomContext(true);
         }
     }
-
 };
 
 static CNetAddr ResolveIP(const std::string& ip)

src/test/fuzz/addrman.cpp

@@ -27,7 +27,7 @@ class CAddrManDeterministic : public CAddrMan
 public:
     void MakeDeterministic(const uint256& random_seed)
     {
-        insecure_rand = FastRandomContext{random_seed};
+        WITH_LOCK(cs, insecure_rand = FastRandomContext{random_seed});
         Clear();
     }
 };
@@ -114,11 +114,11 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
             });
     }
     const CAddrMan& const_addr_man{addr_man};
-    (void)/*const_*/addr_man.GetAddr(
+    (void)const_addr_man.GetAddr(
         /* 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.Select(fuzzed_data_provider.ConsumeBool());
+    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool());
     (void)const_addr_man.size();
     CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
     data_stream << const_addr_man;

src/test/net_tests.cpp

@@ -37,6 +37,7 @@ class CAddrManSerializationMock : public CAddrMan
     //! Ensure that bucket placement is always the same for testing purposes.
     void MakeDeterministic()
     {
+        LOCK(cs);
         nKey.SetNull();
         insecure_rand = FastRandomContext(true);
     }