Check for existing RLPX handshake when connecting to a node#5599
Check for existing RLPX handshake when connecting to a node#5599halfalicious merged 3 commits intomasterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5599 +/- ##
==========================================
+ Coverage 62.21% 62.22% +<.01%
==========================================
Files 347 347
Lines 29284 29296 +12
Branches 3309 3312 +3
==========================================
+ Hits 18219 18228 +9
- Misses 9876 9881 +5
+ Partials 1189 1187 -2 |
f2b0589 to
fce88db
Compare
fce88db to
6b8d16a
Compare
|
Another approach would be to remove the peer entry from aleth/libp2p/RLPxHandshake.cpp Lines 454 to 462 in 32f9c88 However, I think this would be a little messier since it would require adding 2 functions to Host - 1 which enables peer lookup by node ID and returns |
Host only checks in m_pendingPeerCons before attempting to open a new TCP connection to a node (in Host::connect) and start a new RLPX handshake - entries are removed from m_pendingPeerConns after the RLPX handshake has been started rather than when it is completed, which results in Host being able to initiate another TCP connection to the same node before the first RLPX handshake is complete. To fix this, Host::connect also checks the RLPX handshake list (m_connecting) before starting a new connection.
81a7583 to
16dcdfc
Compare
|
Hm, what about removing from Might still be messier, considering that we'd also need to somehow remove them when handshake failed... |
libp2p/Host.cpp
Outdated
| } | ||
| } | ||
|
|
||
| bool Host::isConnecting(NodeID const& _id) |
There was a problem hiding this comment.
Can it be const method? (make mutex mutable if needed)
There was a problem hiding this comment.
Maybe it should be more precisely called isHandshaking, because connection itself actually already exists during handshake
There was a problem hiding this comment.
Need to make mutex mutable, will do this and rename the method.
isHandshaking more accurately describes what the function is checking. Also make the function const (which requires making the x_connecting mutex mutable).
Fix #5600
Check for existing RLPX handshake in
Host::connectbefore attempting a new connection to a node - this is required becauseHost::connectonly checksm_pendingPeerConnsbefore starting a new connection, and entries are removed fromm_pendingPeerConnsafter a new RLPX handshake has started rather than after the handshake has completed.I've chosen to check for an existing RLPX handshake rather than garbage-collecting
m_pendingPeerConnsinHost::runsince the former only adds cost during a new connection while the latter would add cost on everyHost::runinvocation.