From 56fabe31e231686d46b48b45d240f37a82296569 Mon Sep 17 00:00:00 2001
From: zzz <zzz@i2pmail.org>
Date: Wed, 3 Feb 2021 08:40:46 -0500
Subject: [PATCH] Tunnels: Peer selection tweaks

Remove blanket unreachable test for inbound tunnels, replace with reachable test for IBGW only
Retain inbound reachable test for hidden routers
Retain inbound reachable test for first hour of uptime
Add outbound reachable test for first hour of uptime
Add locally-unreachable test for outbound tunnels (was inbound only)
Minor cleanups
Log tweaks
---
 .../i2p/router/tunnel/pool/BuildExecutor.java |  4 +-
 .../tunnel/pool/TunnelPeerSelector.java       | 72 ++++++++++---------
 .../i2p/router/tunnel/pool/TunnelPool.java    |  8 +--
 3 files changed, 45 insertions(+), 39 deletions(-)

diff --git a/router/java/src/net/i2p/router/tunnel/pool/BuildExecutor.java b/router/java/src/net/i2p/router/tunnel/pool/BuildExecutor.java
index d5f43c6b55..54c373e471 100644
--- a/router/java/src/net/i2p/router/tunnel/pool/BuildExecutor.java
+++ b/router/java/src/net/i2p/router/tunnel/pool/BuildExecutor.java
@@ -636,8 +636,8 @@ class BuildExecutor implements Runnable {
             long requestedOn = rv.getExpiration() - 10*60*1000;
             long rtt = _context.clock().now() - requestedOn;
             _context.statManager().addRateData("tunnel.buildReplySlow", rtt, 0);
-            if (_log.shouldLog(Log.WARN))
-                _log.warn("Got reply late (rtt = " + rtt + ") for: " + rv);
+            if (_log.shouldInfo())
+                _log.info("Got reply late (rtt = " + rtt + ") for: " + rv);
         }
         return rv;
     }
diff --git a/router/java/src/net/i2p/router/tunnel/pool/TunnelPeerSelector.java b/router/java/src/net/i2p/router/tunnel/pool/TunnelPeerSelector.java
index fa7105e55a..6b590ea5fa 100644
--- a/router/java/src/net/i2p/router/tunnel/pool/TunnelPeerSelector.java
+++ b/router/java/src/net/i2p/router/tunnel/pool/TunnelPeerSelector.java
@@ -28,6 +28,7 @@ import net.i2p.router.networkdb.kademlia.FloodfillNetworkDatabaseFacade;
 import net.i2p.router.transport.TransportUtil;
 import net.i2p.router.util.HashDistance;
 import net.i2p.util.Log;
+import net.i2p.util.SystemVersion;
 import net.i2p.util.VersionComparator;
 
 /**
@@ -37,6 +38,8 @@ import net.i2p.util.VersionComparator;
  */
 public abstract class TunnelPeerSelector extends ConnectChecker {
 
+    private static final String DEFAULT_EXCLUDE_CAPS = String.valueOf(Router.CAPABILITY_BW12);
+
     protected TunnelPeerSelector(RouterContext context) {
         super(context);
     }
@@ -210,26 +213,23 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
         // We could just try and exclude them as the inbound gateway but that's harder
         // (and even worse for anonymity?).
         //
-        // Defaults changed to true for inbound only in filterUnreachable below.
 
         Set<Hash> peers = new HashSet<Hash>(8);
         peers.addAll(ctx.profileOrganizer().selectPeersRecentlyRejecting());
         peers.addAll(ctx.tunnelManager().selectPeersInTooManyTunnels());
-        // if (false && filterUnreachable(ctx, isInbound, isExploratory)) {
         if (filterUnreachable(isInbound, isExploratory)) {
-            // NOTE: filterUnreachable returns true for inbound, false for outbound
             // This is the only use for getPeersByCapability? And the whole set of datastructures in PeerManager?
             Collection<Hash> caps = ctx.peerManager().getPeersByCapability(Router.CAPABILITY_UNREACHABLE);
             if (caps != null)
                 peers.addAll(caps);
-            caps = ctx.profileOrganizer().selectPeersLocallyUnreachable();
-            if (caps != null)
-                peers.addAll(caps);
         }
+        Collection<Hash> local = ctx.profileOrganizer().selectPeersLocallyUnreachable();
+        if (local != null)
+            peers.addAll(local);
         if (filterSlow(isInbound, isExploratory)) {
             // NOTE: filterSlow always returns true
-            char excl[] = getExcludeCaps(ctx);
-            if (excl != null) {
+            String excl = getExcludeCaps(ctx);
+
                 FloodfillNetworkDatabaseFacade fac = (FloodfillNetworkDatabaseFacade)ctx.netDb();
                 List<RouterInfo> known = fac.getKnownRouterData();
                 if (known != null) {
@@ -334,7 +334,7 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
                     peers.addAll(matches);
                 }
                  */
-            }
+
         }
         return peers;
     }
@@ -367,7 +367,7 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
 
     /**
      *  Should we allow as IBGW?
-     *  This just checks for IPv4 support.
+     *  This just checks for the "R" capability and IPv4 support.
      *  Will return false for hidden or IPv6-only.
      *  This is intended for tunnel candidates, where we already have
      *  the RI. Will not force RI lookups.
@@ -379,6 +379,8 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
         RouterInfo ri = ctx.netDb().lookupRouterInfoLocally(h);
         if (ri == null)
             return true;
+        if (ri.getCapabilities().indexOf(Router.CAPABILITY_REACHABLE) < 0)
+            return false;
         return canConnect(ANY_V4, ri);
     }
     
@@ -452,25 +454,21 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
         return shouldExclude(peer, getExcludeCaps(ctx));
     }
     
-    private static char[] getExcludeCaps(RouterContext ctx) {
-        String excludeCaps = ctx.getProperty("router.excludePeerCaps", 
-                                             String.valueOf(Router.CAPABILITY_BW12));
-        if (excludeCaps != null) {
-            char excl[] = excludeCaps.toCharArray();
-            return excl;
-        } else {
-            return null;
-        }
+    /**
+     *  @return non-null, possibly empty
+     */
+    private static String getExcludeCaps(RouterContext ctx) {
+        return ctx.getProperty("router.excludePeerCaps", DEFAULT_EXCLUDE_CAPS);
     }
     
     /** NTCP2 */
     private static final String MIN_VERSION = "0.9.36";
 
     /** warning, this is also called by ProfileOrganizer.isSelectable() */
-    private static boolean shouldExclude(RouterInfo peer, char excl[]) {
+    private static boolean shouldExclude(RouterInfo peer, String excl) {
         String cap = peer.getCapabilities();
-        for (int j = 0; j < excl.length; j++) {
-            if (cap.indexOf(excl[j]) >= 0) {
+        for (int j = 0; j < excl.length(); j++) {
+            if (cap.indexOf(excl.charAt(j)) >= 0) {
                 return true;
             }
         }
@@ -562,24 +560,32 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
     private static final boolean DEFAULT_OUTBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE = false;
     private static final boolean DEFAULT_OUTBOUND_CLIENT_EXCLUDE_UNREACHABLE = false;
     // see comments at getExclude() above
-    private static final boolean DEFAULT_INBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE = true;
-    private static final boolean DEFAULT_INBOUND_CLIENT_EXCLUDE_UNREACHABLE = true;
+    private static final boolean DEFAULT_INBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE = false;
+    private static final boolean DEFAULT_INBOUND_CLIENT_EXCLUDE_UNREACHABLE = false;
     
     /**
-     * do we want to skip peers who haven't been up for long?
-     * @return true for inbound, false for outbound, unless configured otherwise
+     * do we want to skip unreachable peers?
+     * @return true if yes
      */
-    protected boolean filterUnreachable(boolean isInbound, boolean isExploratory) {
+    private boolean filterUnreachable(boolean isInbound, boolean isExploratory) {
+        if (SystemVersion.isSlow() || ctx.router().getUptime() < 65*60*1000)
+            return true;
         if (isExploratory) {
-            if (isInbound)
+            if (isInbound) {
+                if (ctx.router().isHidden())
+                    return true;
                 return ctx.getProperty(PROP_INBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE, DEFAULT_INBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE);
-            else
+            } else {
                 return ctx.getProperty(PROP_OUTBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE, DEFAULT_OUTBOUND_EXPLORATORY_EXCLUDE_UNREACHABLE);
+            }
         } else {
-            if (isInbound)
+            if (isInbound) {
+                if (ctx.router().isHidden())
+                    return true;
                 return ctx.getProperty(PROP_INBOUND_CLIENT_EXCLUDE_UNREACHABLE, DEFAULT_INBOUND_CLIENT_EXCLUDE_UNREACHABLE);
-            else 
+            } else { 
                 return ctx.getProperty(PROP_OUTBOUND_CLIENT_EXCLUDE_UNREACHABLE, DEFAULT_OUTBOUND_CLIENT_EXCLUDE_UNREACHABLE);
+            }
         }
     }
 
@@ -682,7 +688,7 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
      *  Connectivity check.
      *  Check that each hop can connect to the next, including us.
      *  Check that the OBEP is not IPv6-only, and the IBGW is
-     *  not hidden or IPv6-only.
+     *  reachable and not hidden or IPv6-only.
      *  Tells the profile manager to blame the hop, and returns false on failure.
      *
      *  @param tunnel ENDPOINT FIRST, GATEWAY LAST!!!!, length 2 or greater
@@ -696,7 +702,7 @@ public abstract class TunnelPeerSelector extends ConnectChecker {
             Hash h = tunnel.get(tunnel.size() - 1);
             if (!allowAsIBGW(h)) {
                 if (log.shouldWarn())
-                    log.warn("Picked IPv6-only or hidden peer for IBGW: " + h);
+                    log.warn("Picked IPv6-only or unreachable peer for IBGW: " + h);
                 // treat as a timeout in the profile
                 // tunnelRejected() would set the last heard from time
                 ctx.profileManager().tunnelTimedOut(h);
diff --git a/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java b/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java
index 47d1f3f00e..d3edfe311c 100644
--- a/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java
+++ b/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java
@@ -735,8 +735,8 @@ public class TunnelPool {
 
         int wanted = Math.min(_settings.getQuantity(), LeaseSet.MAX_LEASES);
         if (_tunnels.size() < wanted) {
-            if (_log.shouldLog(Log.WARN))
-                _log.warn(toString() + ": Not enough tunnels (" + _tunnels.size() + ", wanted " + wanted + ")");
+            if (_log.shouldInfo())
+                _log.info(toString() + ": Not enough tunnels (" + _tunnels.size() + ", wanted " + wanted + ")");
             // see comment below
             if (_tunnels.isEmpty())
                 return null;
@@ -797,8 +797,8 @@ public class TunnelPool {
         // So we will generate a succession of leases at startup. That's OK.
         // Do we want a config option for this, or are there times when we shouldn't do this?
         if (leases.size() < wanted) {
-            if (_log.shouldLog(Log.WARN))
-                _log.warn(toString() + ": Not enough leases (" + leases.size() + ", wanted " + wanted + ")");
+            if (_log.shouldInfo())
+                _log.info(toString() + ": Not enough leases (" + leases.size() + ", wanted " + wanted + ")");
             if (leases.isEmpty())
                 return null;
         }
-- 
GitLab