From 785df528fe6e5010a61391994e222028dbc4e4c5 Mon Sep 17 00:00:00 2001
From: Danish Arora <35004822+danisharora099@users.noreply.github.com>
Date: Wed, 2 Aug 2023 13:49:48 +0530
Subject: [PATCH] fix: improve connection manager error handling + edge cases
 (#1450)

* increase TTL on discovery classes

* refactor dialPeer to handle edge cases

* address comment
---
 packages/core/src/lib/connection_manager.ts   | 111 ++++++++++--------
 packages/dns-discovery/src/index.ts           |   2 +-
 .../src/waku_peer_exchange_discovery.ts       |   2 +-
 3 files changed, 61 insertions(+), 54 deletions(-)

diff --git a/packages/core/src/lib/connection_manager.ts b/packages/core/src/lib/connection_manager.ts
index 3770a95cef..6ca9f922a6 100644
--- a/packages/core/src/lib/connection_manager.ts
+++ b/packages/core/src/lib/connection_manager.ts
@@ -174,51 +174,67 @@ export class ConnectionManager
   private async dialPeer(peerId: PeerId): Promise<void> {
     this.currentActiveDialCount += 1;
     let dialAttempt = 0;
-    while (dialAttempt <= this.options.maxDialAttemptsForPeer) {
+    while (dialAttempt < this.options.maxDialAttemptsForPeer) {
       try {
-        log(`Dialing peer ${peerId.toString()}`);
+        log(`Dialing peer ${peerId.toString()} on attempt ${dialAttempt + 1}`);
         await this.libp2p.dial(peerId);
 
         const tags = await this.getTagNamesForPeer(peerId);
         // add tag to connection describing discovery mechanism
         // don't add duplicate tags
-        this.libp2p
-          .getConnections(peerId)
-          .forEach(
-            (conn) => (conn.tags = Array.from(new Set([...conn.tags, ...tags])))
-          );
+        this.libp2p.getConnections(peerId).forEach((conn) => {
+          conn.tags = Array.from(new Set([...conn.tags, ...tags]));
+        });
 
         this.dialAttemptsForPeer.delete(peerId.toString());
-        return;
-      } catch (e) {
-        const error = e as AggregateError;
-
-        this.dialErrorsForPeer.set(peerId.toString(), error);
-        log(`Error dialing peer ${peerId.toString()} - ${error.errors}`);
-
-        dialAttempt = this.dialAttemptsForPeer.get(peerId.toString()) ?? 1;
-        this.dialAttemptsForPeer.set(peerId.toString(), dialAttempt + 1);
-
-        if (dialAttempt <= this.options.maxDialAttemptsForPeer) {
-          log(`Reattempting dial (${dialAttempt})`);
+        // Dialing succeeded, break the loop
+        break;
+      } catch (error) {
+        if (error instanceof AggregateError) {
+          // Handle AggregateError
+          log(`Error dialing peer ${peerId.toString()} - ${error.errors}`);
+        } else {
+          // Handle generic error
+          log(
+            `Error dialing peer ${peerId.toString()} - ${
+              (error as any).message
+            }`
+          );
         }
+        this.dialErrorsForPeer.set(peerId.toString(), error);
+
+        dialAttempt++;
+        this.dialAttemptsForPeer.set(peerId.toString(), dialAttempt);
       }
     }
 
-    try {
-      log(
-        `Deleting undialable peer ${peerId.toString()} from peer store. Error: ${JSON.stringify(
-          this.dialErrorsForPeer.get(peerId.toString()).errors[0]
-        )}
-        }`
-      );
-      this.dialErrorsForPeer.delete(peerId.toString());
-      return await this.libp2p.peerStore.delete(peerId);
-    } catch (error) {
-      throw `Error deleting undialable peer ${peerId.toString()} from peer store - ${error}`;
-    } finally {
-      this.currentActiveDialCount -= 1;
-      this.processDialQueue();
+    // Always decrease the active dial count and process the dial queue
+    this.currentActiveDialCount--;
+    this.processDialQueue();
+
+    // If max dial attempts reached and dialing failed, delete the peer
+    if (dialAttempt === this.options.maxDialAttemptsForPeer) {
+      try {
+        const error = this.dialErrorsForPeer.get(peerId.toString());
+
+        let errorMessage;
+        if (error instanceof AggregateError) {
+          errorMessage = JSON.stringify(error.errors[0]);
+        } else {
+          errorMessage = error.message;
+        }
+
+        log(
+          `Deleting undialable peer ${peerId.toString()} from peer store. Error: ${errorMessage}`
+        );
+
+        this.dialErrorsForPeer.delete(peerId.toString());
+        await this.libp2p.peerStore.delete(peerId);
+      } catch (error) {
+        throw new Error(
+          `Error deleting undialable peer ${peerId.toString()} from peer store - ${error}`
+        );
+      }
     }
   }
 
@@ -302,25 +318,16 @@ export class ConnectionManager
           Tags.BOOTSTRAP
         );
 
-        if (isBootstrap) {
-          this.dispatchEvent(
-            new CustomEvent<PeerId>(
-              EPeersByDiscoveryEvents.PEER_DISCOVERY_BOOTSTRAP,
-              {
-                detail: peerId,
-              }
-            )
-          );
-        } else {
-          this.dispatchEvent(
-            new CustomEvent<PeerId>(
-              EPeersByDiscoveryEvents.PEER_DISCOVERY_PEER_EXCHANGE,
-              {
-                detail: peerId,
-              }
-            )
-          );
-        }
+        this.dispatchEvent(
+          new CustomEvent<PeerId>(
+            isBootstrap
+              ? EPeersByDiscoveryEvents.PEER_DISCOVERY_BOOTSTRAP
+              : EPeersByDiscoveryEvents.PEER_DISCOVERY_PEER_EXCHANGE,
+            {
+              detail: peerId,
+            }
+          )
+        );
 
         try {
           await this.attemptDial(peerId);
diff --git a/packages/dns-discovery/src/index.ts b/packages/dns-discovery/src/index.ts
index e9f94956f8..9cf8f334d2 100644
--- a/packages/dns-discovery/src/index.ts
+++ b/packages/dns-discovery/src/index.ts
@@ -22,7 +22,7 @@ const enrTree = {
 
 const DEFAULT_BOOTSTRAP_TAG_NAME = "bootstrap";
 const DEFAULT_BOOTSTRAP_TAG_VALUE = 50;
-const DEFAULT_BOOTSTRAP_TAG_TTL = 120000;
+const DEFAULT_BOOTSTRAP_TAG_TTL = 100_000_000;
 
 export interface DnsDiscoveryComponents {
   peerStore: PeerStore;
diff --git a/packages/peer-exchange/src/waku_peer_exchange_discovery.ts b/packages/peer-exchange/src/waku_peer_exchange_discovery.ts
index 1c14f6360f..06a22d0c0f 100644
--- a/packages/peer-exchange/src/waku_peer_exchange_discovery.ts
+++ b/packages/peer-exchange/src/waku_peer_exchange_discovery.ts
@@ -47,7 +47,7 @@ export interface Options {
 
 export const DEFAULT_PEER_EXCHANGE_TAG_NAME = Tags.PEER_EXCHANGE;
 const DEFAULT_PEER_EXCHANGE_TAG_VALUE = 50;
-const DEFAULT_PEER_EXCHANGE_TAG_TTL = 120000;
+const DEFAULT_PEER_EXCHANGE_TAG_TTL = 100_000_000;
 
 export class PeerExchangeDiscovery
   extends EventEmitter<PeerDiscoveryEvents>