fix!: refactor connection manager to use a prioritised queue #1678

achingbrain · 2023-04-06T17:40:08Z

Refactors the connection manager.

Internally it uses a queue to control concurrency instead of dial tokens (e.g. maxParallelDials)
A second queue is used for each peer to prevent inividual peers from swamping the dial queue (e.g. maxParallelDialsPerPeer)
The auto dialler now also checks the minimum connection limit on peer discovery and peer disconnect events for fast reconnect
Auto dialled peers are sorted based on tag value so valuable peers should be re-dialled first
Auto dialled peers are dialled in parallel to prevent a slow peer locking up the auto-dial queue
A getDialQueue method has been added to libp2p to allow inspection of the dial queue though it needs exposing in the interface
The connection gater denyDialMultiaddr method now only takes a multiaddr
Fixes duplicate connections to fast peers
Fixes unhanded promise rejections Use of Promise.any() can cause unhandled promise rejection #1587
Connection pruning now happens after peer:connect events not before - fixes Should not emit peer:connect with a closed connection #1565

BREAKING CHANGE: some connection manager options have changed - please see the upgrade guide for full details

SgtPooki

Partial review

SgtPooki · 2023-04-07T05:00:44Z

doc/migrations/v0.43-v0.44.md

+    // these should now be ranges expressed as MultiaddrFilters
+    allow: [
+      '/ip4/0.0.0.0/tcp/123'
+    ],
+
+    // these should now be ranges expressed as MultiaddrFilters
+    deny: [
+      '/ip4/0.0.0.0/tcp/123'
+    ]


Can we get a "allow peer" that allows any appropriate transports given a specific peerId?

I was trying to do that the other day in kubo and realized i needed a full multiaddr. I was trying to grant full-access to/from my helia node and couldn't figure out how to do it within an hour or so and i gave up.

It would be great to be able to do that in js-libp2p

I'm a little confused, access is allowed by default, it's only disallowed if a configured connection gater denys it or you're at maxConnections already.

The allow list above enables specific networks to breach connection limits - a DoS mitigation for when your node is being eclipsed.

It's almost certainly not the solution to your problem - maybe you can open an issue on the helia repo with a repro case?

sure. I will re-open if I run into it.. essentially what I was trying to do was prefer my local node to others, and to allow connections to that one no matter what. it sounds like that may not be necessary

doc/migrations/v0.43-v0.44.md

SgtPooki · 2023-04-07T05:01:41Z

doc/migrations/v0.43-v0.44.md

+    // a low value allows user-initiated dials to take priority over
+    // auto dials
+    autoDialPriority: 0,


SgtPooki · 2023-04-07T05:02:30Z

doc/migrations/v0.43-v0.44.md

+    // this was previously named `maxAddrsToDial`
+    maxPeerAddrsToDial: 20,


PerConnection attempt? Or total(then we banlist the peer as bad??)

Might be good to call out which one and how in jsdoc, or in the name if possible.

Did you check the jsdoc for this config option? If it's not clear could you suggest an edit?

SgtPooki · 2023-04-07T05:05:48Z

doc/migrations/v0.43-v0.44.md

+    // this was previously named `maxDialsPerPeer`
+    maxParallelDialsPerPeer: 20,


Just so i understand fully, this setting changes whether we can attempt to dial 2 or 20 multiaddrs at once when attempting to connect to a specific peer, correct?

Would 'maxParallelMultiAddrDialsPerPeer' be a more explicit name? Is there a way we can get that explicitness without the verbosity?

We only dial multiaddrs so maybe having MultiAddr in the name is redundant?

But yes, a setting of 2 with a peer that has 10 addresses means we'll dial all 10 addresses but only ever 2 at once. This stops a peer with a lot of spam addresses blocking the dial queue.

SgtPooki · 2023-04-07T05:08:14Z

doc/migrations/v0.43-v0.44.md

+    // these should now be ranges expressed as MultiaddrFilters
+    allow: [
+      new MultiaddrFilter('/ip4/192.168.1.1/ipcidr/32')
+    ],
+


As above, can i create a multiaddrFilter consisting only of '/p2p/peerId' ?

No, the allow/deny lists are for allowing/denying classes of network to make connections - this happens before the peer id exchange so we don't know who the remote peer is at that point.

It's a DoS mitigation strategy to always allow connections from networks you know, control or trust, or disallow connections from networks that are attacking you.

To deny specific peers you can implement a connection gater.

SgtPooki · 2023-04-07T05:09:17Z

doc/migrations/v0.43-v0.44.md

+    denyDialMultiaddr: (multiaddr) => {
+      if (multiaddr.getPeerId() != null) {
+        // there is a peer id present in the multiaddr
+      }


SgtPooki · 2023-04-07T05:12:33Z

src/connection-manager/auto-dial.ts

+   * The minimum number of connections below which libp2p will start to dial
+   * peers from the peer book. (default: 0)
+   */
+  minConnections?: number


MinConnections: 0 is the same as saying, I'm allowed to have zero connections, don't autoDial, correct?

Might be useful to call out that setting this to zero disables it

Turns out that default was wrong 🤭 - would much rather generate that info but not sure how.

I've updated the jsdoc for this setting - if it's still not clear could you suggest an edit?

js-libp2p/src/connection-manager/index.ts

Lines 39 to 44 in 4078082

/**

* The minimum number of connections below which libp2p will start to dial peers

* from the peer book. Setting this to 0 effectively disables this behaviour.

* (default: 50)

*/

minConnections?: number

looks much better to me, thanks :)

src/connection-manager/auto-dial.ts

maschad

Great work on this @achingbrain will definitely improve with connection management overall. I made some comments and suggestions.

I think these are a lot of changes though and perhaps we should split this into some smaller PRs to make reviewing a bit easier and the changes more manageable. I would suggest separate PRs for:

Use standard IP cidr format for allow/deny lists #1510
bug: heliaInstance.libp2p.addEventListener('peer:discovery', cb) calls cb twice for same peer #1630
denyDialMultiaddr method now only takes a multiaddr and also the other interfaces updates required with it.

doc/CONFIGURATION.md

doc/migrations/v0.43-v0.44.md

src/connection-manager/auto-dial.ts

src/connection-manager/dial-queue.ts

src/connection-manager/index.ts

…ling Refactors the connection manager. 1. Internally it uses a queue to control concurrency instead of dial tokens 2. A second queue is used for each peer to prevent inividual peers from swamping the dial queue 3. The auto dialler now checks the minimum connection limit when peers disconnect instead of using a timer 4. Auto dialled peers are sorted based on tag value so valuable peers should be re-dialled first 5. Auto dialled peers are dialled in parallel to prevent a slow peer locking up the auto-dial queue 6. allow/deny lists are now `MultiaddrFilter`s 7. A `getDialQueue` method has been added to libp2p to allow inspection of the dial queue though it needs exposing in the interface 8. The connection gater `denyDialMultiaddr` method now only takes a multiaddr BREAKING CHANGE: some connection manager options have changed - please see the upgrade guide for full details

Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>

achingbrain · 2023-04-11T13:04:32Z

I've removed the attempt at resolving the double-peer discovery bug - I think it needs a bit more thought, basically the logic to determine whether to emit the event is complicated and will be vastly simplified by having an atomic peer store.

The auto dial interval has also returned - this is because I found an edge case where we could still end up with no peers if all auto-dials failed - if network connection is lost, for example.

I've reverted the MultiaddrFilter change for allow/deny lists too - with a bit more thought it would be better to have the connection gater control allow/deny behaviour, then we aren't splitting responsibility for that between the connection manager and the connection gater.

maschad

LGTM

achingbrain · 2023-04-13T15:04:49Z

~~Please hold off on merging until jacobheun/any-signal#25 or jacobheun/any-signal#26 is merged~~

All good.

achingbrain changed the title ~~fix!: refactor connection manager to use a queue and add priority dia…~~ fix!: refactor connection manager to use a queue and add priority dialing Apr 6, 2023

achingbrain mentioned this pull request Apr 6, 2023

feat(auto-dialler): dial in parallel #1668

Closed

achingbrain changed the title ~~fix!: refactor connection manager to use a queue and add priority dialing~~ fix!: refactor connection manager to use a queue Apr 6, 2023

achingbrain changed the title ~~fix!: refactor connection manager to use a queue~~ fix!: refactor connection manager to use a prioritsed queue Apr 6, 2023

achingbrain changed the title ~~fix!: refactor connection manager to use a prioritsed queue~~ fix!: refactor connection manager to use a prioritised queue Apr 6, 2023

SgtPooki reviewed Apr 7, 2023

View reviewed changes

achingbrain requested review from wemeetagain and maschad April 7, 2023 10:57

maschad requested changes Apr 9, 2023

View reviewed changes

achingbrain and others added 6 commits April 11, 2023 12:58

chore: consistency

3b66103

chore: apply suggestions from code review

e2f406b

Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>

fix: unhandled promise rejections in browsers

ee7cd99

chore: close superfluous connections when multiple dials succeed

6ff8ed5

chore: restore interval, restore string allow/deny

3d92f93

achingbrain force-pushed the fix/refactor-connection-manager branch from dbca29e to 3d92f93 Compare April 11, 2023 11:59

chore: update docs

1e802d9

chore: update docs

dec86ff

maschad approved these changes Apr 12, 2023

View reviewed changes

chore: update any-signal to fix memory leak

4c4ab3b

achingbrain merged commit 4c02e48 into master Apr 13, 2023

achingbrain deleted the fix/refactor-connection-manager branch April 13, 2023 19:33

achingbrain mentioned this pull request Apr 18, 2023

fix: subsequent dial abort #763

Closed

maschad mentioned this pull request Apr 25, 2023

feat: Update Connection Manager and Dialler Interfaces and Configs. libp2p/js-libp2p-interfaces#342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: refactor connection manager to use a prioritised queue #1678

fix!: refactor connection manager to use a prioritised queue #1678

achingbrain commented Apr 6, 2023 •

edited

Loading

SgtPooki left a comment

SgtPooki Apr 7, 2023

achingbrain Apr 7, 2023 •

edited

Loading

SgtPooki Apr 14, 2023

SgtPooki Apr 7, 2023

SgtPooki Apr 7, 2023

achingbrain Apr 7, 2023

SgtPooki Apr 7, 2023

achingbrain Apr 7, 2023

achingbrain Apr 7, 2023

SgtPooki Apr 7, 2023

achingbrain Apr 7, 2023

SgtPooki Apr 7, 2023

SgtPooki Apr 7, 2023

achingbrain Apr 7, 2023

SgtPooki Apr 14, 2023

maschad left a comment

achingbrain commented Apr 11, 2023

maschad left a comment

achingbrain commented Apr 13, 2023 •

edited

Loading

		// this was previously named `maxAddrsToDial`
		maxPeerAddrsToDial: 20,

		// this was previously named `maxDialsPerPeer`
		maxParallelDialsPerPeer: 20,

	/**
	* The minimum number of connections below which libp2p will start to dial peers
	* from the peer book. Setting this to 0 effectively disables this behaviour.
	* (default: 50)
	*/
	minConnections?: number

fix!: refactor connection manager to use a prioritised queue #1678

fix!: refactor connection manager to use a prioritised queue #1678

Conversation

achingbrain commented Apr 6, 2023 • edited Loading

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain Apr 7, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maschad left a comment

Choose a reason for hiding this comment

achingbrain commented Apr 11, 2023

maschad left a comment

Choose a reason for hiding this comment

achingbrain commented Apr 13, 2023 • edited Loading

achingbrain commented Apr 6, 2023 •

edited

Loading

achingbrain Apr 7, 2023 •

edited

Loading

achingbrain commented Apr 13, 2023 •

edited

Loading