-
Notifications
You must be signed in to change notification settings - Fork 939
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
add peer-discovery example with identify & kademlia #2457
base: master
Are you sure you want to change the base?
add peer-discovery example with identify & kademlia #2457
Conversation
I'm not sure if I should also run gossipsub.add_explicit_peer after kademlia RoutingUpdated events. I'd think that my kademlia.add_address calls in my IdentifyEvent implementation could cause peers to be added to the routing table, leading to peers to be known, without gossipsub incorporating them. But for some reason everything seems to work correctly as is, without doing this. Is there some sort of automatic peer sharing going on between kademlia & gossipsub or do i not properly understand what's going on in my own example 😅 |
Hmm, it seems like I don't need my entire Kademlia event implementation with it's calls to add_explicit_peer. That makes me wonder when I would actually need to call add_explicit_peer for gossipsub? Is that only necessary when receiving new peer information outside of libp2p? |
Similarly, calling identify.push on the boot node peer_id also seems to be unecessary, does kademlia automatically make use of the identify protocol when identify is present? |
As mentioned elsewhere, still worth repeating, this is much appreciated. I wonder whether we should introduce another example, or use the chance to consolidate a couple of the existing ones. As a newcomer to the project @Frederik-Baetens was the amount of different examples helpful or rather confusing? As a first thought I would consider consolidating
Instead of introducing a new example showcasing the interworking of
I don't think you need to call rust-libp2p/protocols/gossipsub/src/behaviour.rs Line 3058 in 13ded7f
I am not sure I follow. Identify's push feature allows one to push local address updates to remote nodes. Is this needed here? |
In my experience, adapting floodsub to gossipsub isn't that hard, so it might be okay to only have a gossipsub example. Gossipsub seems like the more useful protocol out of the two anyways. I am happy that there was an example showing how to drive the swarm in tokio, but if the ping example shows that properly, removing chat-tokio would be okay. I think having a separate ping-tokio is definitly convenient, since I found it convenient that the chat-tokio example was explicitly marked with tokio. The example in this PR would further remove the need for the chat-tokio example with event_process=true. I wouldn't go too wild with removing examples though, because a variety of simple examples didn't really bother me as a newcomer. Some seemingly similar examples still highlight more nuanced differences like using a gossipsub behavior directly compared to using TLDR: I'd remove chat-tokio if this gets merged, but I'd think about keeping both chat and gossipsub-chat b/c of the slight differences in how they construct their behaviours.
I initially thought that i needed to push a new node's identity to existing boot nodes, in order for the boot nodes to actually obtain the multiaddr of the new node. However, it seems like the boot node automatically "asks" for any new node connects through kademlia. I just tested removing the "dial" as well, and that seems to work fine still. Is it safe to remove it here?
It would get the information across, but when starting out, I found the size of the larger examples a bit daunting. If we added peer discovery to the distributed-key-value store, it wouldn't be all that clear to a newcomer which parts of that example are actually required for peer discovery, thereby making it harder for them to extract just the part responsible for peer discovery, and to combine it with the protocol they actually need. That same criticism somewhat applies to the example in this pull request, but I think that the gossipsub protocol is an easy way to test that everything still works after the user shuts down the initial boot node. And in this example, it's much easier to identify & extract the parts responsible for peer discovery since the kademlia & identify protocols are exclusively used for peer discovery. But if desired, removing the gossipsub part and just keeping kademlia & identify would also be okay, since that still keeps things simple and easy to understand. |
Setting this to draft until we proceed. |
This pull request has merge conflicts. Could you please resolve them @Frederik-Baetens? 🙏 |
This builds upon a mix of the chat-tokio and gossipsub chat examples and adds in peer discovery with kademlia & identify.
The additional value of this example lies in further showcasing the identify protocol, and in showcasing how to run a private kademlia network. It makes the other chat examples more functional by being resistant to random peer failure.
Thanks a lot to @mxinden for answering my questions, your answers were invaluable for my own project, and for creating this example.
Depends on #2456 to be able to compile.