From fff8f97917b7dc78d908011f5d2231bbb9cbb5b0 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 9 Mar 2019 07:08:51 +0100 Subject: [PATCH] Merge #15486: [addrman, net] Ensure tried collisions resolve, and allow feeler connections to existing outbound netgroups 20e6ea259b222b10f066f22695a5f56c52071f63 [addrman] Improve collision logging and address nits (Suhas Daftuar) f71fdda3bc2e7acd2a8b74e882364866b8b0f55b [addrman] Ensure collisions eventually get resolved (Suhas Daftuar) 4991e3c813c9848d3b3957ea3ad433f02fca9e81 [net] feeler connections can be made to outbound peers in same netgroup (Suhas Daftuar) 4d834018e368c3481a5421891395f64aa9002185 [addrman] Improve tried table collision logging (Suhas Daftuar) Pull request description: The restriction on outbound peers sharing the same network group is not intended to apply to feeler connections, so fix this. This fixes an issue where a tried table collision with an entry to a netgroup we already have an outbound connection to could cause feelers to stop working, because the tried collision buffer (`m_tried_collisions`) would never be drained. Also, ensure that all entries don't linger in `m_tried_collisions` by evicting an old entry if its collisions is unresolved after 40 minutes. Tree-SHA512: 553fe2b01b82cd7f0f62f90c6781e373455a45b254e3bec085b5e6b16690aa9f3938e8c50e7136f19dafa250ed4578a26227d944b76daf9ce4ef0c75802389b6 --- src/addrman.cpp | 17 +++++++++++++++-- src/addrman.h | 3 +++ src/net.cpp | 8 +++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index b1b53b3698..e227049106 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -261,7 +261,13 @@ void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime // Will moving this address into tried evict another entry? if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) { - if (fLogIPs) LogPrint(BCLog::ADDRMAN, "Collision inserting element into tried table, moving %s to m_tried_collisions=%d\n", addr.ToString(), m_tried_collisions.size()); + // Output the entry we'd be colliding with, for debugging purposes + auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]); + if (fLogIPs) { + LogPrint(BCLog::ADDRMAN, "Collision inserting element into tried table (%s), moving %s to m_tried_collisions=%d\n", + colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "", + addr.ToString(), m_tried_collisions.size()); + } if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) { m_tried_collisions.insert(nId); } @@ -604,12 +610,19 @@ void CAddrMan::ResolveCollisions_() // Give address at least 60 seconds to successfully connect if (GetAdjustedTime() - info_old.nLastTry > 60) { - LogPrint(BCLog::ADDRMAN, "Swapping %s for %s in tried table\n", info_new.ToString(), info_old.ToString()); + LogPrint(BCLog::ADDRMAN, "Replacing %s with %s in tried table\n", info_old.ToString(), info_new.ToString()); // Replaces an existing address already in the tried table with the new address Good_(info_new, false, GetAdjustedTime()); erase_collision = true; } + } else if (GetAdjustedTime() - info_new.nLastSuccess > ADDRMAN_TEST_WINDOW) { + // If the collision hasn't resolved in some reasonable amount of time, + // just evict the old entry -- we must not be able to + // connect to it for some reason. + LogPrint(BCLog::ADDRMAN, "Unable to test; replacing %s with %s in tried table anyway\n", info_old.ToString(), info_new.ToString()); + Good_(info_new, false, GetAdjustedTime()); + erase_collision = true; } } else { // Collision is not actually a collision anymore Good_(info_new, false, GetAdjustedTime()); diff --git a/src/addrman.h b/src/addrman.h index 3e938b00f8..3c725a0172 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -184,6 +184,9 @@ public: //! the maximum number of tried addr collisions to store #define ADDRMAN_SET_TRIED_COLLISION_SIZE 10 +//! the maximum time we'll spend trying to resolve a tried table collision, in seconds +static const int64_t ADDRMAN_TEST_WINDOW = 40*60; // 40 minutes + /** * Stochastical (IP) address manager */ diff --git a/src/net.cpp b/src/net.cpp index b1aaa32431..9dc9e55f5d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2464,6 +2464,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect) auto dmn = mnList.GetMNByService(addr); bool isMasternode = dmn != nullptr; + // Require outbound connections, other than feelers, to be to distinct network groups + if (!fFeeler && setConnected.count(addr.GetGroup(addrman.m_asmap))) { + break; + } + // if we selected an invalid address, restart if (!addr.IsValid() || setConnected.count(addr.GetGroup(addrman.m_asmap))) break; @@ -2474,8 +2479,9 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // if we selected a local address, restart (local addresses are allowed in regtest and devnet) bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort(); - if (!fAllowLocal && IsLocal(addrConnect)) + if (!fAllowLocal && IsLocal(addrConnect)) { break; + } // If we didn't find an appropriate destination after trying 100 addresses fetched from addrman, // stop this loop, and let the outer loop run again (which sleeps, adds seed nodes, recalculates