addrman, refactor: introduce user-defined type for internal nId

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>

Github-Pull: #30568
Rebased-From: 051ba3290e
This commit is contained in:
Martin Zumsande 2024-08-01 12:46:24 -04:00 committed by fanquake
parent 7fec638222
commit 1d0411dc8f
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
3 changed files with 41 additions and 37 deletions

View file

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

View file

@ -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
*/
@ -179,36 +182,36 @@ private:
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;
@ -225,22 +228,22 @@ private:
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. */
@ -256,9 +259,9 @@ private:
/** 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);

View file

@ -186,7 +186,7 @@ public:
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;
}