[addrman, net] Ensure tried collisions resolve, and allow feeler connections to existing outbound netgroups

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);
         }
@@ -567,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());

src/addrman.h

@@ -166,6 +166,9 @@ class CAddrInfo : public CAddress
 //! 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
  */

src/net.cpp

@@ -1765,9 +1765,15 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 addr = addrman.Select(fFeeler);
             }
 
+            // 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() || setConnected.count(addr.GetGroup()) || IsLocal(addr))
+            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