diff --git a/src/addrman.cpp b/src/addrman.cpp index 2fc6ca29a1..324bab7292 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -694,15 +694,13 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi } } -CAddrInfo AddrManImpl::Select_(bool newOnly) const +std::pair AddrManImpl::Select_(bool newOnly) const { AssertLockHeld(cs); - if (vRandom.empty()) - return CAddrInfo(); + if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) - return CAddrInfo(); + if (newOnly && nNew == 0) return {}; // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && @@ -720,8 +718,9 @@ CAddrInfo AddrManImpl::Select_(bool newOnly) const 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; + if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + return {info, info.nLastTry}; + } fChanceFactor *= 1.2; } } else { @@ -738,8 +737,9 @@ CAddrInfo AddrManImpl::Select_(bool newOnly) const 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; + if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + return {info, info.nLastTry}; + } fChanceFactor *= 1.2; } } @@ -883,11 +883,11 @@ void AddrManImpl::ResolveCollisions_() } } -CAddrInfo AddrManImpl::SelectTriedCollision_() +std::pair AddrManImpl::SelectTriedCollision_() { AssertLockHeld(cs); - if (m_tried_collisions.size() == 0) return CAddrInfo(); + if (m_tried_collisions.size() == 0) return {}; std::set::iterator it = m_tried_collisions.begin(); @@ -898,7 +898,7 @@ CAddrInfo AddrManImpl::SelectTriedCollision_() // If id_new not found in mapInfo remove it from m_tried_collisions if (mapInfo.count(id_new) != 1) { m_tried_collisions.erase(it); - return CAddrInfo(); + return {}; } const CAddrInfo& newInfo = mapInfo[id_new]; @@ -907,9 +907,8 @@ CAddrInfo AddrManImpl::SelectTriedCollision_() int tried_bucket = newInfo.GetTriedBucket(nKey, m_asmap); int tried_bucket_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket); - int id_old = vvTried[tried_bucket][tried_bucket_pos]; - - return mapInfo[id_old]; + const CAddrInfo& info_old = mapInfo[vvTried[tried_bucket][tried_bucket_pos]]; + return {info_old, info_old.nLastTry}; } void AddrManImpl::Check() const @@ -1059,20 +1058,20 @@ void AddrManImpl::ResolveCollisions() Check(); } -CAddrInfo AddrManImpl::SelectTriedCollision() +std::pair AddrManImpl::SelectTriedCollision() { LOCK(cs); Check(); - const CAddrInfo ret = SelectTriedCollision_(); + const auto ret = SelectTriedCollision_(); Check(); return ret; } -CAddrInfo AddrManImpl::Select(bool newOnly) const +std::pair AddrManImpl::Select(bool newOnly) const { LOCK(cs); Check(); - const CAddrInfo addrRet = Select_(newOnly); + const auto addrRet = Select_(newOnly); Check(); return addrRet; } @@ -1159,12 +1158,12 @@ void CAddrMan::ResolveCollisions() m_impl->ResolveCollisions(); } -CAddrInfo CAddrMan::SelectTriedCollision() +std::pair CAddrMan::SelectTriedCollision() { return m_impl->SelectTriedCollision(); } -CAddrInfo CAddrMan::Select(bool newOnly) const +std::pair CAddrMan::Select(bool newOnly) const { return m_impl->Select(newOnly); } diff --git a/src/addrman.h b/src/addrman.h index 2135295b89..d176d0a42c 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -171,13 +171,23 @@ public: //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions. void ResolveCollisions(); - //! Randomly select an address in tried that another address is attempting to evict. - CAddrInfo SelectTriedCollision(); + /** + * Randomly select an address in the tried table that another address is + * attempting to evict. + * + * @return CAddress The record for the selected tried peer. + * int64_t The last time we attempted to connect to that peer. + */ + std::pair SelectTriedCollision(); /** * Choose an address to connect to. + * + * @param[in] newOnly Whether to only select addresses from the new table. + * @return CAddress The record for the selected peer. + * int64_t The last time we attempted to connect to that peer. */ - CAddrInfo Select(bool newOnly = false) const; + std::pair Select(bool newOnly = false) const; /** * Return all or many randomly selected addresses, optionally by network. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index ee4b55e5c4..6752d5b81d 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -31,9 +31,9 @@ public: void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!cs); - CAddrInfo SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); + std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - CAddrInfo Select(bool newOnly) const + std::pair Select(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const @@ -161,7 +161,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) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** * Return all or many randomly selected addresses, optionally by network. @@ -193,7 +193,7 @@ private: void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs); //! Return a random to-be-evicted tried table address. - CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected. void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index bebf86a09d..b4c2e35f42 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -87,7 +87,7 @@ static void AddrManSelect(benchmark::Bench& bench) bench.run([&] { const auto& address = addrman.Select(); - assert(address.GetPort() > 0); + assert(address.first.GetPort() > 0); }); } diff --git a/src/net.cpp b/src/net.cpp index b8ff0b13ea..df3b88725e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2006,17 +2006,18 @@ void CConnman::ThreadOpenConnections(const std::vector connect) if (nTries > 100) break; - CAddrInfo addr; + CAddress addr; + int64_t addr_last_try{0}; if (fFeeler) { // First, try to get a tried table collision address. This returns // an empty (invalid) address if there are no collisions to try. - addr = addrman.SelectTriedCollision(); + std::tie(addr, addr_last_try) = addrman.SelectTriedCollision(); if (!addr.IsValid()) { // No tried table collisions. Select a new table address // for our feeler. - addr = addrman.Select(true); + std::tie(addr, addr_last_try) = addrman.Select(true); } else if (AlreadyConnectedToAddress(addr)) { // If test-before-evict logic would have us connect to a // peer that we're already connected to, just mark that @@ -2025,11 +2026,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // a currently-connected peer. addrman.Good(addr); // Select a new table address for our feeler instead. - addr = addrman.Select(true); + std::tie(addr, addr_last_try) = addrman.Select(true); } } else { // Not a feeler - addr = addrman.Select(); + std::tie(addr, addr_last_try) = addrman.Select(); } // Require outbound connections, other than feelers, to be to distinct network groups @@ -2046,7 +2047,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) continue; // only consider very recently tried nodes after 30 failed attempts - if (nANow - addr.nLastTry < 600 && nTries < 30) + if (nANow - addr_last_try < 600 && nTries < 30) continue; // for non-feelers, require all the services we'll want, diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 41c298e036..022f60a8ed 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -172,14 +172,14 @@ BOOST_AUTO_TEST_CASE(addrman_simple) // Test: Does Addrman respond correctly when empty. BOOST_CHECK_EQUAL(addrman->size(), 0U); - CAddrInfo addr_null = addrman->Select(); + auto addr_null = addrman->Select().first; BOOST_CHECK_EQUAL(addr_null.ToString(), "[::]:0"); // Test: Does Addrman::Add work as expected. CService addr1 = ResolveService("250.1.1.1", 8333); BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman->size(), 1U); - CAddrInfo addr_ret1 = addrman->Select(); + auto addr_ret1 = addrman->Select().first; BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333"); // Test: Does IP address deduplication work correctly. @@ -224,7 +224,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports) CService addr1_port = ResolveService("250.1.1.1", 8334); BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman.size(), 1U); - CAddrInfo addr_ret2 = addrman.Select(); + auto addr_ret2 = addrman.Select().first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333"); // Test: Add same IP but diff port to tried table, it doesn't get added. @@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports) addrman.Good(CAddress(addr1_port, NODE_NONE)); BOOST_CHECK_EQUAL(addrman.size(), 1U); bool newOnly = true; - CAddrInfo addr_ret3 = addrman.Select(newOnly); + auto addr_ret3 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); } @@ -249,16 +249,16 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK_EQUAL(addrman.size(), 1U); bool newOnly = true; - CAddrInfo addr_ret1 = addrman.Select(newOnly); + auto addr_ret1 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333"); // Test: move addr to tried, select from new expected nothing returned. addrman.Good(CAddress(addr1, NODE_NONE)); BOOST_CHECK_EQUAL(addrman.size(), 1U); - CAddrInfo addr_ret2 = addrman.Select(newOnly); + auto addr_ret2 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "[::]:0"); - CAddrInfo addr_ret3 = addrman.Select(); + auto addr_ret3 = addrman.Select().first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); BOOST_CHECK_EQUAL(addrman.size(), 1U); @@ -291,7 +291,7 @@ BOOST_AUTO_TEST_CASE(addrman_select) // Test: Select pulls from new and tried regardless of port number. std::set ports; for (int i = 0; i < 20; ++i) { - ports.insert(addrman.Select().GetPort()); + ports.insert(addrman.Select().first.GetPort()); } BOOST_CHECK_EQUAL(ports.size(), 3U); } @@ -869,7 +869,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) BOOST_CHECK(addrman.size() == 0); // Empty addrman should return blank addrman info. - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // Add twenty two addresses. CNetAddr source = ResolveIP("252.2.2.2"); @@ -880,7 +880,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) // No collisions yet. BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Ensure Good handles duplicates well. @@ -889,7 +889,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) addrman.Good(addr); BOOST_CHECK(addrman.size() == 22); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } } @@ -907,7 +907,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) // No collision yet. BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Collision between 36 and 19. @@ -916,11 +916,11 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) addrman.Good(addr36); BOOST_CHECK(addrman.size() == 36); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.19:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.19:0"); // 36 should be discarded and 19 not evicted. addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // Lets create two collisions. for (unsigned int i = 37; i < 59; i++) { @@ -929,7 +929,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) addrman.Good(addr); BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Cause a collision. @@ -938,16 +938,16 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) addrman.Good(addr59); BOOST_CHECK(addrman.size() == 59); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.10:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.10:0"); // Cause a second collision. BOOST_CHECK(!addrman.Add({CAddress(addr36, NODE_NONE)}, source)); addrman.Good(addr36); BOOST_CHECK(addrman.size() == 59); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() != "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() != "[::]:0"); addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } BOOST_AUTO_TEST_CASE(addrman_evictionworks) @@ -957,7 +957,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) BOOST_CHECK(addrman.size() == 0); // Empty addrman should return blank addrman info. - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // Add 35 addresses CNetAddr source = ResolveIP("252.2.2.2"); @@ -968,7 +968,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) // No collision yet. BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Collision between 36 and 19. @@ -977,7 +977,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) addrman.Good(addr); BOOST_CHECK_EQUAL(addrman.size(), 36); - CAddrInfo info = addrman.SelectTriedCollision(); + auto info = addrman.SelectTriedCollision().first; BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0"); // Ensure test of address fails, so that it is evicted. @@ -985,23 +985,23 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) // Should swap 36 for 19. addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // If 36 was swapped for 19, then this should cause no collisions. BOOST_CHECK(!addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(addr); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // If we insert 19 it should collide with 36 CService addr19 = ResolveService("250.1.1.19"); BOOST_CHECK(!addrman.Add({CAddress(addr19, NODE_NONE)}, source)); addrman.Good(addr19); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.36:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.36:0"); addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } BOOST_AUTO_TEST_CASE(load_addrman)