fix: start kad dht random walk

[vasco-santos]

Currently the DHT's randomWalk was not being started. Accordingly, a peer did not find other peers in the network unless we connect them manually.

Needs:

• js-libp2p-kad-dht#42

[daviddias]

Also make sure to stop it. Add tests for this.

[pgte]

Probably not the place for this, but:
I find it weird that we have to call something that is so kademlia-specific. Other DHT solutions may not have this concept of a random walk. IMO, _dht.start() should be enough for the DHT to do all the necessary things for it to work..

[vasco-santos]

@pgte at first I was starting it into the libp2p-kad-dht. However, after @diasdavid 's feedback and reading the docs, I added it here, as it should probably be started with the other Peer Discovery Service.
More context also in the js-libp2p-kad-dht#42

[vasco-santos]

But I agree in the part of loosing the plugability of using a completelly different DHT! Let's wait for @diasdavid feedback

[jacobheun]

If the intention is for random walk to behave as a peer discovery service, so that users can choose to add it or not, it should be passed in via the peerDiscovery array configuration for libp2p. That way it can be started/stopped with all other discovery services.

If it's core to the dht working properly, it should be started when the dht is (inside the dht).

[jacobheun]

The DHT logic will likely need to get refactored so that, like transports, users can provide an instance or a function. This way if they want to enable the discovery service they'd create the DHT instance and pass dht.discovery, or similar, to the peerDiscovery array.

[vasco-santos]

So, after your opinions, I agree that this should be started here like any other peer discovery service.

So, regarding this issue, there are two things that need to be considered.

1. As @jacobheun said, the random-walk implementation needs to be refactored, in order to have the same behavior as the other peer-discovery modules (js-libp2p-mdns and js-libp2p-bootstrap), that is, extend the EventEmitter, have the appropriate constructor and functions and have a proper TAG. After this, it should use the libp2p/interface-peer-discovery as a test suite to ensure compatibility. Currently, the DHT implementation js-libp2p-kad-dht#peer-discovery has the badge representing that it fulfills the requirements of libp2p/interface-peer-discovery, but it does not.
   Moreover, none of the other peer-discovery module run those interface tests (libp2p/interface-peer-discovery), and they seem to not be implemented for now. All things considered, I will create an Issue in js-libp2p-kad-dht to refactor its peer-discovery according to the others and an Issue in interface-peer-discovery to implement its tests and have them running on the modules that claim to have compatibility. This way, I can come back to this later.

2. After having the randomWalk with a compatible implementation, should this be enabled through the libp2p users, as for example IPFS, or should this be enabled if the DHT is enabled in the libp2p? Currently, libp2p users set the peer-discovery modules that they want to use, instead of having them set in here. So, the randomwalk should probably be enabled from outside as well, but it depends on having the DHT instance. However, I feel that when the DHT is enabled, random-walk should be also enabled on libp2p, as otherwise we would need to connect manually through a chain of peers, in order to be able to query the all network. Maybe I should create an issue for this, in order to have a more structured discussion.

So, while those discussions happen, and considering that the interface-peer-discovery tests need to be implemented and the random-walk refactored, would we be able to move forward with an initial version enabling with the DHT, as it is right now? This way, I can continue working on having the DHT Integration on IPFS - Awesome Endeavour: DHT Part II and in its interop tests? If so, can you review the kad-dht blocking PR?

[jacobheun]

I think we should wait until that is configurable to add it in. Having it autostart now and then change in the future to needing to be added via peerDiscovery configuration could end up being confusing to users. It would be great for users to start being able to see an increase in connected peers, but releasing an update later that breaks this would probably result in some frustration. Since it's not currently running by default, I would recommend we take the extra time to get if configurable first.

[vasco-santos]

I understand your point @jacobheun . But I am still not convinced that in the libp2p perspective we will want to have the random-walk disabled by default. Otherwise, we would need to connect manually through a chain of peers, in order to be able to query the all network, which seems strange to me.

However, I can start looking at its modification in the meanwhile.

[vasco-santos]

After discussing this offline with @jacobheun we believe that the best way is to allow the configuration of the randomWalk, but turn it on by default. That way restricted nodes will be able to turn it off.

At first, we tried to understand how we could have randomWalk the same way as the other peer-discovery work from the interface perspective, but the other discovery services are more targeted. In the randomWalk, we will not find a specific peer, but we will ask everyone we know, and everyone they now, if they know a random peer, which is a way of meeting the others, but it is really tight with the DHT behavior.

We ended up thinking, how can we understand that a new peer we just met was a result of the randomWalk, or any other event, since the base codebase for this is shared between randomWalk and the DHT itself. All things considered, a considerable effort would need to be done, in order to get an interface that we can be sure to have an exactly equal behavior to what we expect, as the other peer-discovery modules. Accordingly, should we announce every single peer that we find during the walk? or should we announce only the random one, if we find it? If we go through the second one, we will end up having a different behaviour compared to the other peer-discovery modules.

Considering js-libp2p#242/files#diff-b0f6b211af5c98be53893311b36c8d1fR9, the peer/content routing are separated from the DHT by design, the randomWalk in peerDiscovery do not seem that necessary.

Thus, we think it is better to go by simply enabling by default the randomWalk and allow an easy way through libp2p to disable it.

I haven't reviewed the latest changes.

[daviddias]

All good :)

[vasco-santos]

I am having problems with the tests for websockets stress tests (browser).

The same tests are failing in master.

[jacobheun]

@vasco-santos I am working on that now, there's a resiliency issue with the latest libp2p-mplex. I'll have a PR shortly