Garbage collect incompatible peers in Host::run()

CHANGELOG.md

@@ -11,6 +11,7 @@
 - Added: [#5591](https://github.com/ethereum/aleth/pull/5591) Network logging bugfixes and improvements and add p2pcap log channel.
 - Added: [#5588](https://github.com/ethereum/aleth/pull/5588) Testeth prints similar test suite name suggestions, when the name passed in `-t` argument is not found.
 - Added: [#5593](https://github.com/ethereum/aleth/pull/5593) Dynamically updating host ENR.
+- Added: [#5624](https://github.com/ethereum/aleth/pull/5624) Remove useless peers from peer list.
 - Changed: [#5532](https://github.com/ethereum/aleth/pull/5532) The leveldb is upgraded to 1.22. This is breaking change on Windows and the old databases are not compatible.
 - Changed: [#5559](https://github.com/ethereum/aleth/pull/5559) Update peer validation error messages.
 - Changed: [#5568](https://github.com/ethereum/aleth/pull/5568) Improve rlpx handshake log messages and create new rlpx log channel.

libp2p/Common.h

@@ -110,6 +110,17 @@ enum DisconnectReason
 /// @returns the string form of the given disconnection reason.
 std::string reasonOf(DisconnectReason _r);
 
+enum HandshakeFailureReason
+{
+    NoFailure = 0,
+    UnknownFailure,
+    Timeout,
+    TcpError,
+    FrameDecryptionFailure,
+    InternalError,
+    ProtocolError
+};
+
 using CapDesc = std::pair<std::string, unsigned>;
 using CapDescSet = std::set<CapDesc>;
 using CapDescs = std::vector<CapDesc>;

libp2p/Host.cpp

@@ -196,6 +196,29 @@ void Host::stopCapabilities()
     }
 }
 
+std::shared_ptr<Peer> Host::peer(NodeID const& _n) const
+{
+    RecursiveGuard l(x_sessions);
+    auto it = m_peers.find(_n);
+    if (it == m_peers.end())
+    {
+        LOG(m_logger) << "Peer " << _n << " not found";
+        return nullptr;
+    }
+    return it->second;
+}
+
+void Host::handshakeFailed(NodeID const& _n, HandshakeFailureReason _r)
+{
+    std::shared_ptr<Peer> p = peer(_n);
+    if (!p)
+    {
+        cerror << "Peer " << _n << " not found";
+        return;
+    }
+    p->m_lastHandshakeFailure = _r;
+}
+
 void Host::doneWorking()
 {
     // Return early if we have no capabilities since there's nothing to do. We've already stopped
@@ -284,6 +307,7 @@ void Host::startPeerSession(Public const& _id, RLP const& _hello,
             m_peers[_id] = peer;
         }
     }
+    peer->m_lastHandshakeFailure = NoFailure;
     if (peer->isOffline())
         peer->m_lastConnected = chrono::system_clock::now();
     peer->endpoint.setAddress(_s->remoteEndpoint().address());
@@ -783,15 +807,21 @@ void Host::run(boost::system::error_code const& _ec)
     unsigned reqConn = 0;
     {
         RecursiveGuard l(x_sessions);
-        for (auto const& p : m_peers)
         {
-            bool haveSession = havePeerSession(p.second->id);
-            bool required = p.second->peerType == PeerType::Required;
-            if (haveSession && required)
-                reqConn++;
-            else if (!haveSession && p.second->shouldReconnect() &&
-                        (!m_netConfig.pin || required))
-                toConnect.push_back(p.second);
+            for (auto p = m_peers.cbegin(); p != m_peers.cend(); p++)
+            {
+                bool haveSession = havePeerSession(p->second->id);
+                bool required = p->second->peerType == PeerType::Required;
+                if (haveSession && required)
+                    reqConn++;
+                else if (!haveSession)
+                {
+                    if (p->second->uselessPeer())
+                        p = m_peers.erase(p);
+                    else if (p->second->shouldReconnect() && (!m_netConfig.pin || required))
+                        toConnect.push_back(p->second);
+                }
+            }
         }
     }
 

libp2p/Host.h

@@ -343,6 +343,11 @@ class Host: public Worker
     /// Stop registered capabilities, typically done when the network is being shut down.
     void stopCapabilities();
 
+    std::shared_ptr<Peer> peer(NodeID const& _n) const;
+
+    /// Set a handshake failure reason for a peer
+    void handshakeFailed(NodeID const& _n, HandshakeFailureReason _r);
+
     bytes m_restoreNetwork;										///< Set by constructor and used to set Host key and restore network peers & nodes.
 
     std::atomic<bool> m_run{false};													///< Whether network is running.
@@ -408,7 +413,7 @@ class Host: public Worker
     /// logging to once every c_logActivePeersInterval seconds
     std::chrono::steady_clock::time_point m_lastPeerLogMessage;
 
-    Logger m_logger{createLogger(VerbosityDebug, "net")};
+    mutable Logger m_logger{createLogger(VerbosityDebug, "net")};
     Logger m_detailsLogger{createLogger(VerbosityTrace, "net")};
     Logger m_infoLogger{createLogger(VerbosityInfo, "net")};
 };

libp2p/Peer.cpp

@@ -30,12 +30,12 @@ namespace dev
 
 namespace p2p
 {
-
-Peer::Peer(Peer const& _original):
-    Node(_original),
+Peer::Peer(Peer const& _original)
+  : Node(_original),
     m_lastConnected(_original.m_lastConnected),
     m_lastAttempted(_original.m_lastAttempted),
     m_lastDisconnect(_original.m_lastDisconnect),
+    m_lastHandshakeFailure(_original.m_lastHandshakeFailure),
     m_session(_original.m_session)
 {
     m_score = _original.m_score.load();
@@ -45,18 +45,64 @@ Peer::Peer(Peer const& _original):
 
 bool Peer::shouldReconnect() const
 {
-    return id && endpoint && chrono::system_clock::now() > m_lastAttempted + chrono::seconds(fallbackSeconds());
+    return id && endpoint &&
+           fallbackSeconds() != numeric_limits<unsigned>::max() &&
+           chrono::system_clock::now() > m_lastAttempted + chrono::seconds(fallbackSeconds());
 }
-
+
+bool Peer::uselessPeer() const
+{
+    if (peerType == PeerType::Required)
+        return false;
+
+    switch (m_lastHandshakeFailure)
+    {
+    case FrameDecryptionFailure:
+    case ProtocolError:
+        return true;
+    default:
+        break;
+    }
+
+    switch (m_lastDisconnect)
+    {
+    case BadProtocol:
+    case UselessPeer:
+    case IncompatibleProtocol:
+    case UnexpectedIdentity:
+    case UserReason:
+        return true;
+    default:
+        break;
+    }
+    return false;
+}
+
+
 unsigned Peer::fallbackSeconds() const
 {
+    constexpr unsigned oneYearInSeconds{60 * 60 * 24 * 360};
+
     if (peerType == PeerType::Required)
         return 5;
+
+    switch (m_lastHandshakeFailure)
+    {
+        case FrameDecryptionFailure:
+        case ProtocolError:
+            return oneYearInSeconds;
+        default:
+            break;
+    }
+
     switch (m_lastDisconnect)
     {
     case BadProtocol:
-        return 30 * (m_failedAttempts + 1);
     case UselessPeer:
+    case IncompatibleProtocol:
+    case UnexpectedIdentity:
+    case UserReason:
+        return oneYearInSeconds;
     case TooManyPeers:
         return 25 * (m_failedAttempts + 1);
     case ClientQuit:

libp2p/Peer.h

@@ -70,6 +70,10 @@ class Peer: public Node
     /// Return true if connection attempt should be made to this peer or false if
     bool shouldReconnect() const;
 
+    /// A peer which should never be reconnected to - e.g. it's running on a different network, we
+    /// don't have any common capabilities
+    bool uselessPeer() const;
+
     /// Number of times connection has been attempted to peer.
     int failedAttempts() const { return m_failedAttempts; }
 
@@ -80,7 +84,8 @@ class Peer: public Node
     void noteSessionGood() { m_failedAttempts = 0; }
 
 private:
-    /// Returns number of seconds to wait until attempting connection, based on attempted connection history.
+    /// Returns number of seconds to wait until attempting connection, based on attempted connection history, or
+    /// numeric_limits<unsigned>::max() if a connection should never be attempted.
     unsigned fallbackSeconds() const;
 
     std::atomic<int> m_score{0};									///< All time cumulative.
@@ -92,6 +97,7 @@ class Peer: public Node
     std::chrono::system_clock::time_point m_lastAttempted;
     std::atomic<unsigned> m_failedAttempts{0};
     DisconnectReason m_lastDisconnect = NoDisconnect;	///< Reason for disconnect that happened last.
+    HandshakeFailureReason m_lastHandshakeFailure = NoFailure; ///< Reason for most recent handshake failure
 
     /// Used by isOffline() and (todo) for peer to emit session information.
     std::weak_ptr<Session> m_session;