Implement Kademlia discovery #501
Conversation
austinabell
requested review from
ansermino,
dutterbutter and
ec2
as code owners
austinabell
requested review from
StaticallyTypedAnxiety,
flodesi,
RajarupanSampanthan and
timvermeulen
|
I don't see where you call |
Yeah, I mean, the way it is already bootstraps finding new peers (gets up to ~50 in 10 seconds on the testnet, should probably also look into limiting this) but this was what I was alluding to making tweaks to match behaviour. I can be more specific and detail an issue to come back to after this PR comes in, or if you have an idea of how this can be periodically polled cleanly I'm open to hearing it. The reason I went without the periodic polling of closest peers is because it didn't seem clear that was necessary or even how go impls have it (from what I could see) also because that logic was being refactored I didn't want to cause unnecessary conflicts. Also the other thing was it wasn't clear in that periodic poll what peer id would be provided to get closest peers to, so didn't want to introduce technical debt by guessing. |
| @@ -982,6 +971,18 @@ where | |||
| pub fn set_state(&mut self, new_state: SyncState) { | |||
| self.state = new_state | |||
| } | |||
|
|
|||
| async fn get_peer(&self) -> PeerId { | |||
| while self.peer_manager.is_empty().await { | |||
There was a problem hiding this comment.
If this can't find a peer, it will loop indefinitely, is it worth it to have a time out mechanism?
There was a problem hiding this comment.
Or maybe implement a try_get_peer() that takes in a timeout variable as well?
There was a problem hiding this comment.
Somewhat intentional, because it can't do anything else until it has a peer to sync with. The logic hasn't changed, I just put in a function because there was another place it should be used.
The logic is definitely a bit broken now after the logic had been refactored since this was setup, but I don't want to cause conflicts with the work Erlich is doing in a bit. This shouldn't be an issue once the syncing setup is improved, but maybe something to keep in mind @ec2
There was a problem hiding this comment.
But yeah, definitely agree a timeout is worthwhile to have, just probably after the setup is refactored (unless you guys think otherwise)
There was a problem hiding this comment.
That is totally fine to me!
|
|
||
| // Subscribe to gossipsub topics with the network name suffix | ||
| for topic in PUBSUB_TOPICS.iter() { | ||
| swarm.subscribe(Topic::new(format!("{}/{}", topic, network_name))); |
There was a problem hiding this comment.
Not needed but could also be a one liner by changing to
PUBSUB_TOPICS.iter().for_each(|topic|swarm.subscribe(Topic::new(format!("{}/{}", topic, network_name))));
There was a problem hiding this comment.
ends up being the same (not one liner) because the subscribe returns a boolean for if it was successful (ignoring because it should always be)
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links