Skip to content

Commit

Permalink
addrman, refactor: introduce user-defined type for internal nId
Browse files Browse the repository at this point in the history 
This makes it easier to track which spots refer to an nId
(as opposed to, for example, bucket index etc. which also use int)

Co-authored-by: Pieter Wuille <pieter@wuille.net>
  • Loading branch information
@mzumsande @sipa
mzumsande and sipa committed Aug 30, 2024
1 parent c91cabb commit 051ba32
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 37 deletions.
47 changes: 24 additions & 23 deletions src/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ void AddrManImpl::Serialize(Stream& s_) const

int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
s << nUBuckets;
std::unordered_map<int, int> mapUnkIds;
std::unordered_map<nid_type, int> mapUnkIds;
int nIds = 0;
for (const auto& entry : mapInfo) {
mapUnkIds[entry.first] = nIds;
Expand Down Expand Up @@ -398,7 +398,7 @@ void AddrManImpl::Unserialize(Stream& s_)
}
}

AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)
AddrInfo* AddrManImpl::Find(const CService& addr, nid_type* pnId)
{
AssertLockHeld(cs);

Expand All @@ -413,11 +413,11 @@ AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)
return nullptr;
}

AddrInfo* AddrManImpl::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId)
AddrInfo* AddrManImpl::Create(const CAddress& addr, const CNetAddr& addrSource, nid_type* pnId)
{
AssertLockHeld(cs);

int nId = nIdCount++;
nid_type nId = nIdCount++;
mapInfo[nId] = AddrInfo(addr, addrSource);
mapAddr[addr] = nId;
mapInfo[nId].nRandomPos = vRandom.size();
Expand All @@ -438,8 +438,8 @@ void AddrManImpl::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2) const

assert(nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size());

int nId1 = vRandom[nRndPos1];
int nId2 = vRandom[nRndPos2];
nid_type nId1 = vRandom[nRndPos1];
nid_type nId2 = vRandom[nRndPos2];

const auto it_1{mapInfo.find(nId1)};
const auto it_2{mapInfo.find(nId2)};
Expand All @@ -453,7 +453,7 @@ void AddrManImpl::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2) const
vRandom[nRndPos2] = nId1;
}

void AddrManImpl::Delete(int nId)
void AddrManImpl::Delete(nid_type nId)
{
AssertLockHeld(cs);

Expand All @@ -476,7 +476,7 @@ void AddrManImpl::ClearNew(int nUBucket, int nUBucketPos)

// if there is an entry in the specified bucket, delete it.
if (vvNew[nUBucket][nUBucketPos] != -1) {
int nIdDelete = vvNew[nUBucket][nUBucketPos];
nid_type nIdDelete = vvNew[nUBucket][nUBucketPos];
AddrInfo& infoDelete = mapInfo[nIdDelete];
assert(infoDelete.nRefCount > 0);
infoDelete.nRefCount--;
Expand All @@ -488,7 +488,7 @@ void AddrManImpl::ClearNew(int nUBucket, int nUBucketPos)
}
}

void AddrManImpl::MakeTried(AddrInfo& info, int nId)
void AddrManImpl::MakeTried(AddrInfo& info, nid_type nId)
{
AssertLockHeld(cs);

Expand All @@ -515,7 +515,7 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
// first make space to add it (the existing tried entry there is moved to new, deleting whatever is there).
if (vvTried[nKBucket][nKBucketPos] != -1) {
// find an item to evict
int nIdEvict = vvTried[nKBucket][nKBucketPos];
nid_type nIdEvict = vvTried[nKBucket][nKBucketPos];
assert(mapInfo.count(nIdEvict) == 1);
AddrInfo& infoOld = mapInfo[nIdEvict];

Expand Down Expand Up @@ -554,7 +554,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c
if (!addr.IsRoutable())
return false;

int nId;
nid_type nId;
AddrInfo* pinfo = Find(addr, &nId);

// Do not set a penalty for a source's self-announcement
Expand Down Expand Up @@ -627,7 +627,7 @@ bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, NodeSecond
{
AssertLockHeld(cs);

int nId;
nid_type nId;

m_last_good = time;

Expand Down Expand Up @@ -753,7 +753,8 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option

// Iterate over the positions of that bucket, starting at the initial one,
// and looping around.
int i, position, node_id;
int i, position;
nid_type node_id;
for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
position = (initial_position + i) % ADDRMAN_BUCKET_SIZE;
node_id = GetEntry(search_tried, bucket, position);
Expand Down Expand Up @@ -786,7 +787,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
}
}

int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const
nid_type AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const
{
AssertLockHeld(cs);

Expand Down Expand Up @@ -849,7 +850,7 @@ std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries_(bool
std::vector<std::pair<AddrInfo, AddressPosition>> infos;
for (int bucket = 0; bucket < bucket_count; ++bucket) {
for (int position = 0; position < ADDRMAN_BUCKET_SIZE; ++position) {
int id = GetEntry(from_tried, bucket, position);
nid_type id = GetEntry(from_tried, bucket, position);
if (id >= 0) {
AddrInfo info = mapInfo.at(id);
AddressPosition location = AddressPosition(
Expand Down Expand Up @@ -904,8 +905,8 @@ void AddrManImpl::ResolveCollisions_()
{
AssertLockHeld(cs);

for (std::set<int>::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) {
int id_new = *it;
for (std::set<nid_type>::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) {
nid_type id_new = *it;

bool erase_collision = false;

Expand All @@ -923,7 +924,7 @@ void AddrManImpl::ResolveCollisions_()
} else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty

// Get the to-be-evicted address that is being tested
int id_old = vvTried[tried_bucket][tried_bucket_pos];
nid_type id_old = vvTried[tried_bucket][tried_bucket_pos];
AddrInfo& info_old = mapInfo[id_old];

const auto current_time{Now<NodeSeconds>()};
Expand Down Expand Up @@ -969,11 +970,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision_()

if (m_tried_collisions.size() == 0) return {};

std::set<int>::iterator it = m_tried_collisions.begin();
std::set<nid_type>::iterator it = m_tried_collisions.begin();

// Selects a random element from m_tried_collisions
std::advance(it, insecure_rand.randrange(m_tried_collisions.size()));
int id_new = *it;
nid_type id_new = *it;

// If id_new not found in mapInfo remove it from m_tried_collisions
if (mapInfo.count(id_new) != 1) {
Expand Down Expand Up @@ -1058,15 +1059,15 @@ int AddrManImpl::CheckAddrman() const
LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(
strprintf("new %i, tried %i, total %u", nNew, nTried, vRandom.size()), BCLog::ADDRMAN);

std::unordered_set<int> setTried;
std::unordered_map<int, int> mapNew;
std::unordered_set<nid_type> setTried;
std::unordered_map<nid_type, int> mapNew;
std::unordered_map<Network, NewTriedCount> local_counts;

if (vRandom.size() != (size_t)(nTried + nNew))
return -7;

for (const auto& entry : mapInfo) {
int n = entry.first;
nid_type n = entry.first;
const AddrInfo& info = entry.second;
if (info.fInTried) {
if (!TicksSinceEpoch<std::chrono::seconds>(info.m_last_success)) {
Expand Down
29 changes: 16 additions & 13 deletions src/addrman_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ static constexpr int ADDRMAN_NEW_BUCKET_COUNT{1 << ADDRMAN_NEW_BUCKET_COUNT_LOG2
static constexpr int32_t ADDRMAN_BUCKET_SIZE_LOG2{6};
static constexpr int ADDRMAN_BUCKET_SIZE{1 << ADDRMAN_BUCKET_SIZE_LOG2};

/** User-defined type for the internally used nIds */
using nid_type = int;

/**
* Extended statistics about a CAddress
*/
Expand Down Expand Up @@ -179,36 +182,36 @@ class AddrManImpl
static constexpr uint8_t INCOMPATIBILITY_BASE = 32;

//! last used nId
int nIdCount GUARDED_BY(cs){0};
nid_type nIdCount GUARDED_BY(cs){0};

//! table with information about all nIds
std::unordered_map<int, AddrInfo> mapInfo GUARDED_BY(cs);
std::unordered_map<nid_type, AddrInfo> mapInfo GUARDED_BY(cs);

//! find an nId based on its network address and port.
std::unordered_map<CService, int, CServiceHash> mapAddr GUARDED_BY(cs);
std::unordered_map<CService, nid_type, CServiceHash> mapAddr GUARDED_BY(cs);

//! randomly-ordered vector of all nIds
//! 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);
mutable std::vector<nid_type> vRandom GUARDED_BY(cs);

// number of "tried" entries
int nTried GUARDED_BY(cs){0};

//! list of "tried" buckets
int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);
nid_type vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);

//! number of (unique) "new" entries
int nNew GUARDED_BY(cs){0};

//! list of "new" buckets
int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);
nid_type vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);

//! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse.
NodeSeconds m_last_good GUARDED_BY(cs){1s};

//! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
std::set<int> m_tried_collisions;
std::set<nid_type> m_tried_collisions;

/** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */
const int32_t m_consistency_check_ratio;
Expand All @@ -225,22 +228,22 @@ class AddrManImpl
std::unordered_map<Network, NewTriedCount> m_network_counts GUARDED_BY(cs);

//! Find an entry.
AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
AddrInfo* Find(const CService& addr, nid_type* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);

//! Create a new entry and add it to the internal data structures mapInfo, mapAddr and vRandom.
AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, nid_type* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);

//! Swap two elements in vRandom.
void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2) const EXCLUSIVE_LOCKS_REQUIRED(cs);

//! Delete an entry. It must not be in tried, and have refcount 0.
void Delete(int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);
void Delete(nid_type nId) EXCLUSIVE_LOCKS_REQUIRED(cs);

//! Clear a position in a "new" table. This is the only place where entries are actually deleted.
void ClearNew(int nUBucket, int nUBucketPos) EXCLUSIVE_LOCKS_REQUIRED(cs);

//! Move an entry from the "new" table(s) to the "tried" table
void MakeTried(AddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);
void MakeTried(AddrInfo& info, nid_type nId) EXCLUSIVE_LOCKS_REQUIRED(cs);

/** Attempt to add a single address to addrman's new table.
* @see AddrMan::Add() for parameters. */
Expand All @@ -256,9 +259,9 @@ class AddrManImpl

/** Helper to generalize looking up an addrman entry from either table.
*
* @return int The nid of the entry. If the addrman position is empty or not found, returns -1.
* @return nid_type The nid of the entry. If the addrman position is empty or not found, returns -1.
* */
int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
nid_type GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);

std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);

Expand Down
2 changes: 1 addition & 1 deletion src/test/fuzz/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class AddrManDeterministic : public AddrMan
return false;
}

auto IdsReferToSameAddress = [&](int id, int other_id) EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs, other.m_impl->cs) {
auto IdsReferToSameAddress = [&](nid_type id, nid_type other_id) EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs, other.m_impl->cs) {
if (id == -1 && other_id == -1) {
return true;
}
Expand Down

0 comments on commit 051ba32

Please sign in to comment.