From 4d834018e368c3481a5421891395f64aa9002185 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 26 Feb 2019 14:59:30 -0500 Subject: [PATCH 1/4] [addrman] Improve tried table collision logging --- src/addrman.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 06c342ba73..468618095c 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -239,7 +239,9 @@ 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)) { - 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]); + 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); } From 4991e3c813c9848d3b3957ea3ad433f02fca9e81 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 26 Feb 2019 15:04:48 -0500 Subject: [PATCH 2/4] [net] feeler connections can be made to outbound peers in same netgroup Fixes a bug where feelers could be stuck trying to resolve a collision in the tried table that is to an address in the same netgroup as an existing outbound peer. Thanks to Muoi Tran for the original bug report and detailed debug logs to track this down. --- src/net.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 87f1ef0577..3f87dc6357 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1765,9 +1765,15 @@ void CConnman::ThreadOpenConnections(const std::vector connect) addr = addrman.Select(fFeeler); } - // if we selected an invalid address, restart - if (!addr.IsValid() || setConnected.count(addr.GetGroup()) || IsLocal(addr)) + // Require outbound connections to be to distinct network groups + if (!fFeeler && setConnected.count(addr.GetGroup())) { break; + } + + // if we selected an invalid address, restart + if (!addr.IsValid() || IsLocal(addr)) { + 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 From f71fdda3bc2e7acd2a8b74e882364866b8b0f55b Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 26 Feb 2019 16:34:26 -0500 Subject: [PATCH 3/4] [addrman] Ensure collisions eventually get resolved After 40 minutes, time out a test-before-evict entry and just evict without testing. Otherwise, if we were unable to test an entry for some reason, we might break using feelers altogether. --- src/addrman.cpp | 7 +++++++ src/addrman.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/addrman.cpp b/src/addrman.cpp index 468618095c..45b686e1de 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -569,6 +569,13 @@ void CAddrMan::ResolveCollisions_() 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; swapping %s for %s in tried table anyway\n", info_new.ToString(), info_old.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 003bd059f8..e54184ce35 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -166,6 +166,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 */ From 20e6ea259b222b10f066f22695a5f56c52071f63 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Fri, 1 Mar 2019 16:15:50 -0500 Subject: [PATCH 4/4] [addrman] Improve collision logging and address nits --- src/addrman.cpp | 4 ++-- src/net.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 45b686e1de..8a5f78d1c5 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -563,7 +563,7 @@ 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()); @@ -573,7 +573,7 @@ void CAddrMan::ResolveCollisions_() // 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; swapping %s for %s in tried table anyway\n", info_new.ToString(), info_old.ToString()); + 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; } diff --git a/src/net.cpp b/src/net.cpp index 3f87dc6357..ccab4a1718 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1765,12 +1765,12 @@ void CConnman::ThreadOpenConnections(const std::vector connect) addr = addrman.Select(fFeeler); } - // Require outbound connections to be to distinct network groups + // Require outbound connections, other than feelers, to be to distinct network groups if (!fFeeler && setConnected.count(addr.GetGroup())) { break; } - // if we selected an invalid address, restart + // if we selected an invalid or local address, restart if (!addr.IsValid() || IsLocal(addr)) { break; }