Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

Commit 7b67130

Browse files
halfalicioushalfalicious
committed
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 * 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
1 parent 71d2ea6 commit 7b67130

File tree

5 files changed

+66
-81
lines changed

5 files changed

+66
-81
lines changed

‎libp2p/Host.cpp‎

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -200,23 +200,14 @@ std::shared_ptr<Peer> Host::peer(NodeID const& _n) const
200200
{
201201
RecursiveGuard l(x_sessions);
202202
auto it = m_peers.find(_n);
203-
if (it == m_peers.end())
204-
{
205-
LOG(m_logger) << "Peer " << _n << " not found";
206-
return nullptr;
207-
}
208-
return it->second;
203+
return it != m_peers.end() ? it->second : nullptr;
209204
}
210205

211-
void Host::handshakeFailed(NodeID const& _n, HandshakeFailureReason _r)
206+
void Host::onHandshakeFailed(NodeID const& _n, HandshakeFailureReason _r)
212207
{
213208
std::shared_ptr<Peer> p = peer(_n);
214-
if (!p)
215-
{
216-
cerror << "Peer " << _n << " not found";
217-
return;
218-
}
219-
p->m_lastHandshakeFailure = _r;
209+
if (p)
210+
p->m_lastHandshakeFailure = _r;
220211
}
221212

222213
void Host::doneWorking()
@@ -294,7 +285,10 @@ void Host::startPeerSession(Public const& _id, RLP const& _hello,
294285
{
295286
auto itPeer = m_peers.find(_id);
296287
if (itPeer != m_peers.end())
288+
{
297289
peer = itPeer->second;
290+
peer->m_lastHandshakeFailure = NoFailure;
291+
}
298292
else
299293
{
300294
// peer doesn't exist, try to get port info from node table
@@ -307,7 +301,6 @@ void Host::startPeerSession(Public const& _id, RLP const& _hello,
307301
m_peers[_id] = peer;
308302
}
309303
}
310-
peer->m_lastHandshakeFailure = NoFailure;
311304
if (peer->isOffline())
312305
peer->m_lastConnected = chrono::system_clock::now();
313306
peer->endpoint.setAddress(_s->remoteEndpoint().address());
@@ -807,21 +800,26 @@ void Host::run(boost::system::error_code const& _ec)
807800
unsigned reqConn = 0;
808801
{
809802
RecursiveGuard l(x_sessions);
803+
auto p = m_peers.cbegin();
804+
while (p != m_peers.cend())
810805
{
811-
for (auto p = m_peers.cbegin(); p != m_peers.cend(); p++)
806+
bool peerRemoved = false;
807+
bool haveSession = havePeerSession(p->second->id);
808+
bool required = p->second->peerType == PeerType::Required;
809+
if (haveSession && required)
810+
reqConn++;
811+
else if (!haveSession)
812812
{
813-
bool haveSession = havePeerSession(p->second->id);
814-
bool required = p->second->peerType == PeerType::Required;
815-
if (haveSession && required)
816-
reqConn++;
817-
else if (!haveSession)
813+
if (p->second->isUseless())
818814
{
819-
if (p->second->uselessPeer())
820-
p = m_peers.erase(p);
821-
else if (p->second->shouldReconnect() && (!m_netConfig.pin || required))
822-
toConnect.push_back(p->second);
815+
peerRemoved = true;
816+
p = m_peers.erase(p);
823817
}
818+
else if (p->second->shouldReconnect() && (!m_netConfig.pin || required))
819+
toConnect.push_back(p->second);
824820
}
821+
if (!peerRemoved)
822+
p++;
825823
}
826824
}
827825

‎libp2p/Host.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,7 @@ class Host: public Worker
346346
std::shared_ptr<Peer> peer(NodeID const& _n) const;
347347

348348
/// Set a handshake failure reason for a peer
349-
void handshakeFailed(NodeID const& _n, HandshakeFailureReason _r);
349+
void onHandshakeFailed(NodeID const& _n, HandshakeFailureReason _r);
350350

351351
bytes m_restoreNetwork; ///< Set by constructor and used to set Host key and restore network peers & nodes.
352352

‎libp2p/Peer.cpp‎

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,19 @@ namespace dev
3030

3131
namespace p2p
3232
{
33+
namespace
34+
{
35+
unsigned defaultFallbackSeconds(unsigned _failedAttempts)
36+
{
37+
if (_failedAttempts < 5)
38+
return _failedAttempts ? _failedAttempts * 5 : 5;
39+
else if (_failedAttempts < 15)
40+
return 25 + (_failedAttempts - 5) * 10;
41+
else
42+
return 25 + 100 + (_failedAttempts - 15) * 20;
43+
}
44+
} // namespace
45+
3346
Peer::Peer(Peer const& _original)
3447
: Node(_original),
3548
m_lastConnected(_original.m_lastConnected),
@@ -45,12 +58,11 @@ Peer::Peer(Peer const& _original)
4558

4659
bool Peer::shouldReconnect() const
4760
{
48-
return id && endpoint &&
49-
fallbackSeconds() != numeric_limits<unsigned>::max() &&
61+
return id && endpoint && !isUseless() &&
5062
chrono::system_clock::now() > m_lastAttempted + chrono::seconds(fallbackSeconds());
5163
}
5264

53-
bool Peer::uselessPeer() const
65+
bool Peer::isUseless() const
5466
{
5567
if (peerType == PeerType::Required)
5668
return false;
@@ -66,55 +78,50 @@ bool Peer::uselessPeer() const
6678

6779
switch (m_lastDisconnect)
6880
{
81+
// Critical cases
6982
case BadProtocol:
7083
case UselessPeer:
7184
case IncompatibleProtocol:
7285
case UnexpectedIdentity:
73-
case UserReason:
86+
case DuplicatePeer:
87+
case NullIdentity:
7488
return true;
89+
// Potentially transient cases which can resolve quickly
90+
case PingTimeout:
91+
case TCPError:
92+
case TooManyPeers:
93+
return m_failedAttempts >= 10;
94+
// Potentially transient cases which can take longer to resolve
95+
case ClientQuit:
96+
case UserReason:
97+
return m_failedAttempts >= 25;
7598
default:
7699
break;
77100
}
78101
return false;
79102
}
80103

81-
82104
unsigned Peer::fallbackSeconds() const
83105
{
84106
constexpr unsigned oneYearInSeconds{60 * 60 * 24 * 360};
85107

86108
if (peerType == PeerType::Required)
87109
return 5;
88110

89-
switch (m_lastHandshakeFailure)
90-
{
91-
case FrameDecryptionFailure:
92-
case ProtocolError:
93-
return oneYearInSeconds;
94-
default:
95-
break;
96-
}
111+
if (isUseless())
112+
return oneYearInSeconds;
97113

98114
switch (m_lastDisconnect)
99115
{
100-
case BadProtocol:
101-
case UselessPeer:
102-
case IncompatibleProtocol:
103-
case UnexpectedIdentity:
104-
case UserReason:
105-
return oneYearInSeconds;
116+
case TCPError:
117+
case PingTimeout:
106118
case TooManyPeers:
107-
return 25 * (m_failedAttempts + 1);
108-
case ClientQuit:
109119
return 15 * (m_failedAttempts + 1);
110-
case NoDisconnect:
120+
case ClientQuit:
121+
case UserReason:
122+
return 25 * (m_failedAttempts + 1);
111123
default:
112-
if (m_failedAttempts < 5)
113-
return m_failedAttempts ? m_failedAttempts * 5 : 5;
114-
else if (m_failedAttempts < 15)
115-
return 25 + (m_failedAttempts - 5) * 10;
116-
else
117-
return 25 + 100 + (m_failedAttempts - 15) * 20;
124+
return defaultFallbackSeconds(m_failedAttempts);
118125
}
119126
}
120127

‎libp2p/Peer.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class Peer: public Node
7272

7373
/// A peer which should never be reconnected to - e.g. it's running on a different network, we
7474
/// don't have any common capabilities
75-
bool uselessPeer() const;
75+
bool isUseless() const;
7676

7777
/// Number of times connection has been attempted to peer.
7878
int failedAttempts() const { return m_failedAttempts; }
@@ -84,8 +84,8 @@ class Peer: public Node
8484
void noteSessionGood() { m_failedAttempts = 0; }
8585

8686
private:
87-
/// Returns number of seconds to wait until attempting connection, based on attempted connection history, or
88-
/// numeric_limits<unsigned>::max() if a connection should never be attempted.
87+
/// Returns number of seconds to wait until attempting connection, based on attempted connection
88+
/// history
8989
unsigned fallbackSeconds() const;
9090

9191
std::atomic<int> m_score{0}; ///< All time cumulative.

‎libp2p/RLPxHandshake.cpp‎

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ RLPXHandshake::RLPXHandshake(
3030
m_remote(_remote),
3131
m_originated(_remote),
3232
m_socket(_socket),
33-
m_idleTimer(m_socket->ref().get_executor())
33+
m_idleTimer(m_socket->ref().get_executor()),
34+
m_failureReason{NoFailure}
3435
{
3536
auto const prefixAttr =
3637
boost::log::attributes::constant<std::string>{connectionDirectionString()};
@@ -114,8 +115,6 @@ void RLPXHandshake::writeAckEIP8()
114115
auto self(shared_from_this());
115116
ba::async_write(m_socket->ref(), ba::buffer(m_ackCipher),
116117
[this, self](boost::system::error_code ec, std::size_t) {
117-
if (ec)
118-
m_failureReason = TcpError;
119118
transition(ec);
120119
});
121120
}
@@ -137,10 +136,7 @@ void RLPXHandshake::readAuth()
137136
ba::async_read(m_socket->ref(), ba::buffer(m_authCipher, c_authCipherSizeBytes),
138137
[this, self](boost::system::error_code ec, std::size_t) {
139138
if (ec)
140-
{
141-
m_failureReason = TcpError;
142139
transition(ec);
143-
}
144140
else if (decryptECIES(m_host->m_alias.secret(), bytesConstRef(&m_authCipher), m_auth))
145141
{
146142
LOG(m_logger) << "auth from";
@@ -168,10 +164,7 @@ void RLPXHandshake::readAuthEIP8()
168164
{
169165
bytesConstRef ct(&m_authCipher);
170166
if (ec)
171-
{
172-
m_failureReason = TcpError;
173167
transition(ec);
174-
}
175168
else if (decryptECIES(m_host->m_alias.secret(), ct.cropped(0, 2), ct.cropped(2), m_auth))
176169
{
177170
RLP rlp(m_auth, RLP::ThrowOnFail | RLP::FailIfTooSmall);
@@ -201,10 +194,7 @@ void RLPXHandshake::readAck()
201194
ba::async_read(m_socket->ref(), ba::buffer(m_ackCipher, c_ackCipherSizeBytes),
202195
[this, self](boost::system::error_code ec, std::size_t) {
203196
if (ec)
204-
{
205-
m_failureReason = TcpError;
206197
transition(ec);
207-
}
208198
else if (decryptECIES(m_host->m_alias.secret(), bytesConstRef(&m_ackCipher), m_ack))
209199
{
210200
LOG(m_logger) << "ack from";
@@ -230,10 +220,7 @@ void RLPXHandshake::readAckEIP8()
230220
{
231221
bytesConstRef ct(&m_ackCipher);
232222
if (ec)
233-
{
234-
m_failureReason = TcpError;
235223
transition(ec);
236-
}
237224
else if (decryptECIES(m_host->m_alias.secret(), ct.cropped(0, 2), ct.cropped(2), m_ack))
238225
{
239226
RLP rlp(m_ack, RLP::ThrowOnFail | RLP::FailIfTooSmall);
@@ -262,8 +249,7 @@ void RLPXHandshake::cancel()
262249

263250
void RLPXHandshake::error(boost::system::error_code _ech)
264251
{
265-
if (m_originated)
266-
m_host->handshakeFailed(m_remote, m_failureReason);
252+
m_host->onHandshakeFailed(m_remote, m_failureReason);
267253

268254
stringstream errorStream;
269255
errorStream << "Handshake failed";
@@ -286,6 +272,8 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
286272

287273
if (_ech || m_nextState == Error || m_cancel)
288274
{
275+
if (_ech)
276+
m_failureReason = TcpError;
289277
return error(_ech);
290278
}
291279

@@ -351,8 +339,6 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
351339
m_io->writeSingleFramePacket(&packet, m_handshakeOutBuffer);
352340
ba::async_write(m_socket->ref(), ba::buffer(m_handshakeOutBuffer),
353341
[this, self](boost::system::error_code ec, std::size_t) {
354-
if (ec)
355-
m_failureReason = TcpError;
356342
transition(ec);
357343
});
358344
}
@@ -369,10 +355,7 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
369355
boost::asio::buffer(m_handshakeInBuffer, handshakeSizeBytes),
370356
[this, self](boost::system::error_code ec, std::size_t) {
371357
if (ec)
372-
{
373-
m_failureReason = TcpError;
374358
transition(ec);
375-
}
376359
else
377360
{
378361
if (!m_io)
@@ -436,10 +419,7 @@ void RLPXHandshake::transition(boost::system::error_code _ech)
436419
m_idleTimer.cancel();
437420

438421
if (ec)
439-
{
440-
m_failureReason = TcpError;
441422
transition(ec);
442-
}
443423
else
444424
{
445425
if (!m_io)

0 commit comments

Comments
 (0)