diff --git a/src/addrman.cpp b/src/addrman.cpp index 8192b4eba6..91b6d3fc83 100644 --- a/src/addrman.cpp +++ b/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& vAddr, size_t max_addresses, size_t max_pct, std::optional network) +void CAddrMan::GetAddr_(std::vector& vAddr, size_t max_addresses, size_t max_pct, std::optional network) const { AssertLockHeld(cs); @@ -559,9 +563,10 @@ void CAddrMan::GetAddr_(std::vector& 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; diff --git a/src/addrman.h b/src/addrman.h index 1fc64ac07f..6347a24d55 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -55,7 +55,7 @@ private: bool fInTried{false}; //! position in vRandom - int nRandomPos{-1}; + mutable int nRandomPos{-1}; friend class CAddrMan; @@ -579,7 +579,7 @@ public: /** * 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 @@ public: * @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 GetAddr(size_t max_addresses, size_t max_pct, std::optional network) + std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); @@ -631,12 +631,12 @@ protected: 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 @@ private: std::unordered_map mapAddr GUARDED_BY(cs); //! randomly-ordered vector of all nIds - std::vector 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 vRandom GUARDED_BY(cs); // number of "tried" entries int nTried GUARDED_BY(cs); @@ -697,7 +699,7 @@ private: 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 @@ private: 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 @@ private: CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); //! Consistency check - void Check() + void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs) { #ifdef DEBUG_ADDRMAN @@ -741,7 +743,7 @@ private: #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 @@ private: * @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& vAddr, size_t max_addresses, size_t max_pct, std::optional network) EXCLUSIVE_LOCKS_REQUIRED(cs); + void GetAddr_(std::vector& vAddr, size_t max_addresses, size_t max_pct, std::optional 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() diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 5e5c5eba69..79c7102c4f 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -34,6 +34,7 @@ public: //! 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 @@ public: { CAddrMan::Clear(); if (deterministic) { + LOCK(cs); nKey.SetNull(); insecure_rand = FastRandomContext(true); } } - }; static CNetAddr ResolveIP(const std::string& ip) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 92c34e74d9..344d1dde8e 100644 --- a/src/test/fuzz/addrman.cpp +++ b/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(0, 4096), /* max_pct */ fuzzed_data_provider.ConsumeIntegralInRange(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; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 46f88c1282..acbbf357d2 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -37,6 +37,7 @@ public: //! Ensure that bucket placement is always the same for testing purposes. void MakeDeterministic() { + LOCK(cs); nKey.SetNull(); insecure_rand = FastRandomContext(true); }