Skip to content
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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Frederik-Baetens
Copy link
Contributor

@Frederik-Baetens Frederik-Baetens commented Jan 29, 2022

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.

@Frederik-Baetens
Copy link
Contributor Author

Frederik-Baetens commented Jan 29, 2022

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 😅

@Frederik-Baetens
Copy link
Contributor Author

Frederik-Baetens commented Jan 29, 2022

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?

@Frederik-Baetens
Copy link
Contributor Author

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?

@mxinden
Copy link
Member

mxinden commented Feb 1, 2022

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 chat.rs and gossipsub-chat.rs into one. I don't think there is much need to showcase libp2p-floodsub, thus I would suggest a single chat.rs example using libp2p-gossipsub.

chat-tokio.rs seems like a rather large example to "only" showcase how to use tokio instead of async-std. My suggestion would be to remove chat-tokio.rs and instead have a ping-tokio.rs in addition to the default ping.rs as that would make a lot simpler example (both for newcomers and for maintainers).

Instead of introducing a new example showcasing the interworking of libp2p-gossipsub libp2p-kadandlibp2p-identify, how about extending the distributed-key-value-store.rsexample withlibp2p-identify`? Would that as well get the information across?


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?

I don't think you need to call GossipSub::add_explicit_peer at all. Gossipsub is notified when a new connection comes in and thus learns of new peers.

fn inject_connected(&mut self, peer_id: &PeerId) {


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?

I am not sure I follow. Identify's push feature allows one to push local address updates to remote nodes. Is this needed here?

@Frederik-Baetens
Copy link
Contributor Author

Frederik-Baetens commented Feb 1, 2022

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 #[behaviour(event_process = true)] or #[behaviour(out_event = "OutEvent")]. Same thing with some using a development transport, and others using an encrypted tcp transport.

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 am not sure I follow. Identify's push feature allows one to push local address updates to remote nodes. Is this needed here?

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?

Instead of introducing a new example showcasing the interworking of libp2p-gossipsub libp2p-kad and libp2p-identify, how about extending the distributed-key-value-store.rs example with libp2p-identify? Would that as well get the information across?

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.

@thomaseizinger
Copy link
Contributor

Setting this to draft until we proceed.

@thomaseizinger thomaseizinger marked this pull request as draft November 2, 2022 04:05
@mergify
Copy link

mergify bot commented Mar 22, 2023

This pull request has merge conflicts. Could you please resolve them @Frederik-Baetens? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants