phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-26 03:03:22 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions

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

Browse source 
fae108ceb5 Fix incorrect whitespace in addrman (MarcoFalke)
fa32024d51 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b77f fuzz: Actually use const addrman (MarcoFalke)
fae0c79351 refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934c8c 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 fae108ceb5
  theStack:
    re-ACK fae108ceb5 🍦

Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
This commit is contained in:
MarcoFalke 2021-08-02 12:06:54 +02:00
commit 2f60d9fce6
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
5 changed files with 46 additions and 37 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

49
src/addrman.cpp
View file

@ -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;

24
src/addrman.h
View file

@ -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<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 @@ 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<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 @@ 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<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()

3
src/test/addrman_tests.cpp
View file

@ -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)

6
src/test/fuzz/addrman.cpp
View file

@ -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;

1
src/test/net_tests.cpp
View file

@ -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);
}