-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: start kad dht random walk #251
Conversation
src/index.js
Outdated
this._dht.start(() => { | ||
this._dht.randomWalk.start() | ||
cb() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make sure to stop it. Add tests for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree in the part of loosing the plugability of using a completelly different DHT! Let's wait for @diasdavid feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
-
As @jacobheun said, the
random-walk
implementation needs to be refactored, in order to have the same behavior as the otherpeer-discovery
modules (js-libp2p-mdns
andjs-libp2p-bootstrap
), that is, extend theEventEmitter
, have the appropriate constructor and functions and have a properTAG
. 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 otherpeer-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 injs-libp2p-kad-dht
to refactor itspeer-discovery
according to the others and an Issue ininterface-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. -
After having the
randomWalk
with a compatible implementation, should this be enabled through thelibp2p
users, as for exampleIPFS
, or should this be enabled if the DHT is enabled in thelibp2p
? Currently,libp2p
users set thepeer-discovery
modules that they want to use, instead of having them set in here. So, therandomwalk
should probably be enabled from outside as well, but it depends on having theDHT
instance. However, I feel that when the DHT is enabled,random-walk
should be also enabled onlibp2p
, as otherwise we would need toconnect
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
60b1726
to
b06f6f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good :)
I am having problems with the tests for The same tests are failing in master. |
@vasco-santos I am working on that now, there's a resiliency issue with the latest libp2p-mplex. I'll have a PR shortly |
b06f6f0
to
68d7897
Compare
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: