[ETCM-178] Disallow duplicated connections and connections to self #763

ntallar · 2020-10-28T16:36:30Z

Description

Disallowing duplicated connections
Disallowing connections to self

Proposed Solution

As a new port is used for every connection established with a peer the IP+port a node is not a reliable identifier. Handshaked peers are now identified by their enode. Pending outgoing peers could also be identified by it eventually (as mentioned on the implementation) but not incoming peers (at least without refactoring) as the node id from them is obtained after the RLPx handshake
Discovery should filter out self as one of the discovered peers
Refactored PeerManagerActor internal handling of peers for better readability

Scenarios addressed

Connection to self

Start up a node, obtain it's node id and stop it
Configure the node with it's own id as the bootstrap
Start up the node, which should eventually successfully connect to self. It will appear as 2 connections, as it's both an incoming and outgoing connection

2020-10-28 14:44:13,136 [i.i.e.blockchain.sync.PeersClient] - Handshaked peers status (number of peers: 2)...

Duplicated connections

Start up 2 nodes, obtain their node ids and stop them
Configure both nodes with the node id of the other as their bootstrap
Start up one of the nodes (node 1) and await for it to intend connecting with the other (will fail as it's offline)
Start up the other (node 2), it will connect and handshake with node 1
Eventually node 1 will re-attempt at succeed at connecting and handshaking with node 2, meaning that there will be a repeated connection between node 1 and node 2, as seen on the log:

2020-10-28 14:44:13,136 [i.i.e.blockchain.sync.PeersClient] - Handshaked peers status (number of peers: 2)...

Testing

Added unit tests to PeerManagerSpec
Reproducing the scenario of duplicated connections mentioned above should result in both nodes having a single handshaked peer
Note that the connection node 1 to node 2 will take a while to be closed, at it will do so only after re-attempting and succeeding the handshake
Reproducing the scenario of connection to self should result in the node not even adding itself as a discovered peer

Pending

Disallow connections to self

KonradStaniec · 2020-10-30T11:13:50Z

src/main/scala/io/iohk/ethereum/network/discovery/PeerDiscoveryManager.scala

-    nodesInfo.map { nodeInfo => nodeInfo.node.id -> nodeInfo }.toMap
+    val startingNodesInfo = knownNodesURIs.map(uri => DiscoveryNodeInfo.fromUri(uri)) ++ bootStrapNodesInfo
+    val startingNodesInfoWithoutSelf = startingNodesInfo.filterNot {
+      _.node.id == ByteString(nodeStatusHolder.get().nodeId)


shouldn't we filter self every time manager responds to GetDiscoveredNodesInfo message ?

This depends on what discovery guarantees, given we are in the process of integrating our new discovery implementation I didn't want to get in the middle

(for example on the other project it does guarantee that it won't yourself as a discovered peer)

thats a valid point. We can always fix it after integration with new discovery.

KonradStaniec · 2020-10-30T11:17:08Z

src/main/scala/io/iohk/ethereum/network/ConnectedPeers.scala

+      However, with checking the node id of only handshaked peers we prioritize handshaked peers over pending ones,
+      in the above mentioned case the repeated pending peer connection will eventually die out
+   */
+  def hasHandshakedWith(nodeId: ByteString): Boolean =


shouldn't we have lazy val with all handshaked peer id to avoid this traversing every time ?

KonradStaniec · 2020-10-30T11:18:23Z

src/main/scala/io/iohk/ethereum/network/ConnectedPeers.scala

+  private lazy val allPeers: Map[PeerId, Peer] = outgoingPendingPeers ++ handshakedPeers ++ incomingPendingPeers
+
+  def isConnectionHandled(remoteAddress: InetSocketAddress): Boolean =
+    allPeers.values.map(_.remoteAddress).toSet.contains(remoteAddress)


shouldn't we have lazy val with all connected peers adresses to avoid this traversing every time ?

KonradStaniec · 2020-10-30T11:26:17Z

src/main/scala/io/iohk/ethereum/network/PeerManagerActor.scala

  ): Unit = {
-    val connectionHandled = isConnectionHandled(remoteAddress, pendingPeers, peers)
-    val isPendingPeersNotMaxValue = pendingPeers.size < peerConfiguration.maxPendingPeers
+    val alreadyConnectedToPeer = connectedPeers.isConnectionHandled(remoteAddress)


I wonder, sholdn't we check this only based only on InetAddress (not InetSocketAddress) , as every incoming connection can have different ephemeral port, so the same peer can create several outgoing connections to us from different ports. I remember in other project, I did it as temporarly solution before migrating to indentifing peers here by their NodeIds.

It can have downside, that if there are two peers in the same subnet, we will allow only one of them, but if we will improve it later to identifing peers by node ids, then it can be good temporary solution.

so the same peer can create several outgoing connections to us from different ports

The only consequence from this is that those peers will be detected as repeated only after the handshake, right? Because at least as I was able to test out that might only happen initially, eventually those repeated connections will die our and no new ones will be started

If we had planned to continue improving on this I'd agree with your proposal, but as I'm not sure for how long we'll have to keep that temporary solution I prefer keeping it as is

sure, then maybe leave a comment about it here ?

…-178-remove-repeated-connections

KonradStaniec · 2020-11-06T13:47:16Z

src/main/scala/io/iohk/ethereum/network/discovery/PeerDiscoveryManager.scala

-    nodesInfo.map { nodeInfo => nodeInfo.node.id -> nodeInfo }.toMap
+    val startingNodesInfo = knownNodesURIs.map(uri => DiscoveryNodeInfo.fromUri(uri)) ++ bootStrapNodesInfo
+    val startingNodesInfoWithoutSelf = startingNodesInfo.filterNot {
+      _.node.id == ByteString(nodeStatusHolder.get().nodeId)


thats a valid point. We can always fix it after integration with new discovery.

KonradStaniec · 2020-11-06T13:49:37Z

src/main/scala/io/iohk/ethereum/network/PeerManagerActor.scala

  ): Unit = {
-    val connectionHandled = isConnectionHandled(remoteAddress, pendingPeers, peers)
-    val isPendingPeersNotMaxValue = pendingPeers.size < peerConfiguration.maxPendingPeers
+    val alreadyConnectedToPeer = connectedPeers.isConnectionHandled(remoteAddress)


sure, then maybe leave a comment about it here ?

mirkoAlic · 2020-11-09T14:17:55Z

src/test/scala/io/iohk/ethereum/jsonrpc/DebugServiceSpec.scala

@@ -80,7 +80,7 @@ class DebugServiceSpec
      bestBlockHash = peerStatus.bestHash
    )
    val peer1Probe = TestProbe()
-    val peer1 = Peer(new InetSocketAddress("127.0.0.1", 1), peer1Probe.ref, incomingConnection = false)
+    val peer1 = Peer(new InetSocketAddress("127.0.0.1", 1), peer1Probe.ref, false)


mirkoAlic · 2020-11-09T15:00:34Z

src/test/scala/io/iohk/ethereum/network/PeerManagerSpec.scala

@@ -231,12 +288,15 @@ class PeerManagerSpec extends AnyFlatSpec with Matchers with Eventually with Nor
      )
    )(system)

-    def startConnecting(): Unit = {
+    def start(): Unit = {


) - Disallow repeated connections and connections to self - Added further lazy vals to ConnectedPeers

ntallar changed the title ~~[ETCM-178] Identify handshaked peers by their node id~~ [ETCM-178] Disallow duplicated connections and connections to self Oct 28, 2020

ntallar force-pushed the etcm-178-remove-repeated-connections branch 2 times, most recently from d60cd85 to 9422464 Compare October 29, 2020 15:26

[ETCM-178] Disallow repeated connections and connections to self

ef84bec

ntallar force-pushed the etcm-178-remove-repeated-connections branch from 9422464 to ef84bec Compare October 29, 2020 15:45

ntallar requested review from aakoshh and KonradStaniec October 29, 2020 15:50

ntallar marked this pull request as ready for review October 29, 2020 15:50

KonradStaniec reviewed Oct 30, 2020

View reviewed changes

Nicolas Tallar added 2 commits November 4, 2020 13:22

[ETCM-178] Added further lazy vals to ConnectedPeers

2e1fc09

Merge branch 'develop' of github.com:input-output-hk/mantis into etcm…

e4c50d4

…-178-remove-repeated-connections

ntallar requested a review from KonradStaniec November 4, 2020 16:28

KonradStaniec approved these changes Nov 6, 2020

View reviewed changes

ntallar mentioned this pull request Nov 6, 2020

[Fix] Propagation improvements #784

Merged

ntallar added the high priority This PRs should be reviewed and merged ASAP label Nov 9, 2020

mirkoAlic reviewed Nov 9, 2020

View reviewed changes

Merge branch 'develop' into etcm-178-remove-repeated-connections

eb91bfe

mirkoAlic merged commit 1ea4cff into develop Nov 9, 2020

mirkoAlic deleted the etcm-178-remove-repeated-connections branch November 9, 2020 19:25

kapke pushed a commit that referenced this pull request Nov 16, 2020

[ETCM-178] Disallow duplicated connections and connections to self (#763

3aab3ae

) - Disallow repeated connections and connections to self - Added further lazy vals to ConnectedPeers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-178] Disallow duplicated connections and connections to self #763

[ETCM-178] Disallow duplicated connections and connections to self #763

ntallar commented Oct 28, 2020 •

edited

Loading

KonradStaniec Oct 30, 2020

ntallar Nov 4, 2020

KonradStaniec Nov 6, 2020

KonradStaniec Oct 30, 2020

KonradStaniec Oct 30, 2020

KonradStaniec Oct 30, 2020

ntallar Nov 4, 2020

KonradStaniec Nov 6, 2020

KonradStaniec Nov 6, 2020

KonradStaniec Nov 6, 2020

mirkoAlic Nov 9, 2020

mirkoAlic Nov 9, 2020

[ETCM-178] Disallow duplicated connections and connections to self #763

[ETCM-178] Disallow duplicated connections and connections to self #763

Conversation

ntallar commented Oct 28, 2020 • edited Loading

Description

Proposed Solution

Scenarios addressed

Connection to self

Duplicated connections

Testing

Pending

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntallar commented Oct 28, 2020 •

edited

Loading