From 051ba3290e30e210bfc50dea974063053313ad3e Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 1 Aug 2024 12:46:24 -0400 Subject: [PATCH] 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 --- src/addrman.cpp | 47 ++++++++++++++++++++------------------- src/addrman_impl.h | 29 +++++++++++++----------- src/test/fuzz/addrman.cpp | 2 +- 3 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 054a9bee32d5c..11ae49cfad8f3 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -188,7 +188,7 @@ void AddrManImpl::Serialize(Stream& s_) const int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30); s << nUBuckets; - std::unordered_map mapUnkIds; + std::unordered_map 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 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 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> AddrManImpl::GetEntries_(bool std::vector> 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::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) { - int id_new = *it; + for (std::set::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()}; @@ -969,11 +970,11 @@ std::pair AddrManImpl::SelectTriedCollision_() if (m_tried_collisions.size() == 0) return {}; - std::set::iterator it = m_tried_collisions.begin(); + std::set::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 setTried; - std::unordered_map mapNew; + std::unordered_set setTried; + std::unordered_map mapNew; std::unordered_map 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(info.m_last_success)) { diff --git a/src/addrman_impl.h b/src/addrman_impl.h index dd7f7b318f9b2..fcc1d8d7786f1 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -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 @@ 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 mapInfo GUARDED_BY(cs); + std::unordered_map mapInfo GUARDED_BY(cs); //! find an nId based on its network address and port. - std::unordered_map mapAddr GUARDED_BY(cs); + std::unordered_map 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 vRandom GUARDED_BY(cs); + mutable std::vector 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 m_tried_collisions; + std::set 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 @@ class AddrManImpl std::unordered_map 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 @@ 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 GetAddr_(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index dbec2bc858e05..f5310c45f5714 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -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; }