Local Polykey Node Discovery

[amydevs]

Description

This PR integrates MDNS into the NodeConnectionManager to allow for the discovery of nodes on the local network, as well as disable holepunching for connections to local network addresses.

This PR should only focus on the Step 1 of #537 spec, as modifying the node graph to accept multiple IP addresses will require ACL changes that default disable the sharing of local addresses with seed nodes, as well as expansion to support multiple IP addresses.

This PR will encapsulate MDNS in the NodeConnectionManager, as well as make methods such as getConnection to be able to query MDNS for known hosts through the newly added findNodesAll and findNodesLocal methods. findNodesAll is a method that calls both the original findNode and findNodesLocal to aggregate a list of all NodeAddresses. This is notably only used in connection-related methods rather than those related to Kademlia (NodeGraph).

When the NodeConnectionManager is started, MDNS will be started, with a Polykey service registered, and a query for Polykey services started.

When the NodeConnectionManager is stopped, MDNS will be stopped alongside all pending queries, and deregistering all registered services.

Current scopes: external and local.

Issues Related

• Related MDNS integration and NodeGraph structure expansion #537

Tasks

• 1. Integrate MDNS into NodeConnectionManager
• 2. Separate MDNS and Kademlia Behaviour
• 3. Update NodeConnectionManager Methods to Support Multiple IP Addresses
  • getConnectionWithAddress
  • getConnection
• 4. Select a well-known port and multicast address for Polykey usage.
• 5. Modify Network Domain to Support Link-Local Addresses with a Interface ID Suffix.
• 6. Creating of connections should happen in stages, where local addresses should be tried for first before external addresses

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[amydevs]

When a Polykey agent has it's host bound to ::0 and is trying to connect to a node at some ipv6 address, it is failing due to the formatting of my MDNS supplied ipv6 addresses.

When i encapsulate my IPv6 addresses in brackets (i.e [::1]), they are not recognized as hosts by resolveHostnames in src/network/utils.ts of (calls isHost to check if the provided address is a valid host). resolveHostnames will hence return no addresses at all, and will cause no QUICConnections to be made.

When i don't encapsulate my IPv6 addresses in brackets, they are recognized as hosts by resolveHostnames in src/network/utils.ts. However, the std::net IPv6Addr parse function fails only on link-local IPv6 addresses that aren't encapsulated in brackets (i.e fe80::260:8ff:fe52:f9d8) fail to be parsed as ipv6 addresses.

So, we're going to be ignoring ipv6 for now regarding MDNS.

See #584 (comment)

[ghost]

👇 Click on the image for a new way to code review

Legend

[CMCDragonkai]

If it only fixes a portion of #537, don't say Fixes #537, say Related #537 in the OP.

[CMCDragonkai]

Is the bracket encapsulated format the correct format?

If so, just align on that, and then just update isHost to make it work.

[CMCDragonkai]

QUIC definitely supports ipv6. Try testing with non-link local IPv6 addresses first.

[CMCDragonkai]

The square braces should only come into play with URIs, generally speaking none of internal network code should be using square braced hosts. So it should not be expected to work.

[CMCDragonkai]

The only time we use square braces is on input and output, like for example the seed node configuration.

[CMCDragonkai]

Can you check if maybe a scoped ipv6 address is needed, and if so, perhaps that needs to be passed all the way down. In that case the isHost or isIPv6 check needs to support scoped ipv6 address.

[amydevs]

Can you check if maybe a scoped ipv6 address is needed, and if so, perhaps that needs to be passed all the way down. In that case the isHost or isIPv6 check needs to support scoped ipv6 address.

it seems that ip-num does not correctly parse nor validate ipv6 link-local addresses, this will have to be addressed

[amydevs]

it turns out there were two issues with MDNS integration that showed up on attempting to connect to a node via ipv6.

1. Link-local ip addresses will fail to be parsed by std::net Rust library if they do not contain the network interface name or scope id.
2. Promise.all([Promise.race(connProms)]) was causing getMultiConnection to fail where there is more than one address provided. In the case of MDNS discovery, there were more than one address discovered on my machine due IPv4 + IPv6 interfaces.

[CMCDragonkai]

it turns out there were two issues with MDNS integration that showed up on attempting to connect to a node via ipv6.

1. Link-local ip addresses will fail to be parsed by std::net Rust library if they do not contain the network interface name or scope id.
2. Promise.all([Promise.race(connProms)]) was causing getMultiConnection to fail where there is more than one address provided. In the case of MDNS discovery, there were more than one address discovered on my machine due IPv4 + IPv6 interfaces.

See #585, @tegefaulkes any fixes relating to multi-connection?

So how are you working around ip-num support?

[CMCDragonkai]

Can you explain the result of this PR? What changes to the behaviour to agent startup? You should expand the spec, since this is a rather large major feature.

[tegefaulkes]

I'll do a proper review tomorrow.

[amydevs]

it turns out there were two issues with MDNS integration that showed up on attempting to connect to a node via ipv6.

1. Link-local ip addresses will fail to be parsed by std::net Rust library if they do not contain the network interface name or scope id.
2. Promise.all([Promise.race(connProms)]) was causing getMultiConnection to fail where there is more than one address provided. In the case of MDNS discovery, there were more than one address discovered on my machine due IPv4 + IPv6 interfaces.

See #585, @tegefaulkes any fixes relating to multi-connection?

So how are you working around ip-num support?

i copied over the fixes yeah

[CMCDragonkai]

You can rebase now, since it has been merged to staging.

[CMCDragonkai]

Are all the tasks there still todo, and are there more tasks to do? When can @tegefaulkes review this?

[amydevs]

Are all the tasks there still todo, and are there more tasks to do? When can @tegefaulkes review this?

I think review should be ready, although I have to make the MDNS tests to be more concise, and haven't tested it in a local network over multiple devices yet.

[CMCDragonkai]

Are you going to tick off the tasks then?

[CMCDragonkai]

Can you elaborate on the current problem atm.

[CMCDragonkai]

MatrixAI/js-mdns#28

[amydevs]

I need to have some empirical results on what you mean by "slow". @amydevs

i mean iterating over every address is a bit inefficient, especially because i have to do it twice, once when turning the queried MDNS services into NodeAddresses, and the second time here

[amydevs]

The latest commit makes it so that starting a connection (getConnectionWithAddresses) will first try the local addresses, and then the external addresses. This is currently slow and requires an O(n) operation where n is the number of addresses. A solution would be to be able to pass in multiple arrays of addresses for each address scope. if this currently isn't a concern, commit can be dropped.

const addressGroups: { local: Array<NodeAddress>, external: Array<NodeAddress> } = { local: [], external: [] };
    for (const address of addresses) {
      if (address.scopes.includes('local')) {
        addressGroups.local.push(address);
      }
      else {
        addressGroups.external.push(address);
      }
    }
    let timeoutDivisions = 0;
    for (const addressGroup of Object.values(addressGroups)) {
      if (addressGroup.length !== 0) {
        timeoutDivisions++;
      }
    }
    this.logger.debug(`Getting NodeConnection for ${targetNodeIdEncoded}`);
    return await this.connectionLocks
      .withF([targetNodeIdString, Lock, ctx], async () => {
        this.logger.debug(`acquired lock for ${targetNodeIdEncoded}`);
        // Attempting a multi-connection for the target node
        const timeout = ctx.timer.getTimeout() / timeoutDivisions;
        for (const addressGroup of Object.values(addressGroups)) {
          if (addressGroup.length === 0) {
            continue;
          }
          let results = await this.establishMultiConnection(
            [targetNodeId],
            addressGroup,
            {
              signal: ctx.signal,
              timer: timeout
            },
          );
          // Should be a single result.
          for (const [, connAndTimer] of results) {
            return connAndTimer;
          }
        }
        // Should throw before reaching here
        utils.never();
      })

I'm thinking you should do something like this?

* Try all local

* Try all global

That way local set is prioritised first.

Timeout wise, it would be shared between the 2... it might not be a problem, but maybe it needs to increase. Needs empirical test, tell me what the result will be.

let timeoutDivisions = 0;
    const addressGroups: { local: Array<NodeAddress>, external: Array<NodeAddress> } = { local: [], external: [] };
    for (const address of addresses) {
      let scope = address.scopes.includes('local') ? 'local' : 'external';
      // If this is the first time an addressGroup has had an address added, the timeout divisions must be incremented.
      if (addressGroups[scope].length === 0) {
        timeoutDivisions++;
      }
      addressGroups[scope].push(address);
    }
    this.logger.debug(`Getting NodeConnection for ${targetNodeIdEncoded}`);
    return await this.connectionLocks
      .withF([targetNodeIdString, Lock, ctx], async () => {
        this.logger.debug(`acquired lock for ${targetNodeIdEncoded}`);
        // Attempting a multi-connection for the target node using local addresses
        const timeout = ctx.timer.getTimeout() / timeoutDivisions;
        let results: Map<NodeIdString, ConnectionAndTimer> | undefined;
        if (addressGroups.local.length !== 0) {
          results = await this.establishMultiConnection(
            [targetNodeId],
            addressGroups.local,
            {
              signal: ctx.signal,
              timer: timeout
            },
          );
        }
        // If there are no results from the attempted local connections, attempt a multi-connection for the target node using external addresses
        if (results == null || results.size === 0) {
          results = await this.establishMultiConnection(
            [targetNodeId],
            addressGroups.external,
            {
              signal: ctx.signal,
              timer: timeout
            },
          );
        }
        // Should be a single result.
        for (const [, connAndTimer] of results) {
          return connAndTimer;
        }
        // Should throw before reaching here
        utils.never();
      })

i've fixed this

[tegefaulkes]

Some small things but otherwise seems fine.

Don't forget to lint, I saw an unused import somewhere and linting should catch that.

If all the tests pass then you can merge.

[CMCDragonkai]

I need a better spec for scopes. What is meaning and consequences of it?

What is external? What is local? Why is there no "global"? What can this be expanded to? Copy content to #537.

[amydevs]

I need a better spec for scopes. What is meaning and consequences of it?

What is external? What is local? Why is there no "global"? What can this be expanded to? Copy content to #537.

I already expanded on it in the spec before:

#537