From 2da95545ea42f925dbc7703e42e9356908a8c83e Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 16:27:14 +0300 Subject: [PATCH 01/10] test: Drop excessive locking in CAddrManTest::SimConnFail The unit test is single threaded, so there's no need to hold the mutex between Good() and Attempt(). This change avoids recursive locking in the CAddrMan::Attempt function. Co-authored-by: John Newbery --- src/test/addrman_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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; From efc6fac951e75ba913350bb470c3d4e6a4e284b9 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 18:54:23 +0300 Subject: [PATCH 02/10] refactor: Avoid recursive locking in CAddrMan::size --- src/addrman.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index b9fee8f627..36cd28cc58 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -362,7 +362,7 @@ void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) CAddrInfo CAddrMan::Select_(bool newOnly) { - if (size() == 0) + if (vRandom.empty()) return CAddrInfo(); if (newOnly && nNew == 0) From 06703973c758c2c5d0ff916993aa7055f609d2d7 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 19:17:23 +0300 Subject: [PATCH 03/10] Make CAddrMan::Check private Change in the addrman.h header is move-only. --- src/addrman.h | 26 +++++++++++++------------- src/test/fuzz/addrman.cpp | 1 - 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 4929fd2ecf..a11dffd048 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -492,19 +492,6 @@ public: 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) { @@ -725,6 +712,19 @@ private: //! Return a random to-be-evicted tried table address. CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); + //! Consistency check + void Check() + { +#ifdef DEBUG_ADDRMAN + { + LOCK(cs); + int err; + if ((err=Check_())) + 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/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); From f77d9c79aa41dab4285e95c9432cc6d853be67a3 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 19:23:28 +0300 Subject: [PATCH 04/10] refactor: Fix CAddrMan::Check style This change improves readability, and follows Developer Notes. --- src/addrman.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index a11dffd048..600c524bce 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -716,11 +716,10 @@ private: void Check() { #ifdef DEBUG_ADDRMAN - { - LOCK(cs); - int err; - if ((err=Check_())) - LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); + LOCK(cs); + const int err = Check_(); + if (err) { + LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); } #endif } From 187b7d2bb36e6de9cd960378021ebe690619a2ef Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 19:32:03 +0300 Subject: [PATCH 05/10] refactor: Avoid recursive locking in CAddrMan::Check --- src/addrman.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 600c524bce..3474153e36 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -585,12 +585,10 @@ public: */ std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) { + 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; } @@ -714,9 +712,10 @@ private: //! Consistency check void Check() + EXCLUSIVE_LOCKS_REQUIRED(cs) { #ifdef DEBUG_ADDRMAN - LOCK(cs); + AssertLockHeld(cs); const int err = Check_(); if (err) { LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); From f79a664314b88941c1a2796623e846d0a5916c06 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 19:40:21 +0300 Subject: [PATCH 06/10] refactor: Apply consistent pattern for CAddrMan::Check usage Co-authored-by: John Newbery --- src/addrman.h | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 3474153e36..2250d1a14c 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -551,13 +551,10 @@ public: //! Randomly select an address in tried that another address is attempting to evict. CAddrInfo SelectTriedCollision() { - CAddrInfo ret; - { - LOCK(cs); - Check(); - ret = SelectTriedCollision_(); - Check(); - } + LOCK(cs); + Check(); + const CAddrInfo ret = SelectTriedCollision_(); + Check(); return ret; } @@ -566,13 +563,10 @@ public: */ CAddrInfo Select(bool newOnly = false) { - CAddrInfo addrRet; - { - LOCK(cs); - Check(); - addrRet = Select_(newOnly); - Check(); - } + LOCK(cs); + Check(); + const CAddrInfo addrRet = Select_(newOnly); + Check(); return addrRet; } From b138973a8b4bbe061ad97011f278a21e08ea79e6 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 20:53:03 +0300 Subject: [PATCH 07/10] refactor: Avoid recursive locking in CAddrMan::Clear Co-authored-by: John Newbery --- src/addrman.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/addrman.h b/src/addrman.h index 2250d1a14c..b391bac258 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -299,7 +299,7 @@ public: { LOCK(cs); - Clear(); + assert(vRandom.empty()); Format format; s_ >> Using>(format); From 5ef1d0b6982f05f70ff2164ab9af1ac1d2f97f5d Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 20:53:25 +0300 Subject: [PATCH 08/10] Add thread safety annotations to CAddrMan public functions --- src/addrman.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/addrman.h b/src/addrman.h index b391bac258..ab97d6fb12 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,6 +297,7 @@ public: template void Unserialize(Stream& s_) + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); @@ -452,6 +454,7 @@ public: } void Clear() + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); std::vector().swap(vRandom); @@ -487,6 +490,7 @@ 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(); @@ -494,6 +498,7 @@ public: //! 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; @@ -508,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; @@ -523,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(); @@ -532,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(); @@ -541,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(); @@ -550,6 +559,7 @@ public: //! Randomly select an address in tried that another address is attempting to evict. CAddrInfo SelectTriedCollision() + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); Check(); @@ -562,6 +572,7 @@ public: * Choose an address to connect to. */ CAddrInfo Select(bool newOnly = false) + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); Check(); @@ -578,6 +589,7 @@ 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(); @@ -589,6 +601,7 @@ public: //! Outer function for Connected_() void Connected(const CService &addr, int64_t nTime = GetAdjustedTime()) + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); Check(); @@ -597,6 +610,7 @@ public: } void SetServices(const CService &addr, ServiceFlags nServices) + EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); Check(); From f5d1c7fac70f424114dae3be270fdc31589a8c34 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 21:06:10 +0300 Subject: [PATCH 09/10] Add AssertLockHeld to CAddrMan private functions --- src/addrman.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/addrman.cpp b/src/addrman.cpp index 36cd28cc58..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,6 +380,8 @@ void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) CAddrInfo CAddrMan::Select_(bool newOnly) { + AssertLockHeld(cs); + if (vRandom.empty()) return CAddrInfo(); @@ -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(); From ae98aec9c0521cdcec76459c8200bd45ff6a1485 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 23 May 2021 21:11:00 +0300 Subject: [PATCH 10/10] refactor: Make CAddrMan::cs non-recursive --- src/addrman.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index ab97d6fb12..665e253192 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -626,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 {