From ac20830113f12c746549c748fad37ed2a7fb20c7 Mon Sep 17 00:00:00 2001 From: cheatfate Date: Mon, 27 Jul 2020 01:25:12 +0300 Subject: [PATCH 1/3] Limit inbound connections to `maxIncomingPeers` value. Fix #1362. Fix variable names and 80 chars per line. Removal of unnecessary sleepAsync. --- beacon_chain/conf.nim | 9 +++++++-- beacon_chain/eth2_network.nim | 29 ++++++++++++++++------------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/beacon_chain/conf.nim b/beacon_chain/conf.nim index 46ca733219..a4c364f6f1 100644 --- a/beacon_chain/conf.nim +++ b/beacon_chain/conf.nim @@ -106,10 +106,15 @@ type name: "udp-port" }: Port maxPeers* {. - defaultValue: 79 # The Wall gets released - desc: "The maximum number of peers to connect to" + defaultValue: 100 # The Wall gets released + desc: "The maximum number of connected peers" name: "max-peers" }: int + maxIncomingPeers* {. + defaultValue: 21 + desc: "The maximum number of inbound peer's connections" + name: "max-incoming-peers"}: int + nat* {. desc: "Specify method to use for determining public address. " & "Must be one of: any, none, upnp, pmp, extip:" diff --git a/beacon_chain/eth2_network.nim b/beacon_chain/eth2_network.nim index 06e7da539f..ed9de67c2b 100644 --- a/beacon_chain/eth2_network.nim +++ b/beacon_chain/eth2_network.nim @@ -776,22 +776,24 @@ proc connectWorker(network: Eth2Node) {.async.} = remotePeerInfo = await network.connQueue.popFirst() peerPoolHasRemotePeer = network.peerPool.hasPeer(remotePeerInfo.peerId) seenTableHasRemotePeer = network.isSeen(remotePeerInfo) - remotePeerAlreadyConnected = network.connTable.hasKey(remotePeerInfo.peerId) + remotePeerConnecting = network.connTable.hasKey(remotePeerInfo.peerId) - if not(peerPoolHasRemotePeer) and not(seenTableHasRemotePeer) and not(remotePeerAlreadyConnected): + if not(peerPoolHasRemotePeer) and + not(seenTableHasRemotePeer) and not(remotePeerConnecting): network.connTable[remotePeerInfo.peerId] = remotePeerInfo try: # We trying to connect to peers which are not in PeerPool, SeenTable and # ConnTable. var fut = network.dialPeer(remotePeerInfo) - # We discarding here just because we going to check future state, to avoid - # condition where connection happens and timeout reached. + # We discarding here just because we going to check future state, + # to avoid condition where connection happens and timeout reached. discard await withTimeout(fut, network.connectTimeout) # We handling only timeout and errors, because successfull connections # will be stored in PeerPool. if fut.finished(): if fut.failed() and not(fut.cancelled()): - debug "Unable to establish connection with peer", peer = remotePeerInfo.id, + debug "Unable to establish connection with peer", + peer = remotePeerInfo.id, errMsg = fut.readError().msg inc nbc_failed_dials network.addSeen(remotePeerInfo, SeenTableTimeDeadPeer) @@ -803,18 +805,18 @@ proc connectWorker(network: Eth2Node) {.async.} = network.connTable.del(remotePeerInfo.peerId) else: trace "Peer is already connected, connecting or already seen", - peer = remotePeerInfo.id, peer_pool_has_peer = $peerPoolHasRemotePeer, seen_table_has_peer = $seenTableHasRemotePeer, - connecting_peer = $remotePeerAlreadyConnected, seen_table_size = len(network.seenTable) - - # Prevent (a purely theoretical) high CPU usage when losing connectivity. - await sleepAsync(1.seconds) + peer = remotePeerInfo.id, + peer_pool_has_peer = $peerPoolHasRemotePeer, + seen_table_has_peer = $seenTableHasRemotePeer, + connecting_peer = $remotePeerConnecting, + seen_table_size = len(network.seenTable) proc runDiscoveryLoop*(node: Eth2Node) {.async.} = debug "Starting discovery loop" let enrField = ("eth2", SSZ.encode(node.forkId)) while true: - let currentPeerCount = node.peerPool.len + let currentPeerCount = node.peerPool.lenAvailable({PeerType.Outgoing}) if currentPeerCount < node.wantedPeers: try: let discoveredPeers = @@ -853,8 +855,9 @@ proc init*(T: type Eth2Node, conf: BeaconNodeConf, enrForkId: ENRForkID, privKey: keys.PrivateKey, rng: ref BrHmacDrbgContext): T = new result result.switch = switch - result.wantedPeers = conf.maxPeers - result.peerPool = newPeerPool[Peer, PeerID](maxPeers = conf.maxPeers) + result.wantedPeers = (conf.maxPeers - conf.maxIncomingPeers) + result.peerPool = newPeerPool[Peer, PeerID]( + maxPeers = conf.maxPeers, maxIncomingPeers = conf.maxIncomingPeers) result.connectTimeout = 10.seconds result.seenThreshold = 10.minutes result.seenTable = initTable[PeerID, SeenItem]() From 18730a9a35d32e46b7b4b63d95a6461372fdb1f9 Mon Sep 17 00:00:00 2001 From: cheatfate Date: Mon, 27 Jul 2020 07:29:28 +0300 Subject: [PATCH 2/3] Add explicit rejection of incoming peer's connections. --- beacon_chain/eth2_network.nim | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/beacon_chain/eth2_network.nim b/beacon_chain/eth2_network.nim index ed9de67c2b..d79f4fbb49 100644 --- a/beacon_chain/eth2_network.nim +++ b/beacon_chain/eth2_network.nim @@ -709,12 +709,15 @@ proc handleIncomingPeer*(peer: Peer): Future[bool] {.async.} = debug "Peer (incoming) lost", peer nbc_peers.set int64(len(network.peerPool)) - let res = await network.peerPool.addIncomingPeer(peer) + let res = network.peerPool.addIncomingPeerNoWait(peer) if res: peer.updateScore(NewPeerScore) debug "Peer (incoming) has been added to PeerPool", peer peer.getFuture().addCallback(onPeerClosed) result = true + else: + debug "Too many incoming connections in PeerPool, disconnecting", peer + result = false nbc_peers.set int64(len(network.peerPool)) From 3ee7fc71f4079941e5c958b0c10875115eaab201 Mon Sep 17 00:00:00 2001 From: cheatfate Date: Wed, 29 Jul 2020 18:29:57 +0300 Subject: [PATCH 3/3] Change default values to odd prime numbers. Add comment about why we limiting number of incoming connections. --- beacon_chain/conf.nim | 4 ++-- beacon_chain/eth2_network.nim | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/beacon_chain/conf.nim b/beacon_chain/conf.nim index a4c364f6f1..0f0b4f1595 100644 --- a/beacon_chain/conf.nim +++ b/beacon_chain/conf.nim @@ -106,12 +106,12 @@ type name: "udp-port" }: Port maxPeers* {. - defaultValue: 100 # The Wall gets released + defaultValue: 101 # The Wall gets released desc: "The maximum number of connected peers" name: "max-peers" }: int maxIncomingPeers* {. - defaultValue: 21 + defaultValue: 23 desc: "The maximum number of inbound peer's connections" name: "max-incoming-peers"}: int diff --git a/beacon_chain/eth2_network.nim b/beacon_chain/eth2_network.nim index d79f4fbb49..e619963d58 100644 --- a/beacon_chain/eth2_network.nim +++ b/beacon_chain/eth2_network.nim @@ -859,6 +859,10 @@ proc init*(T: type Eth2Node, conf: BeaconNodeConf, enrForkId: ENRForkID, new result result.switch = switch result.wantedPeers = (conf.maxPeers - conf.maxIncomingPeers) + # We are limiting number of incoming connections (peers) to some value, + # otherwise it is possible to fill our PeerPool with only incoming connections + # This is undesired behavior, because in such way attackers could easily + # falsify our vision of network and beacon chain. result.peerPool = newPeerPool[Peer, PeerID]( maxPeers = conf.maxPeers, maxIncomingPeers = conf.maxIncomingPeers) result.connectTimeout = 10.seconds