feat(libp2p): add autodial retry threshold config option #1943

achingbrain · 2023-08-08T16:21:18Z

When auto-dialing peers, if we have failed to dial them recently there's little point in redialing them as it's probably just going to fail again which consumes a slot in the dial queue and other resources.

Adds a autoDialPeerRetryThreshold config key to the connection manager which is a value in ms.

If we have attempted to dial a peer and that dial attempt failed but we are under our min connection count, do not auto dial the peer within the retry threshold.

Defaults to 1 minute.

Closes #1899

When auto-dialing peers, if we have failed to dial them recently there's little point in redialing them. Adds a `autoDialPeerRetryThreshold` which is a value in ms. If we have attempted to dial a peer and that dial attempt failed but we are under our min connection count, do not auto dial the peer within the retry threshold. Defaults to 10 minutes.

maschad

Looks promising, and should address some of the concerns of #1899. I have a few comments.

packages/libp2p/src/connection-manager/dial-queue.ts

maschad · 2023-08-09T14:44:34Z

packages/libp2p/test/connection-manager/auto-dial.spec.ts

+    expect(connectionManager.openConnection.callCount).to.equal(1)
+    expect(connectionManager.openConnection.calledWith(matchPeerId(peerWithAddress.id))).to.be.true()
+    expect(connectionManager.openConnection.calledWith(matchPeerId(undialablePeer.id))).to.be.false()
+  })


I think we should add another test to ensure that the undialable peer is eventually dialled once the threshold has expired.

packages/libp2p/test/connection-manager/auto-dial.spec.ts

SgtPooki

some comments / questions. Action items i would love to see addressed prior to merging:

lower the default value
don't set LAST_DIAL_FAILED if not an auto-dial dial

SgtPooki · 2023-08-11T21:25:11Z

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

+  /**
+   * When we've failed to dial a peer, do not autodial them again within this
+   * number of ms. (default: 10 minutes)
+   */
+  autoDialPeerRetryThreshold?: number


honestly, even 1 minute would significantly help from my testing experience.

Once we add in a badPeer/bad multiaddr, it will free up time to check others instead of constantly spamming the same addresses.

SgtPooki · 2023-08-11T21:27:18Z

packages/libp2p/src/connection-manager/auto-dial.ts

+      const when = parseInt(uint8ArrayToString(lastDialFailure))
+
+      if (isNaN(when)) {
+        return true
+      }
+
+      // only dial if the time since the last failure is above the retry threshold
+      return Date.now() - when > this.autoDialPeerRetryThresholdMs


Suggested change

const when = parseInt(uint8ArrayToString(lastDialFailure))

if (isNaN(when)) {

return true

}

// only dial if the time since the last failure is above the retry threshold

return Date.now() - when > this.autoDialPeerRetryThresholdMs

const lastDialFailuretimestamp = parseInt(uint8ArrayToString(lastDialFailure))

if (isNaN(lastDialFailuretimestamp)) {

return true

}

// only dial if the time since the last failure is above the retry threshold

return Date.now() - lastDialFailuretimestamp > this.autoDialPeerRetryThresholdMs

a more intentional variable name here would be nice

SgtPooki · 2023-08-11T21:28:49Z

packages/libp2p/src/connection-manager/constants.ts

+/**
+ * @see https://libp2p.github.io/js-libp2p/interfaces/libp2p.index.unknown.ConnectionManagerInit.html#autoDialPeerRetryThreshold
+ */
+export const AUTO_DIAL_PEER_RETRY_THRESHOLD = 1000 * 60 * 10


I think we could safely lower this and still achieve the goals of not spamming likely-to-fail multiaddrs, while also not blocking users with a 10 minute wait (or forcing a refresh in the browser)

1-2 minutes would be a significant improvement and minor inconvenience in the case of internet outages.

A good approach could be determining how long it takes(D) to dial typical size of peers(N) and setting the value to some ratio of that number. If we expect users to typically find 100-200 peers per session (in the browser that's likely), and it takes 5 minutes to dial them all, then 5 minutes should be default.. but I doubt it needs to be that high.

SgtPooki · 2023-08-11T21:33:20Z

packages/libp2p/src/connection-manager/constants.ts

+
+/**
+ * Store as part of the peer store metadata for a given peer, the value for this
+ * key is a stringified number representing the timestamp of the last time a


Suggested change

* key is a stringified number representing the timestamp of the last time a

* key is a timestamp representing the last time a

I think the existing comment is wordier than needs to be

packages/libp2p/src/connection-manager/constants.ts

SgtPooki · 2023-08-11T21:37:52Z

packages/libp2p/src/connection-manager/dial-queue.ts

@@ -230,9 +232,22 @@ export class DialQueue {
        // clean up abort signals/controllers
        signal.clear()
      })
-      .catch(err => {
+      .catch(async err => {


if i'm not mistaken, this dial-queue is also used for direct dials, should we discern between an auto-dial triggered dial vs a manually/user requested dial?

SgtPooki · 2023-08-11T21:38:46Z

packages/libp2p/test/connection-manager/auto-dial.spec.ts

+  it('should not re-dial peers we have recently failed to dial', async () => {
+    const peerWithAddress: Peer = {
+      id: await createEd25519PeerId(),
+      protocols: [],
+      addresses: [{
+        multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4001'),
+        isCertified: true
+      }],
+      metadata: new Map(),
+      tags: new Map()
+    }
+    const undialablePeer: Peer = {
+      id: await createEd25519PeerId(),
+      protocols: [],
+      addresses: [{
+        multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4002'),
+        isCertified: true
+      }],
+      // we failed to dial them recently
+      metadata: new Map([[LAST_DIAL_FAILURE_KEY, uint8ArrayFromString(`${Date.now() - 10}`)]]),
+      tags: new Map()
+    }
+
+    await peerStore.save(peerWithAddress.id, peerWithAddress)
+    await peerStore.save(undialablePeer.id, undialablePeer)
+
+    const connectionManager = stubInterface<ConnectionManager>({
+      getConnectionsMap: new PeerMap(),
+      getDialQueue: []
+    })
+
+    autoDialler = new AutoDial({
+      peerStore,
+      connectionManager,
+      events
+    }, {
+      minConnections: 10,
+      autoDialInterval: 10000
+    })
+    autoDialler.start()
+    void autoDialler.autoDial()


See my other comment as well, but do we need all the test scaffolding for auto-dialer or can we just attempt to dial the peer directly?

regardless of the answer here, we may want another test regarding direct-dials covering the expected cases (dial them again even if failed auto dial previously, because it's a direct dial request)

The last-failed-dial key is only used by the auto-dialer to exclude those peers from autodialing, it's not used by explicit dials.

I see that now, thanks

SgtPooki · 2023-08-11T21:40:07Z

packages/libp2p/src/connection-manager/dial-queue.ts

        log.error('dial failed to %s', pendingDial.multiaddrs.map(ma => ma.toString()).join(', '), err)

+        if (peerId != null) {


Suggested change

if (peerId != null) {

if (peerId != null && priority < someVal) {

to address manual dials? or do we want a specific dial option to flag whether we will update the LAST_DIAL_FAILURE_KEY for a peer that we only use with auto-dialer?

achingbrain · 2023-08-15T12:44:26Z

don't set LAST_DIAL_FAILED if not an auto-dial dial

Any reason why? The value is only used by the auto-dialer to exclude peers from the list to dial, direct dials don't use this.

maschad

Looks good, ~~I would add another test to ensure that the undiallable peer is eventually dialled once the threshold has expired, but my opinion is strong on that~~.

achingbrain · 2023-08-15T13:51:38Z

I would add another test to ensure that the undiallable peer is eventually dialled once the threshold has expired

I added this check into the existing test

maschad · 2023-08-15T13:55:24Z

I added this check into the existing test

Ah, I see now, great.

SgtPooki · 2023-08-15T15:09:06Z

packages/libp2p/src/connection-manager/constants.ts

+
+/**
+ * Store as part of the peer store metadata for a given peer, the value for this
+ * key is a stringified number representing the timestamp of the last time a


I think the existing comment is wordier than needs to be

SgtPooki · 2023-08-15T15:11:43Z

packages/libp2p/src/connection-manager/auto-dial.ts

+      const when = parseInt(uint8ArrayToString(lastDialFailure))
+
+      if (isNaN(when)) {
+        return true
+      }
+
+      // only dial if the time since the last failure is above the retry threshold
+      return Date.now() - when > this.autoDialPeerRetryThresholdMs


a more intentional variable name here would be nice

achingbrain requested review from wemeetagain and maschad August 8, 2023 16:21

achingbrain changed the title ~~feat: add autodial retry threshold for peers that we have previously failed to dial~~ feat: add autodial retry threshold config option Aug 8, 2023

maschad requested changes Aug 9, 2023

View reviewed changes

achingbrain mentioned this pull request Aug 10, 2023

[🏆 Golden path scenario] Browser-authored content retrievable by another machine through the ipfs.io gateway directly ipfs/helia#182

Open

achingbrain changed the title ~~feat: add autodial retry threshold config option~~ feat(libp2p): add autodial retry threshold config option Aug 10, 2023

maschad mentioned this pull request Aug 10, 2023

feat: Prioritize peers based on previous dial succession / failure #1953

Closed

SgtPooki requested changes Aug 11, 2023

View reviewed changes

chore: lower the default value

9b59ccc

achingbrain added 2 commits August 15, 2023 13:55

chore: update test to actually test retry threshold

dea67af

Merge branch 'master' into feat/do-not-reconnect

bcc23a3

achingbrain requested review from maschad and SgtPooki August 15, 2023 13:29

maschad approved these changes Aug 15, 2023

View reviewed changes

SgtPooki approved these changes Aug 15, 2023

View reviewed changes

achingbrain added 3 commits August 15, 2023 17:22

Merge remote-tracking branch 'origin/master' into feat/do-not-reconnect

295544a

chore: pr comments

8285e9e

chore: update comment

e65724f

achingbrain merged commit 4ef9c79 into master Aug 16, 2023
18 checks passed

achingbrain deleted the feat/do-not-reconnect branch August 16, 2023 06:54

maschad mentioned this pull request Aug 28, 2023

bug: connection manager seems to have no awareness of pending connections #1898

Closed

maschad mentioned this pull request Oct 2, 2023

fix: replace autodial retry threshold in peers to be per multiaddr #2114

Closed

This was referenced Jan 18, 2024

chore(main): release 1.0.0 #2365

Closed

chore(main): release 1.0.0 #2366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(libp2p): add autodial retry threshold config option #1943

feat(libp2p): add autodial retry threshold config option #1943

achingbrain commented Aug 8, 2023 •

edited

Loading

maschad left a comment •

edited

Loading

maschad Aug 9, 2023

SgtPooki left a comment

SgtPooki Aug 11, 2023

SgtPooki Aug 11, 2023

SgtPooki Aug 15, 2023

SgtPooki Aug 11, 2023

SgtPooki Aug 11, 2023

SgtPooki Aug 15, 2023

SgtPooki Aug 11, 2023

SgtPooki Aug 11, 2023

SgtPooki Aug 11, 2023

achingbrain Aug 15, 2023

SgtPooki Aug 15, 2023

SgtPooki Aug 11, 2023

achingbrain commented Aug 15, 2023

maschad left a comment •

edited

Loading

achingbrain commented Aug 15, 2023

maschad commented Aug 15, 2023

SgtPooki Aug 15, 2023

SgtPooki Aug 15, 2023

	* key is a stringified number representing the timestamp of the last time a
	* key is a timestamp representing the last time a

		log.error('dial failed to %s', pendingDial.multiaddrs.map(ma => ma.toString()).join(', '), err)

		if (peerId != null) {

	if (peerId != null) {
	if (peerId != null && priority < someVal) {

feat(libp2p): add autodial retry threshold config option #1943

feat(libp2p): add autodial retry threshold config option #1943

Conversation

achingbrain commented Aug 8, 2023 • edited Loading

maschad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a 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

achingbrain commented Aug 15, 2023

maschad left a comment • edited Loading

Choose a reason for hiding this comment

achingbrain commented Aug 15, 2023

maschad commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain commented Aug 8, 2023 •

edited

Loading

maschad left a comment •

edited

Loading

maschad left a comment •

edited

Loading