refactor: updated implementation of nodePing

Added `nodePing` command to `NodeConnectionManager` and `NodeManager.nodePing` calls that now. the new `nodePing` command essentially attempts to create a connection to the target node. In this process we authenticate the connection and check that the nodeIds match. If no address is provided it will default to trying to find the node through kademlia.

Related #322

src/nodes/NodeConnectionManager.ts

@@ -515,7 +515,7 @@ class NodeConnectionManager {
       // Check to see if any of these are the target node. At the same time, add
       // them to the shortlist
       for (const [nodeId, nodeData] of foundClosest) {
-        // Ignore any nodes that have been contacted
+        // Ignore a`ny nodes that have been contacted
         if (contacted[nodeId]) {
           continue;
         }
@@ -688,6 +688,42 @@ class NodeConnectionManager {
     );
     return nodeIds;
   }
+
+  /**
+   * Checks if a connection can be made to the target. Returns true if the
+   * connection can be authenticated, it's certificate matches the nodeId and
+   * the addresses match if provided. Otherwise returns false.
+   * @param nodeId - NodeId of the target
+   * @param address - Optional address of the target
+   * @param connConnectTime - Optional timeout for making the connection.
+   */
+  @ready(new nodesErrors.ErrorNodeConnectionManagerNotRunning())
+  public async pingNode(nodeId: NodeId, address?: NodeAddress, connConnectTime?: number): Promise<boolean> {
+    // If we can create a connection then we have punched though the NAT,
+    // authenticated and confimed the nodeId matches
+    let connAndLock: ConnectionAndLock;
+    try {
+      connAndLock = await this.createConnection(nodeId, address, connConnectTime);
+    } catch (e) {
+      if (
+        e instanceof nodesErrors.ErrorNodeConnectionDestroyed ||
+        e instanceof nodesErrors.ErrorNodeConnectionTimeout ||
+        e instanceof grpcErrors.ErrorGRPC ||
+        e instanceof agentErrors.ErrorAgentClientDestroyed
+      ) {
+        // failed to connect, returning false
+        return false;
+      }
+      throw e;
+    }
+    const remoteHost = connAndLock.connection?.host;
+    const remotePort = connAndLock.connection?.port;
+    // if address wasn't set then nothing to check
+    if (address == null) return true;
+    // check if the address information match in case there was an
+    // existing connection
+    return address.host === remoteHost && address.port === remotePort;
+  }
 }
 
 export default NodeConnectionManager;

src/nodes/NodeManager.ts

@@ -54,31 +54,12 @@ class NodeManager {
   /**
    * Determines whether a node in the Polykey network is online.
    * @return true if online, false if offline
+   * @param nodeId - NodeId of the node we're pinging
+   * @param address - Optional Host and Port we want to ping
+   * @param timeout - Optional timeout
    */
-  // FIXME: We shouldn't be trying to find the node just to ping it
-  //  since we are usually pinging it during the find procedure anyway.
-  //  I think we should be providing the address of what we're trying to ping,
-  //  possibly make it an optional parameter?
-  public async pingNode(targetNodeId: NodeId): Promise<boolean> {
-    const targetAddress: NodeAddress =
-      await this.nodeConnectionManager.findNode(targetNodeId);
-    try {
-      // Attempt to open a connection via the forward proxy
-      // i.e. no NodeConnection object created (no need for GRPCClient)
-      await this.nodeConnectionManager.holePunchForward(
-        targetNodeId,
-        await networkUtils.resolveHost(targetAddress.host),
-        targetAddress.port,
-      );
-    } catch (e) {
-      // If the connection request times out, then return false
-      if (e instanceof networkErrors.ErrorConnectionStart) {
-        return false;
-      }
-      // Throw any other error back up the callstack
-      throw e;
-    }
-    return true;
+  public async pingNode(nodeId: NodeId, address?: NodeAddress, timeout?: number): Promise<boolean> {
+    return this.nodeConnectionManager.pingNode(nodeId, address, timeout);
   }
 
   /**

tests/nodes/NodeConnectionManager.lifecycle.test.ts

@@ -1,4 +1,4 @@
-import type { NodeId, NodeIdString, SeedNodes } from '@/nodes/types';
+import type { NodeAddress, NodeId, NodeIdString, SeedNodes } from '@/nodes/types';
 import type { Host, Port } from '@/network/types';
 import fs from 'fs';
 import path from 'path';
@@ -97,12 +97,18 @@ describe(`${NodeConnectionManager.name} lifecycle test`, () => {
     remoteNode1 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode1'),
+      networkConfig: {
+        proxyHost: serverHost
+      },
       logger: logger.getChild('remoteNode1'),
     });
     remoteNodeId1 = remoteNode1.keyManager.getNodeId();
     remoteNode2 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode2'),
+      networkConfig: {
+        proxyHost: serverHost
+      },
       logger: logger.getChild('remoteNode2'),
     });
     remoteNodeId2 = remoteNode2.keyManager.getNodeId();
@@ -521,4 +527,152 @@ describe(`${NodeConnectionManager.name} lifecycle test`, () => {
       await nodeConnectionManager?.stop();
     }
   });
+
+  // new ping tests
+  test('should ping node', async () => {
+    // NodeConnectionManager under test
+    let nodeConnectionManager: NodeConnectionManager | undefined;
+    try {
+      nodeConnectionManager = new NodeConnectionManager({
+        keyManager,
+        nodeGraph,
+        proxy,
+        logger: nodeConnectionManagerLogger,
+      });
+      await nodeConnectionManager.start();
+      // @ts-ignore: kidnap connections
+      const connections = nodeConnectionManager.connections;
+      await expect(nodeConnectionManager.pingNode(remoteNodeId1)).resolves.toBe(true);
+      const finalConnLock = connections.get(
+        remoteNodeId1.toString() as NodeIdString,
+      );
+      // Check entry is in map and lock is released
+      expect(finalConnLock).toBeDefined();
+      expect(finalConnLock?.lock.isLocked()).toBeFalsy();
+    } finally {
+      await nodeConnectionManager?.stop();
+    }
+  });
+  test('should ping node with address', async () => {
+    // NodeConnectionManager under test
+    let nodeConnectionManager: NodeConnectionManager | undefined;
+    try {
+      nodeConnectionManager = new NodeConnectionManager({
+        keyManager,
+        nodeGraph,
+        proxy,
+        logger: nodeConnectionManagerLogger,
+      });
+      await nodeConnectionManager.start();
+      const remoteNodeAddress1: NodeAddress = {
+        host: remoteNode1.proxy.getProxyHost(),
+          port: remoteNode1.proxy.getProxyPort(),
+      }
+      await nodeConnectionManager.pingNode(remoteNodeId1, remoteNodeAddress1);
+    } finally {
+      await nodeConnectionManager?.stop();
+    }
+  });
+  test('should ping node with address when connection exists', async () => {
+    // NodeConnectionManager under test
+    let nodeConnectionManager: NodeConnectionManager | undefined;
+    try {
+      nodeConnectionManager = new NodeConnectionManager({
+        keyManager,
+        nodeGraph,
+        proxy,
+        logger: nodeConnectionManagerLogger,
+      });
+      await nodeConnectionManager.start();
+      const remoteNodeAddress1: NodeAddress = {
+        host: remoteNode1.proxy.getProxyHost(),
+          port: remoteNode1.proxy.getProxyPort(),
+      }
+      await nodeConnectionManager.withConnF(remoteNodeId1, nop);
+      await nodeConnectionManager.pingNode(remoteNodeId1, remoteNodeAddress1);
+    } finally {
+      await nodeConnectionManager?.stop();
+    }
+  });
+  test('should fail to ping non existent node', async () => {
+    // NodeConnectionManager under test
+    let nodeConnectionManager: NodeConnectionManager | undefined;
+    try {
+      nodeConnectionManager = new NodeConnectionManager({
+        keyManager,
+        nodeGraph,
+        proxy,
+        logger: nodeConnectionManagerLogger,
+      });
+      await nodeConnectionManager.start();
+
+      // Pinging node
+      expect(await nodeConnectionManager.pingNode(
+        remoteNodeId1,
+        {host: '127.1.2.3' as Host, port: 55555 as Port},
+        1000,
+      )).toEqual(false);
+
+    } finally {
+      await nodeConnectionManager?.stop();
+    }
+  });
+  test('should fail to ping node with wrong address when connection exists', async () => {
+    // NodeConnectionManager under test
+    let nodeConnectionManager: NodeConnectionManager | undefined;
+    try {
+      nodeConnectionManager = new NodeConnectionManager({
+        keyManager,
+        nodeGraph,
+        proxy,
+        logger: nodeConnectionManagerLogger,
+      });
+      await nodeConnectionManager.start();
+      await nodeConnectionManager.withConnF(remoteNodeId1, nop);
+      expect(await nodeConnectionManager.pingNode(
+        remoteNodeId1,
+        {host: '127.1.2.3' as Host, port: 55555 as Port},
+        1000,
+      )).toEqual(false);
+
+    } finally {
+      await nodeConnectionManager?.stop();
+    }
+  });
+  test('should fail to ping node if NodeId does not match', async () => {
+    // NodeConnectionManager under test
+    let nodeConnectionManager: NodeConnectionManager | undefined;
+    try {
+      nodeConnectionManager = new NodeConnectionManager({
+        keyManager,
+        nodeGraph,
+        proxy,
+        logger: nodeConnectionManagerLogger,
+      });
+      await nodeConnectionManager.start();
+      const remoteNodeAddress1: NodeAddress = {
+        host: remoteNode1.proxy.getProxyHost(),
+        port: remoteNode1.proxy.getProxyPort(),
+      }
+      const remoteNodeAddress2: NodeAddress = {
+        host: remoteNode2.proxy.getProxyHost(),
+        port: remoteNode2.proxy.getProxyPort(),
+      }
+
+      expect(await nodeConnectionManager.pingNode(
+        remoteNodeId1,
+        remoteNodeAddress2,
+        1000,
+      )).toEqual(false);
+
+      expect(await nodeConnectionManager.pingNode(
+        remoteNodeId2,
+        remoteNodeAddress1,
+        1000,
+      )).toEqual(false);
+
+    } finally {
+      await nodeConnectionManager?.stop();
+    }
+  });
 });