Address PR feedback

Various minor changes in RLPxHandshake, Host, and Peer classes:
* Initialize RLPxHandshake::m_failureReason in ctor
* Reduce redundant code in RLPxHandshake which sets the last failure reason when TCP errors occur
* Rename Host::handshakeFailed
* Fix m_peers iteration bug in Host
* Make Host::onHandshakeFailed public (we want to avoid using friend classes where possible)
* Rename Peers::uselessPeer
* Update deprecated comment in Peer
* Take # of failed connection attempts into account in Peer function which determines if instance is useless or not
Move fallback seconds computation for default case to anonymous namespace function

Author: halfalicious

libp2p/Host.cpp

@@ -200,23 +200,7 @@ 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;
+    return it != m_peers.end() ? it->second : nullptr;
 }
 
 void Host::doneWorking()
@@ -294,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
@@ -307,7 +294,6 @@ 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());
@@ -410,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)
@@ -807,21 +800,26 @@ void Host::run(boost::system::error_code const& _ec)
     unsigned reqConn = 0;
     {
         RecursiveGuard l(x_sessions);
+        auto p = m_peers.cbegin();
+        while (p != m_peers.cend())
         {
-            for (auto p = m_peers.cbegin(); p != m_peers.cend(); p++)
+            bool peerRemoved = false;
+            bool haveSession = havePeerSession(p->second->id);
+            bool required = p->second->peerType == PeerType::Required;
+            if (haveSession && required)
+                reqConn++;
+            else if (!haveSession)
             {
-                bool haveSession = havePeerSession(p->second->id);
-                bool required = p->second->peerType == PeerType::Required;
-                if (haveSession && required)
-                    reqConn++;
-                else if (!haveSession)
+                if (p->second->isUseless())
                 {
-                    if (p->second->uselessPeer())
-                        p = m_peers.erase(p);
-                    else if (p->second->shouldReconnect() && (!m_netConfig.pin || required))
-                        toConnect.push_back(p->second);
+                    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(); }
 
@@ -345,9 +348,6 @@ class Host: public Worker
 
     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.

libp2p/Peer.cpp

@@ -30,6 +30,19 @@ 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),
     m_lastConnected(_original.m_lastConnected),
@@ -45,12 +58,11 @@ Peer::Peer(Peer const& _original)
 
 bool Peer::shouldReconnect() const
 {
-    return id && endpoint &&
-           fallbackSeconds() != numeric_limits<unsigned>::max() &&
+    return id && endpoint && !isUseless() &&
            chrono::system_clock::now() > m_lastAttempted + chrono::seconds(fallbackSeconds());
 }
 
-bool Peer::uselessPeer() const
+bool Peer::isUseless() const
 {
     if (peerType == PeerType::Required)
         return false;
@@ -66,55 +78,50 @@ bool Peer::uselessPeer() const
 
     switch (m_lastDisconnect)
     {
+    // Critical cases
     case BadProtocol:
     case UselessPeer:
     case IncompatibleProtocol:
     case UnexpectedIdentity:
-    case UserReason:
+    case DuplicatePeer:
+    case NullIdentity:
         return true;
+    // Potentially transient cases which can resolve quickly
+    case PingTimeout:
+    case TCPError:
+    case TooManyPeers:
+        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;
 
-    switch (m_lastHandshakeFailure)
-    {
-        case FrameDecryptionFailure:
-        case ProtocolError:
-            return oneYearInSeconds;
-        default:
-            break;
-    }
+    if (isUseless())
+        return oneYearInSeconds;
 
     switch (m_lastDisconnect)
     {
-    case BadProtocol:
-    case UselessPeer:
-    case IncompatibleProtocol:
-    case UnexpectedIdentity:
-    case UserReason:
-        return oneYearInSeconds;
+    case TCPError:
+    case PingTimeout:
     case TooManyPeers:
-        return 25 * (m_failedAttempts + 1);
-    case ClientQuit:
         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

@@ -72,7 +72,7 @@ class Peer: public Node
 
     /// 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;
+    bool isUseless() const;
 
     /// Number of times connection has been attempted to peer.
     int failedAttempts() const { return m_failedAttempts; }
@@ -84,8 +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, or
-    /// numeric_limits<unsigned>::max() if a connection should never be attempted.
+    /// 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.

libp2p/RLPxHandshake.cpp

@@ -30,7 +30,8 @@ RLPXHandshake::RLPXHandshake(
     m_remote(_remote),
     m_originated(_remote),
     m_socket(_socket),
-    m_idleTimer(m_socket->ref().get_executor())
+    m_idleTimer(m_socket->ref().get_executor()),
+    m_failureReason{NoFailure}
 {
     auto const prefixAttr =
         boost::log::attributes::constant<std::string>{connectionDirectionString()};
@@ -114,8 +115,6 @@ void RLPXHandshake::writeAckEIP8()
     auto self(shared_from_this());
     ba::async_write(m_socket->ref(), ba::buffer(m_ackCipher),
         [this, self](boost::system::error_code ec, std::size_t) {
-            if (ec)
-                m_failureReason = TcpError;
             transition(ec);
         });
 }
@@ -137,10 +136,7 @@ void RLPXHandshake::readAuth()
     ba::async_read(m_socket->ref(), ba::buffer(m_authCipher, c_authCipherSizeBytes),
         [this, self](boost::system::error_code ec, std::size_t) {
             if (ec)
-            {
-                m_failureReason = TcpError;
                 transition(ec);
-            }
             else if (decryptECIES(m_host->m_alias.secret(), bytesConstRef(&m_authCipher), m_auth))
             {
                 LOG(m_logger) << "auth from";
@@ -168,10 +164,7 @@ void RLPXHandshake::readAuthEIP8()
     {
         bytesConstRef ct(&m_authCipher);
         if (ec)
-        {
-            m_failureReason = TcpError;
             transition(ec);
-        }
         else if (decryptECIES(m_host->m_alias.secret(), ct.cropped(0, 2), ct.cropped(2), m_auth))
         {
             RLP rlp(m_auth, RLP::ThrowOnFail | RLP::FailIfTooSmall);
@@ -201,10 +194,7 @@ void RLPXHandshake::readAck()
     ba::async_read(m_socket->ref(), ba::buffer(m_ackCipher, c_ackCipherSizeBytes),
         [this, self](boost::system::error_code ec, std::size_t) {
             if (ec)
-            {
-                m_failureReason = TcpError;
                 transition(ec);
-            }
             else if (decryptECIES(m_host->m_alias.secret(), bytesConstRef(&m_ackCipher), m_ack))
             {
                 LOG(m_logger) << "ack from";
@@ -230,10 +220,7 @@ void RLPXHandshake::readAckEIP8()
     {
         bytesConstRef ct(&m_ackCipher);
         if (ec)
-        {
-            m_failureReason = TcpError;
             transition(ec);
-        }
         else if (decryptECIES(m_host->m_alias.secret(), ct.cropped(0, 2), ct.cropped(2), m_ack))
         {
             RLP rlp(m_ack, RLP::ThrowOnFail | RLP::FailIfTooSmall);
@@ -262,8 +249,7 @@ void RLPXHandshake::cancel()
 
 void RLPXHandshake::error(boost::system::error_code _ech)
 {
-    if (m_originated)
-        m_host->handshakeFailed(m_remote, m_failureReason);
+    m_host->onHandshakeFailed(m_remote, m_failureReason);
 
     stringstream errorStream;
     errorStream << "Handshake failed";
@@ -286,6 +272,8 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
 
     if (_ech || m_nextState == Error || m_cancel)
     {
+        if (_ech)
+            m_failureReason = TcpError;
         return error(_ech);
     }
 
@@ -351,8 +339,6 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
         m_io->writeSingleFramePacket(&packet, m_handshakeOutBuffer);
         ba::async_write(m_socket->ref(), ba::buffer(m_handshakeOutBuffer),
             [this, self](boost::system::error_code ec, std::size_t) {
-                if (ec)
-                    m_failureReason = TcpError;
                 transition(ec);
             });
     }
@@ -369,10 +355,7 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
             boost::asio::buffer(m_handshakeInBuffer, handshakeSizeBytes),
             [this, self](boost::system::error_code ec, std::size_t) {
                 if (ec)
-                {
-                    m_failureReason = TcpError;
                     transition(ec);
-                }
                 else
                 {
                     if (!m_io)
@@ -436,10 +419,7 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
                             m_idleTimer.cancel();
 
                             if (ec)
-                            {
-                                m_failureReason = TcpError;
                                 transition(ec);
-                            }
                             else
                             {
                                 if (!m_io)