Implement K-DHT for spartan-farmer
#27
Conversation
1. Update `oneshot` to receive errors. 2. Add `ClientType` enum. 3. Add methods to create bootstrap node.
1. Add `listen_addr` in `ClientConfig` for bootstrap nodes. 2. Add code in `dial_bootstrap` methods. 3. Add more comments.
1. Return Peer ID with `Client` object. 2. Add `enum` for `ClientType`. 3. Add methods to create bootstrap and normal nodes.
1. Add `ClientEvent` and `NetworkEvent` for `Bootstrap`. 2. Add flag for inputting bootstrap nodes through the CLI.
whereistejas
force-pushed
the
impletement-dht2
branch
from
3057b23
to
7e271cd
Compare
|
This code is ready for review. It currently supports bootstrapping but does not yet support peer-discovery. The core methods for peer-discovery are in place, but I need to give more thought about how to setup recurring peer-discovery after specified time interval/delays. For example, in |
| pub enum ClientEvent { | ||
| // Event for adding a listening address. | ||
| Listen { | ||
| addr: Multiaddr, | ||
| sender: oneshot::Sender<OneshotType>, | ||
| }, | ||
| // Kademlia Random Walk for Peer Discovery. (GetClosestPeer) | ||
| #[allow(dead_code)] | ||
| RandomWalk { | ||
| key: Option<PeerId>, | ||
| sender: oneshot::Sender<QueryId>, | ||
| }, | ||
| // List all known peers. | ||
| #[allow(dead_code)] | ||
| KnownPeers { | ||
| sender: oneshot::Sender<Vec<PeerId>>, | ||
| }, | ||
| // Dial another peer. | ||
| #[allow(dead_code)] | ||
| Dial { | ||
| addr: Multiaddr, | ||
| peer: PeerId, | ||
| sender: oneshot::Sender<OneshotType>, | ||
| }, | ||
| // Bootstrap during the initial connection to the DHT. | ||
| // NOTE: All the bootstrap nodes must already be connected to the swarm before we can start the | ||
| // bootstrap process. | ||
| Bootstrap { | ||
| sender: oneshot::Sender<QueryId>, | ||
| }, | ||
| // Get all listening addresses. | ||
| #[allow(dead_code)] | ||
| Listeners { | ||
| sender: oneshot::Sender<Vec<Multiaddr>>, | ||
| }, | ||
| // Read Kademlia Query Result. | ||
| #[allow(dead_code)] | ||
| QueryResult { | ||
| qid: QueryId, | ||
| sender: oneshot::Sender<String>, | ||
| }, | ||
| } |
There was a problem hiding this comment.
It would be much better to remove this and have implementation in methods that you have already added..
There is a similar construction in plot module, but the reason it is there is because we have both read and write operations that should have different priorities, hence we have a custom loop there that prefers read operations over writes whenever possible. It doesn't seem like we have such requirements here, so it would be better to have async methods with necessary code in them. Much easier to read and maintain.
There was a problem hiding this comment.
I would very much prefer to use async mutex to access mutable state rather than even loop. Event loop still woks with channels that have locks inside of them one way or another.
There was a problem hiding this comment.
Can we keep this for now just to get bootstrap and peer discovery working and then, replace it async mutex's later on? The entire "eventloop and client API" depends on the ClientEvent enum.
There was a problem hiding this comment.
That is the point, I would very much prefer to not get the entire event loop into the code base in the first place.
| pub listen_addr: Option<Multiaddr>, | ||
| } | ||
|
|
||
| pub struct Client { |
There was a problem hiding this comment.
This crate will become a library, think carefully what should actually be pub and what is only necessary as implementation detail and should be pub(super) or pub(crate). I think at least some of the stuff in this file if not all shouldn't be public.
There was a problem hiding this comment.
Client and its config are still public, while constructor isn't. Are they supposed to be public or it can be made more limited in visibility too?
There was a problem hiding this comment.
Actually we are not supposed to use Client::new(). In the current implementation, we use the create_connection method that gives us the Client and EventLoop objects in return. We need create_connection and ClientConfig to be public.
| pub listen_addr: Option<Multiaddr>, | ||
| } | ||
|
|
||
| pub struct Client { |
There was a problem hiding this comment.
Client and its config are still public, while constructor isn't. Are they supposed to be public or it can be made more limited in visibility too?
…nto impletement-dht2
|
Closing for now since we can't merge it as is. Feel free to submit another PR once you have time to work on this again. |
This PR creates a submodule
dhtin thecommandsmodule, that acts as a wrapper aroundrust-libp2p.The
dhtmodule is logically divided into two sections:ClientAPI - these are the methods that we will be using insidespartan-farmerto interact with the DHT.EventLoop- this is the part of code that handles the actual network events from the Kademlia protocol.The
EventLoopobject is spawned as a background task, so that it works in an async manner.Design patterns used here were picked up from the following PR in
rust-libp2p:TODO
bootstrap.Resultto method return values.ClientConfigto be able to create bootstrap nodes.oneshotchannels to getOk/Errfrom ClientEvents to NetworkEvents to ClientEvents.