feat: auto dial discovered peers #349

jacobheun · 2019-04-04T18:52:10Z

This PR leverages the strategies detailed at #349 (comment) to implement auto dial in libp2p. This should improve our ability to maintain an appropriate level of connections based on the min and max peers configured for the ConnectionManager.

Applications leveraging libp2p should ensure they are not dialing discovered peers when using this update. Applications that do wish to handle all dialing can disable autoDial in the config.

Depends on libp2p/js-libp2p-switch#325

alanshaw · 2019-04-05T07:58:32Z

That's interesting - can you foresee any strategies other than low and all?

jacobheun · 2019-04-05T12:24:09Z

After thinking through this a bit more I put together some notes. The main take away would be to change the behavior of libp2p to:

auto dial discovered peers under the low watermark
allow this to be disabled
applications should not dial on discovery unless they have disabled this.

Scenarios

In any scenario, if a peer is discovered it should be added to the PeerBook. This ensures that
even if we don't dial to a node when we discover it, we know about it in the event that it
becomes known as a provider for something we need.

1. Joining the network

The node is new and needs to join the network. It currently has 0 peers.
Discovery Mechanisms: Ambient Discovery

Action to take

Connect to discovered peers. This should have some degree of concurrency limiting. While the case should be low, if we immediately discover more peers than our high watermark we should avoid dialing them all.

2. Connected to some

The node is connected to other nodes. The current number of connections is less than the desired low watermark.
Discovery Mechanisms: Ambient Discovery and Active Discovery

Action to take

Connect to discovered peers. This should have some degree of concurrency limiting. The concurrency may need to be modified to reflect the current number of peers connected. The more peers we have, the lower the concurrency may need to be.

3. Connected to enough

Discovery Mechanisms: Ambient Discovery and Active Discovery

Action to take

None. If we are connected to enough peers, the low watermark, we should not connect to discovered peers. As other peers discover us, they may connect to us based on their current scenario.

For example, a long running node with adequate peers is on an MDNS network. A new peer joins the network and both become aware of each other. The new peer should be the peer that dials, as it has too few peers. The existing node has no reason to dial the new peer, but should keep a record of it in case it later becomes an important node due to its contents/capabilities.

Avoiding dials above the low watermark also allows for a pool of connections to be reserved for application specific actions, such as connecting to a specific content provider via a DHT query to find that content (ipfs-bitswap).

4. Connected to too many

The node has more connections than it wants. The current number of connections is greater than the high watermark.

WIP Connection Manager v2 spec
Discovery Mechanisms: Ambient Discovery and Active Discovery

Discovery Mechanisms

Means of which a libp2p node discovers other peers.

Active Discovery

Through active use of the libp2p network, a node may discovery peers.

Content/Peer routing (DHT, delegated, etc) provider and peer queries
DHT random walk
Rendezvous servers

Ambient Discovery

Leveraging known addresses, or network discovery mechanisms, a node may discover peers outside of the bounds of the libp2p network.

Bootstrap
MDNS
proximity based (bluetooth, sound, etc)

Thoughts

All peers discovered should be emitted via peer:discovery as applications may wish to do something with that information.
Libp2p should default to automatically connecting to new peers, when under the low watermark.
- Applications should be able to disable this, and handle connections themselves.
- Applications who have not disabled this should never connect on peer discovery.

dirkmc · 2019-04-08T12:47:57Z

This sounds like a good approach 👍

A few questions:

Should we make any distinction between inbound / outbound / duplex connections?
Do the LOW / ALL discovery strategies still apply in this second set of concepts?
Should the dial queue have a priority assigned to dials? (eg low for discovered peers, high for peers that are actively dialed by libp2p clients)

jacobheun · 2019-04-08T13:54:09Z

Should we make any distinction between inbound / outbound / duplex connections?

There may be value in doing so in the future. I think the first step here will be improving how inbound connections are being handled in the switch and adding the appropriate tags to the connections, which is something the connection manager needs.

Do the LOW / ALL discovery strategies still apply in this second set of concepts?

No, we shouldn't need the discovery strategies anymore. I pushed up the latest code I have (tests need to be fixed). This changes things over to a configurable autoDial approach.

Should the dial queue have a priority assigned to dials? (eg low for discovered peers, high for peers that are actively dialed by libp2p clients)

Yes, I think they should. Doing auto dialing makes this a lot easier to know where things are coming from so deprioritizing these calls becomes a lot easier.

vasco-santos · 2019-04-08T14:27:57Z

Thanks for clearly exposing this problem and your ideas in this regard @jacobheun ! 🙌

Totally agree with the following reasoning 👍 :

Automatic dial when below the low watermark
Don't dial when higher than the low watermark. We need to keep dial queues ready for application specific actions and the applications should be able to connect if they want to.
Behaviors configurable from outside for applications being able overwrite

alanshaw · 2019-04-09T08:50:29Z

👍 thanks for putting so much thought into this @jacobheun - this sounds like a good way forward to limit the number of dials we're doing.

alanshaw · 2019-04-09T08:51:14Z

Can we add your documentation here to the README?

jacobheun · 2019-04-09T09:39:30Z

Since there's a good amount of info there I created a separate readme with the peer discovery / auto dial information. I also updated some language in the main readme and linked to the new readme.

README.md

src/index.js

alanshaw · 2019-04-09T14:40:22Z

src/index.js

+
+    if (!this.isStarted()) return
+
+    this.emit('peer:discovery', peerInfo)


Why no discovery event if not started?

It keeps a record of those peers and will emit them on startup. This avoids the scenario where you disable autoDial and get a bootstrap or mdns peer discovered result before libp2p starts. If you try to dial and libp2p hasn't finished starting it will fail. This just makes libp2p take care of the logic internally.

alanshaw · 2019-04-09T14:43:03Z

src/index.js

+        this.emit('peer:discovery', peerInfo)
+        this._maybeConnect(peerInfo)
+      })
+    })


Do we need to unregister this on stop?

This should really run anytime the node starts. It will get run multiple times if the node is started again, though. I can just register this in the constructor to avoid the need to remove and re-add it on a restart.

src/index.js

alanshaw · 2019-04-09T14:52:17Z

PEER_DISCOVERY.md

+* All peers discovered are emitted via `peer:discovery` so applications can take any desired action.
+* Libp2p defaults to automatically connecting to new peers, when under the [ConnectionManager](https://github.com/libp2p/js-libp2p-connection-manager) low watermark (minimum peers).
+  * Applications can disable this via the `peerDiscovery.autoDial` config property, and handle connections themselves.
+  * Applications who have not disabled this should **never** connect on peer discovery, unless performing a specific action.


Maybe elaborate on what "performing a specific action" is. This is not just "dial with a protocol", you can cold call but it should be in response to some user action.

Thinking through this more, I don't think there's any reason for a connect to ever happen on peer discovery with auto dial on. peer:connect would be a more appropriate event to take action on.

PEER_DISCOVERY.md

Co-Authored-By: jacobheun <jacobheun@gmail.com>

dirkmc

A couple of nits, otherwise LGTM 👍

src/index.js

alanshaw

Code LGTM, I would like to see how this performs (just haven't had time this week), but we can always iterate if you want to get this merged.

vasco-santos

LGTM

ghost assigned jacobheun Apr 4, 2019

ghost added the status/in-progress In progress label Apr 4, 2019

feat: add strategies to discovery configs

2f10234

jacobheun changed the base branch from fix/remove-put to master April 5, 2019 12:29

jacobheun force-pushed the feat/disco-strategies branch from 892c1d3 to 2f10234 Compare April 5, 2019 12:29

feat: add auto dial support

4ec1701

test: fix transport tests

b1c9bb1

jacobheun changed the title ~~[WIP][RFC] feat: support discovery strategies~~ [WIP][RFC] feat: auto dial discovered peers Apr 8, 2019

refactor: do some cleanup and fix tests

568c423

fix: use switch priority queue via .connect

f52e7fd

jacobheun mentioned this pull request Apr 9, 2019

feat: support a priority queue for dials libp2p/js-libp2p-switch#325

Merged

chore: fix lint

600bdd4

alanshaw suggested changes Apr 9, 2019

View reviewed changes

Alan Shaw and others added 5 commits April 9, 2019 17:54

chore: fix README.md language

ebb49e8

Co-Authored-By: jacobheun <jacobheun@gmail.com>

chore: fix feedback

f9b9834

test: disable autodial for browser tests

2d4c675

test: fix browser tests for auto dial

7c9d1b4

docs: update peer discovery readme language

5aabb17

jacobheun changed the title ~~[WIP][RFC] feat: auto dial discovered peers~~ feat: auto dial discovered peers Apr 10, 2019

jacobheun marked this pull request as ready for review April 10, 2019 09:08

jacobheun requested review from vasco-santos and dirkmc April 10, 2019 09:08

jacobheun requested a review from alanshaw April 10, 2019 09:08

test: improve browser test determinism

c3e2072

jacobheun mentioned this pull request Apr 10, 2019

fix: use libp2p auto dial ipfs/js-ipfs#1983

Merged

1 task

dirkmc suggested changes Apr 10, 2019

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

src/index.js Show resolved Hide resolved

alanshaw approved these changes Apr 10, 2019

View reviewed changes

fix: resolve feedback

b26e0ba

jacobheun requested a review from dirkmc April 10, 2019 17:02

jacobheun added 2 commits April 10, 2019 20:50

chore: update switch version

3bb07b3

chore: bump bundle size for switch updates

6a82f61

dirkmc approved these changes Apr 11, 2019

View reviewed changes

vasco-santos approved these changes Apr 11, 2019

View reviewed changes

alanshaw mentioned this pull request Apr 11, 2019

Cannot retrieve content via the DHT ipfs/js-ipfs#1984

Closed

jacobheun merged commit 01aa447 into master Apr 11, 2019

ghost removed the status/in-progress In progress label Apr 11, 2019

jacobheun deleted the feat/disco-strategies branch April 11, 2019 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto dial discovered peers #349

feat: auto dial discovered peers #349

jacobheun commented Apr 4, 2019 •

edited

Loading

alanshaw commented Apr 5, 2019 •

edited

Loading

jacobheun commented Apr 5, 2019 •

edited

Loading

dirkmc commented Apr 8, 2019

jacobheun commented Apr 8, 2019

vasco-santos commented Apr 8, 2019

alanshaw commented Apr 9, 2019

alanshaw commented Apr 9, 2019

jacobheun commented Apr 9, 2019

alanshaw Apr 9, 2019

jacobheun Apr 9, 2019

alanshaw Apr 9, 2019

jacobheun Apr 9, 2019

alanshaw Apr 9, 2019

jacobheun Apr 9, 2019

dirkmc left a comment

alanshaw left a comment •

edited

Loading

vasco-santos left a comment


		if (!this.isStarted()) return

		this.emit('peer:discovery', peerInfo)

feat: auto dial discovered peers #349

feat: auto dial discovered peers #349

Conversation

jacobheun commented Apr 4, 2019 • edited Loading

alanshaw commented Apr 5, 2019 • edited Loading

jacobheun commented Apr 5, 2019 • edited Loading

Scenarios

1. Joining the network

Action to take

2. Connected to some

Action to take

3. Connected to enough

Action to take

4. Connected to too many

Discovery Mechanisms

Active Discovery

Ambient Discovery

Thoughts

dirkmc commented Apr 8, 2019

jacobheun commented Apr 8, 2019

vasco-santos commented Apr 8, 2019

alanshaw commented Apr 9, 2019

alanshaw commented Apr 9, 2019

jacobheun commented Apr 9, 2019

alanshaw Apr 9, 2019

Choose a reason for hiding this comment

jacobheun Apr 9, 2019

Choose a reason for hiding this comment

alanshaw Apr 9, 2019

Choose a reason for hiding this comment

jacobheun Apr 9, 2019

Choose a reason for hiding this comment

alanshaw Apr 9, 2019

Choose a reason for hiding this comment

jacobheun Apr 9, 2019

Choose a reason for hiding this comment

dirkmc left a comment

Choose a reason for hiding this comment

alanshaw left a comment • edited Loading

Choose a reason for hiding this comment

vasco-santos left a comment

Choose a reason for hiding this comment

jacobheun commented Apr 4, 2019 •

edited

Loading

alanshaw commented Apr 5, 2019 •

edited

Loading

jacobheun commented Apr 5, 2019 •

edited

Loading

alanshaw left a comment •

edited

Loading