Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#19238: refactor: Make CAddrMan::cs non-recursive
Browse files Browse the repository at this point in the history 
ae98aec refactor: Make CAddrMan::cs non-recursive (Hennadii Stepanov)
f5d1c7f Add AssertLockHeld to CAddrMan private functions (Hennadii Stepanov)
5ef1d0b Add thread safety annotations to CAddrMan public functions (Hennadii Stepanov)
b138973 refactor: Avoid recursive locking in CAddrMan::Clear (Hennadii Stepanov)
f79a664 refactor: Apply consistent pattern for CAddrMan::Check usage (Hennadii Stepanov)
187b7d2 refactor: Avoid recursive locking in CAddrMan::Check (Hennadii Stepanov)
f77d9c7 refactor: Fix CAddrMan::Check style (Hennadii Stepanov)
0670397 Make CAddrMan::Check private (Hennadii Stepanov)
efc6fac refactor: Avoid recursive locking in CAddrMan::size (Hennadii Stepanov)
2da9554 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 ae98aec

Tree-SHA512: c3a2d3d955a75befd7e497a802b8c10730e393be9111ca263ad0464d32fae6c7edf9bd173ffb6bc9bb61c4b39073a74eba12979d47f26b0b7b4a861d100942df
  • Loading branch information
MarcoFalke committed Jun 14, 2021
2 parents 5c4f0c4 + ae98aec commit 3a2c84a
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 38 deletions.
34 changes: 33 additions & 1 deletion src/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand All @@ -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);
Expand All @@ -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];
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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;

Expand Down Expand Up @@ -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();
Expand Down
74 changes: 40 additions & 34 deletions src/addrman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ class CAddrMan
*/
template <typename Stream>
void Serialize(Stream& s_) const
EXCLUSIVE_LOCKS_REQUIRED(!cs)
{
LOCK(cs);

Expand Down Expand Up @@ -296,10 +297,11 @@ class CAddrMan

template <typename Stream>
void Unserialize(Stream& s_)
EXCLUSIVE_LOCKS_REQUIRED(!cs)
{
LOCK(cs);

Clear();
assert(vRandom.empty());

Format format;
s_ >> Using<CustomUintFormatter<1>>(format);
Expand Down Expand Up @@ -452,6 +454,7 @@ class CAddrMan
}

void Clear()
EXCLUSIVE_LOCKS_REQUIRED(!cs)
{
LOCK(cs);
std::vector<int>().swap(vRandom);
Expand Down Expand Up @@ -487,26 +490,15 @@ class CAddrMan

//! 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;
Expand All @@ -521,6 +513,7 @@ class CAddrMan

//! 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;
Expand All @@ -536,6 +529,7 @@ class CAddrMan

//! 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();
Expand All @@ -545,6 +539,7 @@ class CAddrMan

//! 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();
Expand All @@ -554,6 +549,7 @@ class CAddrMan

//! 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();
Expand All @@ -563,29 +559,25 @@ class CAddrMan

//! 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;
}

/**
* 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;
}

Expand All @@ -597,19 +589,19 @@ class CAddrMan
* @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;
{
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();
Expand All @@ -618,6 +610,7 @@ class CAddrMan
}

void SetServices(const CService &addr, ServiceFlags nServices)
EXCLUSIVE_LOCKS_REQUIRED(!cs)
{
LOCK(cs);
Check();
Expand All @@ -633,8 +626,8 @@ class CAddrMan
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 {
Expand Down Expand Up @@ -725,6 +718,19 @@ class CAddrMan
//! 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);
Expand Down
4 changes: 2 additions & 2 deletions src/test/addrman_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ class CAddrManTest : public CAddrMan
// 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;
Expand Down
1 change: 0 additions & 1 deletion src/test/fuzz/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 3a2c84a

Please sign in to comment.