fix: not dial all known peers in parallel on startup #698

vasco-santos · 2020-07-09T09:14:48Z

This PR aims to block large numbers of dials on a peer restart, if this peer already new a large number of peers in the past.

It checks the minPeers as the maybeConnect function used by the discovery event handlers. This was also using the minPeers, but as all the dials were put in parallel, the connectionManager.size would not be updated to avoid further dials.

When we work on connectionManager, we should work on better strategies for this, and likely have them as configurable

vasco-santos · 2020-07-09T09:18:45Z

src/index.js

+    const minPeers = this._options.connectionManager.minPeers || 0
+
+    // TODO: this should be ordered by score on PeerStoreV2
+    this.peerStore.peers.forEach(async (peer) => {


@jacobheun I started by doing the dials in serial as the token based dial will already use parallel dials for several multiaddrs the peer we are targeting might have. Also, the discovery services kicked in, and their new discoveries might be more important than the older ones (this is where having scores for the older peers would help a lot too).

Another solution I thought is to do the minPeers parallel dials and then do the rest in serial (to compensate possible offline peers). Let me know what do you think about the pros and cons of both approaches.

vasco-santos · 2020-07-09T09:21:33Z

src/index.js

+   * @returns {void}
+   */
+  _maybeConnectPeerStorePeers () {
+    if (!this._config.peerDiscovery.autoDial) {


Maybe this should deserve its own config in the peerStore instead? 🤔

jacobheun · 2020-07-09T09:38:19Z

I think the more general case that would be helpful here is going to the PeerStore when our connection count is below the low watermark. Right now autoDial is only ever triggered on peer discovery. I don't think we should be mass emitting peer discovered events on boot for persisted peers. Application code can check the peer store.

I think we should add a counterpart of the ConnectionManager_.maybeDisconnectOne that deals with proactive connecting. If our connection count is below our low watermark, we should attempt to dial peers from our peerstore (ideally ranked, but we can do that later) that we are not connected to, until we are at or above the low watermark.
On startup, we can just start this recursive call. We could batch the requests to reduce initial connection time. This should also get called when a peer disconnects and we drop below the low watermark.

vasco-santos · 2020-07-09T09:55:23Z

I don't think we should be mass emitting peer discovered events on boot for persisted peers. Application code can check the peer store.

Totally agree, but I don't think this is the correct timing for that. We are doing that in 0.27 and 0.28 and this will be a breaking change for who expects this! I agree we include this in 0.30. I can open an issue for it.

I think the more general case that would be helpful here is going to the PeerStore when our connection count is below the low watermark.

This PR does that for boot and I agree with this, but with some other things in consideration. When having a more intelligent PeerStore/ConnectionManager, we should select peers based on their score and the protocols we are running.

I think we should add a counterpart of the ConnectionManager_.maybeDisconnectOne that deals with proactive connecting. If our connection count is below our low watermark, we should attempt to dial peers from our peerstore (ideally ranked, but we can do that later) that we are not connected to, until we are at or above the low watermark.

It seems a good idea to do it on disconnect, but just iterating blindly in the PeerStore and connect will not likely bring much value. But I think we can include it here as it is quite simple.

On startup, we can just start this recursive call. We could batch the requests to reduce initial connection time. This should also get called when a peer disconnects and we drop below the low watermark.

This seems the way to go for me, but this should happen for the 0.28.x line for me.

jacobheun · 2020-07-09T10:09:06Z

Totally agree, but I don't think this is the correct timing for that. We are doing that in 0.27 and 0.28 and this will be a breaking change for who expects this!

We can talk about this separately, it doesn't need to change for this, but it should change soon because we're going to hit scaling issues. If I know thousands of peers when I boot, that's going to get nasty real quick.

It seems a good idea to do it on disconnect, but just iterating blindly in the PeerStore and connect will not likely bring much value.

Actually, we do have some metrics we could use. Instead of looking at ALL peers in the peer store, for now we could prioritize peers we have keys/protocols for. Both should indicate a previous direct exchange, although keys might have come indirectly (i cant recall which subsystems we might have added this to in JS).

…rStore

jacobheun · 2020-07-14T09:27:38Z

src/connection-manager/index.js

@@ -206,6 +210,7 @@ class ConnectionManager extends EventEmitter {
      this.connections.delete(peerId)
      this._peerValues.delete(connection.remotePeer.toB58String())
      this.emit('peer:disconnect', connection)
+      this._maybeConnectN()


If a disconnect occurs and the previous call to _maybeConnectN happens we're going to run two calls in parallel, we should avoid this.

you mean the _maybeConnectN in start?

We can potentially have it running from the start call while trying to dial a bunch of peers and one of them meanwhile disconnected. I can add a verification for wether we are currently running it

jacobheun · 2020-07-14T10:20:34Z

src/connection-manager/index.js

+    this._isTryingToConnect = true
+


This can go away

Suggested change

this._isTryingToConnect = true

src/connection-manager/index.js

jacobheun · 2020-07-14T10:26:05Z

src/connection-manager/index.js

+      }
+    }
+
+    this._maybeConnectTimeout = setTimeout(this._maybeConnectN, this._options.maybeConnectInterval)


Use retimer for this. We're already using it and it will cut down on multiple timers in the background.

jacobheun · 2020-07-14T12:26:21Z

test/connection-manager/index.node.js

+          },
+          connectionManager: {
+            minConnections,
+            maybeConnectInterval


This should be autoDialInterval. Since it's the wrong param, the default should be 10s, it's odd this test is passing. I assume it's actively in the process of connecting when the first check is done.

Yeah, I debugged it earlier. It was failing sometimes before I add the interval param. I will fix it now

jacobheun

LGTM. We'll look at cleaning up this connection manager flow when we tackle the v2 work for that (tentatively 0.30/0.31).

vasco-santos commented Jul 9, 2020

View reviewed changes

vasco-santos changed the title ~~fix: not dial all known peers on startup~~ fix: not dial all known peers in parallel on startup Jul 9, 2020

vasco-santos commented Jul 9, 2020

View reviewed changes

fix: not dial all known peers on startup

695f77c

vasco-santos force-pushed the fix/not-dial-all-know-peers-on-startup branch from e25915f to 695f77c Compare July 9, 2020 09:22

vasco-santos added 2 commits July 9, 2020 15:29

feat: connection manager should proactively connect to peers from pee…

3fcc2d6

…rStore

chore: increase bundle size

4afb801

vasco-santos marked this pull request as ready for review July 9, 2020 13:46

vasco-santos requested a review from jacobheun July 9, 2020 13:54

jacobheun reviewed Jul 14, 2020

View reviewed changes

fix: do connMgr proactive dial on an interval

6ac3328

jacobheun reviewed Jul 14, 2020

View reviewed changes

vasco-santos added 2 commits July 14, 2020 12:48

chore: address review

9156e29

chore: use retimer reschedule

2985a5f

jacobheun reviewed Jul 14, 2020

View reviewed changes

vasco-santos added 3 commits July 14, 2020 14:29

chore: address review

4ea8838

fix: use minConnections in default config

c92d2ea

chore: minPeers to minConnections everywhere

43a52a6

jacobheun approved these changes Jul 14, 2020

View reviewed changes

jacobheun merged commit 9ccab40 into master Jul 14, 2020

jacobheun deleted the fix/not-dial-all-know-peers-on-startup branch July 14, 2020 14:05

vasco-santos mentioned this pull request Jul 15, 2020

[tests] fix flakey pubsub test #709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: not dial all known peers in parallel on startup #698

fix: not dial all known peers in parallel on startup #698

vasco-santos commented Jul 9, 2020

vasco-santos Jul 9, 2020

vasco-santos Jul 9, 2020

jacobheun commented Jul 9, 2020

vasco-santos commented Jul 9, 2020

jacobheun commented Jul 9, 2020

jacobheun Jul 14, 2020

vasco-santos Jul 14, 2020

vasco-santos Jul 14, 2020

jacobheun Jul 14, 2020

jacobheun Jul 14, 2020

jacobheun Jul 14, 2020

vasco-santos Jul 14, 2020

jacobheun left a comment

fix: not dial all known peers in parallel on startup #698

fix: not dial all known peers in parallel on startup #698

Conversation

vasco-santos commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Jul 9, 2020

vasco-santos commented Jul 9, 2020

jacobheun commented Jul 9, 2020

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

jacobheun left a comment

Choose a reason for hiding this comment