feat: compatibility with go-libp2p-mdns

alanshaw commented 5 years ago

This PR adds a compatibility class that allows a js-libp2p node to find a go-libp2p node (and vice versa) over MDNS. I do not know when go-libp2p plans to land the long awaited new MDNS implementation but until then this will allow interop between js and go.

It's implemented as a separate class so the two differing implementations do not get confused.

Fun fact, it uses 2 dgram servers. 1 is the multicast listener, which simply responds to queries. The other is a dgram server that exists for 5 seconds at a time, sends a multicast query, waits for unicast responses, stops and then starts up again on a different random port.

I've verified this is working correctly by running a go-ipfs and js-ipfs node with no boostrap nodes (and no other discovery methods) and verifying they find each other.

TODO:

[x] Add tests!

Some tips if you want to try this out:

After you've run ipfs init, remember to remove the bootstrap nodes from the config file (~/.ipfs/config) of each node before you start up
Use ipfs log level mdns debug for some go-ipfs mdns logs
You can use the following script (after npm linking this branch) to start a js-ipfs node with no bootstrap nodes and no discovery modules other than MDNS:

const IPFS = require('ipfs')
const MDNS = require('libp2p-mdns')
const TCP = require('libp2p-tcp')

const ipfs = new IPFS({
  repo: '/tmp/ipfs-mdns',
  config: {
    Bootstrap: []
  },
  libp2p: {
    modules: {
      peerDiscovery: [MDNS],
      transport: [TCP]
    }
  }
})

ipfs.on('ready', async () => {
  console.log('ipfs is ready')
  console.log('My Peer ID:', (await ipfs.id()).id)
  setInterval(async () => {
    const peers = await ipfs.swarm.peers()
    console.log(peers.length, 'peers:')
    peers.forEach(p => console.log(p.peer.toB58String()))
  }, 10000)
})

alanshaw commented 5 years ago

What we can also do is changing this PR's codebase (compat) to use async await from the beginning, which would ease the other PR to get merged afterward. What do you guys think?

I don't think we should mix the two styles in the code base:

IMHO we should only mix promises and callbacks when we have to deal with an external library we have no control over. We already have async as a dependency so we'd not be saving any bundle bytes by using builtins. I think it sends the wrong message to contributors. We have a awesome endeavour where we're trying to coordinate all of this at the same time and adding Promises here piecemeal is problematic for a number of reasons:

It's super inconsistent with the rest of the libp2p code (everything is callbacks, not even a dual promise/callback api)

Sets the precedent for other contributors to do the same thing, when actually we want contributions to either focus on fixing problems or focus on the endeavour

Finally, without going into specifics, mixing callbacks and Promises is fraught with double callback danger, perhaps not in this specific case, but it can be super hard to debug when it happens!

from https://github.com/libp2p/js-libp2p-switch/pull/310#discussion_r267298611

So either we merge as is or rebase off of the async/await branch.

jacobheun commented 5 years ago

My main concern here relates to https://github.com/libp2p/js-libp2p-mdns/pull/71, in regards to polling mdns. Currently this is configured to run the go query every 5 seconds by default. The normal query is set to run every 10s by default. Running 3 queries every 10 seconds is abusive to mdns and devices using it.

I think this might be a good opportunity for us to look at disabling polling by default, and allowing an interval to be set, instead of defaulting to polling. As new devices join, we should discover them when they broadcast.

alanshaw commented 5 years ago

My main concern here relates to #71, in regards to polling mdns. Currently this is configured to run the go query every 5 seconds by default. The normal query is set to run every 10s by default. Running 3 queries every 10 seconds is abusive to mdns and devices using it.

Ok, there are some options:

We replace the current implementation with the go compatible implementation
- (+) Smaller code base, compatible with go-ipfs
- (-) We poll every 5 seconds (but we could just increase that to 10s so we're no worse than we are already)
- (-) We break MDNS discovery of older js-ipfs nodes running on the network (although this is already broken for other reasons https://github.com/libp2p/js-libp2p-switch/issues/326)
We turn off polling (but not responding) for Go MDNS compat
- (+) Not introducing more MDNS polling
- (+) go-ipfs nodes still poll so we'll send responses and go-ipfs nodes will probably connect to us (i.e. go-ipfs can discover js-ipfs but not the other way round)
- (-) If go-ipfs nodes stop polling they'll never discover js-ipfs nodes

I think this might be a good opportunity for us to look at disabling polling by default, and allowing an interval to be set, instead of defaulting to polling. As new devices join, we should discover them when they broadcast.

Simply sending a multicast go-ipfs compatible "response" on an interval might work because I think go-ipfs accepts responses multicast as well as unicast. It's similar to (2) above, except that go-ipfs nodes will discover us when we send a response in an interval, not as a response to their query.

I'll check it out and report back.

alanshaw commented 5 years ago

It works! https://github.com/libp2p/js-libp2p-mdns/pull/81

jacobheun commented 5 years ago

So, #81 doesn't do a query at all which is why we won't be able to get go nodes. It should still do a query, it just shouldn't need to be run on an interval.

Anytime a node starts, it should listen for MDNS queries and then immediately query. That same query will be received by the started node, which will respond to it. So we've discovered any existing peers, and told them about ourselves.

If we start another node 10 minutes later and it does the same thing. The first node should be made aware of the new node, and vice versa.

For every node we have, we should only need that many queries, as long as everyone responds. There is the problem of sleeping nodes or network changes, but since our discovery doesn't work all that consistently right now, I think that could be addressed in a followup release or temporarily just increasing the interval.

alanshaw commented 5 years ago

@jacobheun I've altered this PR to increase the interval between MDNS queries to 1 min.

When GoMulticastDNS starts it creates 2 MDNS servers:

A Responder which listens for multicast queries and responds DIRECTLY to them i.e. a unicast response
A Querier which listens on a RANDOM port for UNICAST responses. It immediately sends a multicast query and waits for 5s for responses. After 5s have elapsed it shuts down and repeats after 1 minute

This follows the implementation of go-libp2p-mdns except in go the interval is 0 seconds not 1 minute.

The JS implementation is untouched.

Can you please confirm that's cool and I'll get on with adding tests.

jacobheun commented 5 years ago

@alanshaw that sounds reasonable to me.

I'll look at trying to get https://github.com/libp2p/specs/pull/80 at least moved into Draft so those proper implementations can get unblocked. I believe rust-libp2p is already using that spec.

libp2p / js-libp2p-mdns

feat: compatibility with go-libp2p-mdns #80