Merge pull request #5624 from ethereum/gc-peers

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.

libethereum/BlockChainSync.cpp

@@ -197,7 +197,7 @@ void BlockChainSync::onPeerStatus(EthereumPeer const& _peer)
     if (!disconnectReason.empty())
     {
         LOG(m_logger) << "Peer " << _peer.id() << " not suitable for sync: " << disconnectReason;
-        m_host.capabilityHost().disconnect(_peer.id(), p2p::UserReason);
+        m_host.capabilityHost().disconnect(_peer.id(), p2p::UselessPeer);
         return;
     }
 

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,13 @@ void Host::stopCapabilities()
     }
 }
 
+std::shared_ptr<Peer> Host::peer(NodeID const& _n) const
+{
+    RecursiveGuard l(x_sessions);
+    auto it = m_peers.find(_n);
+    return it != m_peers.end() ? it->second : nullptr;
+}
+
 void Host::doneWorking()
 {
     // Return early if we have no capabilities since there's nothing to do. We've already stopped
@@ -271,7 +278,10 @@ void Host::startPeerSession(Public const& _id, RLP const& _hello,
     {
         auto itPeer = m_peers.find(_id);
         if (itPeer != m_peers.end())
+        {
             peer = itPeer->second;
+            peer->m_lastHandshakeFailure = NoFailure;
+        }
         else
         {
             // peer doesn't exist, try to get port info from node table
@@ -386,6 +396,13 @@ void Host::startPeerSession(Public const& _id, RLP const& _hello,
                   << _s->remoteEndpoint();
 }
 
+void Host::onHandshakeFailed(NodeID const& _n, HandshakeFailureReason _r)
+{
+    std::shared_ptr<Peer> p = peer(_n);
+    if (p)
+        p->m_lastHandshakeFailure = _r;
+}
+
 void Host::onNodeTableEvent(NodeID const& _n, NodeTableEventType const& _e)
 {
     if (_e == NodeEntryAdded)
@@ -783,15 +800,26 @@ void Host::run(boost::system::error_code const& _ec)
     unsigned reqConn = 0;
     {
         RecursiveGuard l(x_sessions);
-        for (auto const& p : m_peers)
+        auto p = m_peers.cbegin();
+        while (p != m_peers.cend())
         {
-            bool haveSession = havePeerSession(p.second->id);
-            bool required = p.second->peerType == PeerType::Required;
+            bool peerRemoved = false;
+            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);
+            else if (!haveSession)
+            {
+                if (p->second->isUseless())
+                {
+                    peerRemoved = true;
+                    p = m_peers.erase(p);
+                }
+                else if (p->second->shouldReconnect() && (!m_netConfig.pin || required))
+                    toConnect.push_back(p->second);
+            }
+            if (!peerRemoved)
+                p++;
         }
     }
 

libp2p/Host.h

@@ -214,6 +214,9 @@ class Host: public Worker
         return m_sessions.count(_id) ? m_sessions[_id].lock() : std::shared_ptr<SessionFace>();
     }
 
+    /// Set a handshake failure reason for a peer
+    void onHandshakeFailed(NodeID const& _n, HandshakeFailureReason _r);
+
     /// Get our current node ID.
     NodeID id() const { return m_alias.pub(); }
 
@@ -343,6 +346,8 @@ 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;
+
     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,25 @@ namespace dev
 
 namespace p2p
 {
+namespace
+{
+unsigned defaultFallbackSeconds(unsigned _failedAttempts)
+{
+    if (_failedAttempts < 5)
+        return _failedAttempts ? _failedAttempts * 5 : 5;
+    else if (_failedAttempts < 15)
+        return 25 + (_failedAttempts - 5) * 10;
+    else
+        return 25 + 100 + (_failedAttempts - 15) * 20;
+}
+}  // namespace
 
-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,30 +58,70 @@ Peer::Peer(Peer const& _original):
 
 bool Peer::shouldReconnect() const
 {
-    return id && endpoint && chrono::system_clock::now() > m_lastAttempted + chrono::seconds(fallbackSeconds());
+    return id && endpoint && !isUseless() &&
+           chrono::system_clock::now() > m_lastAttempted + chrono::seconds(fallbackSeconds());
 }
-    
-unsigned Peer::fallbackSeconds() const
+
+bool Peer::isUseless() const
 {
     if (peerType == PeerType::Required)
-        return 5;
+        return false;
+
+    switch (m_lastHandshakeFailure)
+    {
+    case FrameDecryptionFailure:
+    case ProtocolError:
+        return true;
+    default:
+        break;
+    }
+
     switch (m_lastDisconnect)
     {
+    // Critical cases
     case BadProtocol:
-        return 30 * (m_failedAttempts + 1);
     case UselessPeer:
+    case IncompatibleProtocol:
+    case UnexpectedIdentity:
+    case DuplicatePeer:
+    case NullIdentity:
+        return true;
+    // Potentially transient cases which can resolve quickly
+    case PingTimeout:
+    case TCPError:
     case TooManyPeers:
-        return 25 * (m_failedAttempts + 1);
+        return m_failedAttempts >= 10;
+    // Potentially transient cases which can take longer to resolve
     case ClientQuit:
+    case UserReason:
+        return m_failedAttempts >= 25;
+    default:
+        break;
+    }
+    return false;
+}
+
+unsigned Peer::fallbackSeconds() const
+{
+    constexpr unsigned oneYearInSeconds{60 * 60 * 24 * 360};
+
+    if (peerType == PeerType::Required)
+        return 5;
+
+    if (isUseless())
+        return oneYearInSeconds;
+
+    switch (m_lastDisconnect)
+    {
+    case TCPError:
+    case PingTimeout:
+    case TooManyPeers:
         return 15 * (m_failedAttempts + 1);
-    case NoDisconnect:
+    case ClientQuit:
+    case UserReason:
+        return 25 * (m_failedAttempts + 1);
     default:
-        if (m_failedAttempts < 5)
-            return m_failedAttempts ? m_failedAttempts * 5 : 5;
-        else if (m_failedAttempts < 15)
-            return 25 + (m_failedAttempts - 5) * 10;
-        else
-            return 25 + 100 + (m_failedAttempts - 15) * 20;
+        return defaultFallbackSeconds(m_failedAttempts);
     }
 }
 

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 isUseless() 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
     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;