Kademlia: Address some TODOs - Refactoring - API updates. #1174
Conversation
The following left-over issues are addressed:
* The key for FIND_NODE requests is generalised to any Multihash,
instead of just peer IDs.
* All queries get a (configurable) timeout.
* Finishing queries as soon as enough results have been received is simplified
to avoid code duplication.
* No more panics in provider-API-related code paths. The provider API is
however still untested and (I think) still incomplete (e.g. expiration
of provider records).
* Numerous smaller TODOs encountered in the code.
The following public API changes / additions are made:
* Introduce a `KademliaConfig` with new configuration options for
the replication factor and query timeouts.
* Rename `find_node` to `get_closest_peers`.
* Rename `get_value` to `get_record` and `put_value` to `put_record`,
introducing a `Quorum` parameter for both functions, replacing the
existing `num_results` parameter with clearer semantics.
* Rename `add_providing` to `start_providing` and `remove_providing`
to `stop_providing`.
* Add a `bootstrap` function that implements a (almost) standard
Kademlia bootstrapping procedure.
* Rename `KademliaOut` to `KademliaEvent` with an updated list of
constructors (some renaming). All events that report query results
now report a `Result` to uniformly permit reporting of errors.
The following refactorings are made:
* Introduce some constants.
* Consolidate `query.rs` and `write.rs` behind a common query interface
to reduce duplication and facilitate better code reuse, introducing
the notion of a query peer iterator. `query/peers/closest.rs`
contains the code that was formerly in `query.rs`. `query/peers/fixed.rs` contains
a modified variant of `write.rs` (which is removed). The new `query.rs`
provides an interface for working with a collection of queries, taking
over some code from `behaviour.rs`.
* Reduce code duplication in tests and use the current_thread runtime for
polling swarms to avoid spurious errors in the test output due to aborted
connections when a test finishes prematurely (e.g. because a quorum of
results has been collected).
* Some additions / improvements to the existing tests.
| behaviour.add_address(&"QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64".parse().unwrap(), "/ip6/2604:a880:800:10::4a:5001/tcp/4001".parse().unwrap()); | ||
| behaviour.add_address(&"QmSoLer265NRgSp2LA3dPaeykiS1J6DifTC88f5uVQKNAd".parse().unwrap(), "/ip6/2a03:b0c0:0:1010::23:1001/tcp/4001".parse().unwrap()); | ||
| libp2p::core::Swarm::new(transport, behaviour, local_peer_id) | ||
| /*behaviour.add_address(&"QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN".parse().unwrap(), "/dnsaddr/bootstrap.libp2p.io".parse().unwrap()); |
There was a problem hiding this comment.
Why comment out addresses just because they don't work?
There was a problem hiding this comment.
Because they unnecessarily and permanently slow down running the example, which already runs into plenty of timeouts as it is.
| KademliaOut::PutValueResult( | ||
| PutValueResult::Err { key, cause: error } | ||
| ) | ||
| /// Stores a record in the DHT. |
There was a problem hiding this comment.
| /// Stores a record in the DHT. | |
| /// Starts the process of storing a record in the DHT. |
| /// The successful result of [`Kademlia::bootstrap`]. | ||
| #[derive(Debug, Clone)] | ||
| pub struct BootstrapOk { | ||
| pub peer: PeerId |
There was a problem hiding this comment.
The peer ID that is the target of the bootstrapping query - that is, either the local peer ID (first query) or a random peer ID for a specific bucket (follow-up queries). I can add a doc-comment.
There was a problem hiding this comment.
To me it sounded obvious that BootstrapOk was only produced once at the very end of the bootstrapping process.
There was a problem hiding this comment.
Which, to me, shows that more documentation is needed here.
There was a problem hiding this comment.
How about the existing documentation of Kademlia::bootstrap? I think it does explain it in sufficient detail, but let me know what you think is missing.
|
|
||
| /// The error result of [`Kademlia::put_record`]. | ||
| #[derive(Debug, Clone)] | ||
| pub enum PutRecordError { |
There was a problem hiding this comment.
A method into_key() would probably be appreciated by the user who wants to try again.
|
It looks like you don't want to put the mention in the docs that Other than that, looks good to me. |
|
I'm still unsure about the best wording in the documentation, but we can always change it later. Thanks for the review! |
Sits on top of #1154, addressing some left-over TODOs, as well as some refactoring and preparation for further work on the (still incomplete) record and provider system.
The following left-over issues from earlier work have been addressed:
The following public API changes / additions have been made:
KademliaConfigwith new configuration options for the replication factor and query timeouts.find_nodetoget_closest_peers.get_valuetoget_recordandput_valuetoput_record, introducing aQuorumparameter for both functions, replacing the existingnum_resultsparameter with clearer semantics.add_providingtostart_providingandremove_providingtostop_providing.bootstrapfunction that implements a (almost) standard Kademlia bootstrapping procedure.KademliaOuttoKademliaEventwith an updated list of constructors (some renaming). All events that report query results now report aResultto uniformly permit reporting of errors.The following refactorings have been made:
query.rsandwrite.rsbehind a common interface to reduce duplication and facilitate better code reuse, introducing the notion of a query peer "iterator".query/peers/closest.rscontains the code that was formerly inquery.rs.query/peers/fixed.rscontains a modified variant ofwrite.rs(which is removed). The newquery.rsprovides an interface for working with a collection of queries, taking over some code frombehaviour.rs.