diff --git a/src/addrman.cpp b/src/addrman.cpp index b9fee8f627..8f702b5a8c 100644 --- a/src/addrman.cpp +++ b/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 setTried; std::unordered_map mapNew; @@ -487,6 +509,8 @@ int CAddrMan::Check_() void CAddrMan::GetAddr_(std::vector& vAddr, size_t max_addresses, size_t max_pct, std::optional 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& 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::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::iterator it = m_tried_collisions.begin(); diff --git a/src/addrman.h b/src/addrman.h index 4929fd2ecf..665e253192 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -231,6 +231,7 @@ public: */ template void Serialize(Stream& s_) const + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); @@ -296,10 +297,11 @@ public: template void Unserialize(Stream& s_) + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); - Clear(); + assert(vRandom.empty()); Format format; s_ >> Using>(format); @@ -452,6 +454,7 @@ public: } void Clear() + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); std::vector().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 &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(); - ret = SelectTriedCollision_(); - Check(); - } + LOCK(cs); + Check(); + const CAddrInfo 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(); - addrRet = Select_(newOnly); - Check(); - } + LOCK(cs); + Check(); + const CAddrInfo addrRet = Select_(newOnly); + Check(); return addrRet; } @@ -597,19 +589,19 @@ public: * @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) + EXCLUSIVE_LOCKS_REQUIRED(!cs) { + LOCK(cs); Check(); std::vector vAddr; - { - LOCK(cs); - GetAddr_(vAddr, max_addresses, max_pct, network); - } + 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 - mutable RecursiveMutex cs; + //! A mutex to protect the inner data structures. + 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); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 49b40924e0..eb5c37b34d 100644 --- a/src/test/addrman_tests.cpp +++ b/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; diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index b5b402829b..db0b461873 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -107,7 +107,6 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) /* max_addresses */ fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), /* max_pct */ fuzzed_data_provider.ConsumeIntegralInRange(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);