Kademlia: Somewhat complete the records implementation. #1189

romanb · 2019-06-30T16:47:36Z

Sits on top of #1174 and relates to libp2p-146 and libp2p-1089.

The following are the major changes:

All records expire (by default, configurable).
Storage of provider records is also moved to the RecordStore, and the RecordStore API extended.
Background jobs for periodic (re-)replication and (re-)publication of records. Regular (value-)records are subject to re-replication and re-publication as per standard Kademlia. Provider records are only subject to re-publication.
For standard Kademlia value lookups (quorum = 1), if found, the record is cached at the closest peer to the key that did not return the value, as per standard Kademlia.
Expiration times of regular (value-)records are computed exponentially inversely proportional to the number of nodes between the local node and the closest node known to the key (beyond the k closest), as per standard Kademlia.

To that end, the protobuf structure for regular (value-)records is extended with the two fields ttl and publisher, in order to implement the different semantics of re-replication (by any of the k closest peers to the key, not affecting expiry) and re-publication (by the original publisher, resetting the expiry). This is not done yet in other libp2p Kademlia implementations (see e.g. libp2p-go-323). The new protobuf fields have been given somewhat unique identifiers to prevent future collision, should the libp2p spec and protocol be extended.

Similarly, periodic re-publication of provider records does not seem to be done yet in other implementations (see e.g. libp2p-js-98) but was already drafted in the existing Rust implementation, which I thus continued.

montekki · 2019-07-02T21:12:10Z

protocols/kad/src/behaviour.rs

-                )));
-                return;
+    /// The result of this operation is delivered in [`KademliaEvent::GetRecordResult`].
+    pub fn get_record(&mut self, key: &Multihash, quorum: Quorum) {


I think that we shouldn't be handing out to users the Multihash types in the api, since it would make a lot of code in substrate depentable on Multihash and different places in code can hash things differently and so on. What if the api took bytes and did the hashing?

Choosing multihashes as record keys was done in your original PR and didn't change here. I think it would be fine (even good) to further generalise over the record key type, but that should be done in another PR - this one is big enough as it is.

What if the api took bytes and did the hashing?

Not sure what you mean by that. Whatever the key is, it is always hashed into the Kademlia keyspace, see protocols/kad/kbucket/key.rs.

For what it's worth, I have a branch here that sits on top of this PR and generalises record keys. I may open a PR for that once / if this PR is merged.

This commit relates to [libp2p-146] and [libp2p-1089]. * All records expire (by default, configurable). * Provider records are also stored in the RecordStore, and the RecordStore API extended. * Background jobs for periodic (re-)replication and (re-)publication of records. Regular (value-)records are subject to re-replication and re-publication as per standard Kademlia. Provider records are only subject to re-publication. * For standard Kademlia value lookups (quorum = 1), the record is cached at the closest peer to the key that did not return the value, as per standard Kademlia. * Expiration times of regular (value-)records is computed exponentially inversely proportional to the number of nodes between the local node and the closest node known to the key (beyond the k closest), as per standard Kademlia. The protobuf messages are extended with two fields: `ttl` and `publisher` in order to implement the different semantics of re-replication (by any of the k closest peers to the key, not affecting expiry) and re-publication (by the original publisher, resetting the expiry). This is not done yet in other libp2p Kademlia implementations, see e.g. [libp2p-go-323]. The new protobuf fields have been given somewhat unique identifiers to prevent future collision. Similarly, periodic re-publication of provider records does not seem to be done yet in other implementations, see e.g. [libp2p-js-98]. [libp2p-146]: libp2p#146 [libp2p-1089]: libp2p#1089 [libp2p-go-323]: libp2p/go-libp2p-kad-dht#323 [libp2p-js-98]: libp2p/js-libp2p-kad-dht#98

To ensure task notification, since `NotReady` is returned right after.

In order for a user to easily distinguish the result of e.g. a `put_record` operation from the result of a later republication, different event constructors are used. Furthermore, for now, re-replication and "caching" of records (at the closest peer to the key that did not return a value during a successful lookup) do not yield events for now as they are less interesting.

twittner

I have not finished the review, but left some comments/questions.

protocols/kad/src/addresses.rs

protocols/kad/src/lib.rs

protocols/kad/src/query.rs

protocols/kad/src/addresses.rs

twittner · 2019-07-05T10:49:12Z

protocols/kad/src/jobs.rs

+#[derive(Debug)]
+enum PeriodicJobState<T> {
+    Running(T),
+    Waiting(Delay)


Why contains PeriodicJobState::Waiting a future instead of a deadline value?

The idea is that when a job is polled (see PutRecordJob::poll and AddProviderJob::poll) and returns NotReady the current task is woken up when the job is ready to run, since there may be nothing else going on otherwise. This polling of the jobs is done in Kademlia::poll. Does that make sense or did you have something else in mind with your question?

No, thanks. It is a bit subtle and is_ready does not convey that behind the scenes there is some task registration that will eventually cause poll to be invoked again.

protocols/kad/dht.proto

* Guard a node against overriding records for which it considers itself to be the publisher. * Document the jobs module more extensively.

…ders

twittner · 2019-07-12T15:47:15Z

protocols/kad/src/jobs.rs

+//! whilst (re-)replication primarily ensures persistence for the duration
+//! of the TTL in the light of topology changes. Consequently, replication
+//! intervals should be shorter than publication intervals and
+//! publication intervals should be shorter than the TTL.


This deviates somewhat from the paper I think. In there, re-publishing is done once per hour on each node and every 24 hours by the owner, or else the key expires. Replication is not periodic but happens when a node discovers a new node closer to some of its keys. Then, those key-value pairs will be replicated to the new node. I have not found anything about "re-replication" in the paper.

I think that is just a matter of terminology. In order to distinguish the hourly re-publishing done by every node from the 24h re-publishing done by the node that is the "original publisher", I call the former re-replication. The replication of values to a newly joined node can of course be given separate attention, but since that is also covered by the hourly re-republication / re-replication, I didn't want to over-complicate things.

I guess my choice of naming is partly derived from the choice of naming of the constants for the intervals in this design specification.

twittner · 2019-07-12T15:47:58Z

protocols/kad/src/jobs.rs

+#[derive(Debug)]
+enum PeriodicJobState<T> {
+    Running(T),
+    Waiting(Delay)


No, thanks. It is a bit subtle and is_ready does not convey that behind the scenes there is some task registration that will eventually cause poll to be invoked again.

protocols/kad/src/addresses.rs

tomaka

Mostly nits and questions ; the PR in general looks excellent!

tomaka · 2019-07-17T09:13:26Z

protocols/kad/src/handler.rs

@@ -271,6 +270,8 @@ impl From<ProtocolsHandlerUpgrErr<io::Error>> for KademliaHandlerQueryErr {
 /// Event to send to the handler.
 #[derive(Debug)]
 pub enum KademliaHandlerIn<TUserData> {
+    Reset(KademliaRequestId),


Needs some documentation saying what that does.

tomaka · 2019-07-17T09:14:10Z

protocols/kad/src/handler.rs

    fn inject_event(&mut self, message: KademliaHandlerIn<TUserData>) {
        match message {
+            KademliaHandlerIn::Reset(request_id) => {


Can't we just wait for the requests to be answered or to time out instead?

This is currently used when the node receives a request to store a record, but for some reason (e.g. a storage error) is unable to (or refuses to) store it. We can just let the request time out on the remote, which is what was done before but there was a TODO left for it (by me). I thought it would be preferable to signal errors as quick as possible to not cause queries unnecessary delay in the normal case, and since the protocol itself has no explicit error responses, resetting the stream seemed the only option. Does that make sense?

Oh I see, I misunderstood the purpose.

tomaka · 2019-07-17T09:17:37Z

protocols/kad/src/record/store.rs

+    fn remove(&'a mut self, k: &Multihash);
+
+    /// Gets an iterator over all (value-) records currently stored.
+    fn records(&'a self) -> Self::RecordsIter;


Is that a good idea? It works for an in-memory records store, but not a records store that stores them on disk.

Why do you think it can only be implemented by an in-memory store? That is certainly not my intention, but every record store must provide a means to iterate through all records - that is simply a requirement of the Kademlia protocol.

My impression is that the minimum requirement would be an iterator over the record keys/ids. My concerns is that the iterator also contains tons of metadata as well as the record's content, which might not be desirable to keep in memory.

This is a minor concern though.

I see - since I implemented the optimisation from the paper that records are skipped from hourly replication if they have recently been received from another peer (i.e. that effectively only 1 of the k peers closest to a key replicates a record hourly in most cases) just iterating over the keys may indeed be beneficial in terms of memory use, albeit requiring separate lookups from the storage for every record that is not skipped. We should certainly keep that in mind as a trade-off to (re)consider.

protocols/kad/dht.proto

tomaka · 2019-07-17T09:20:20Z

protocols/kad/src/addresses.rs

+    ///
+    /// An address should only be removed if is determined to be invalid or
+    /// otherwise unreachable.
+    pub fn remove(&mut self, addr: &Multiaddr) -> bool {


What about returning -> Result<(), ()> instead? IMO it's more explicit that Ok means that the removal was successful.

Sounds good to me.

tomaka · 2019-07-17T09:24:32Z

protocols/kad/src/behaviour.rs

+
+        let now = Instant::now();
+
+        // Calculate the expiration exponentially inversely proportional to the


I didn't even know that was in the paper.

tomaka · 2019-07-17T09:27:44Z

protocols/kad/src/record/store.rs

+    type ProvidedIter: Iterator<Item = Cow<'a, ProviderRecord>>;
+
+    /// Gets a record from the store, given its key.
+    fn get(&'a self, k: &Multihash) -> Option<Cow<Record>>;


Should eventually be asynchronous, but this would probably be extremely hard to implement as long as we don't have async/await, and not worth it.

I thought the same - besides we have #1159 - but I am myself not sure whether the complications w.r.t. the implementation of the Kademlia behaviour that would come along with such a change would be worth it, especially not before async/await. I think a first step would be to actually allow even RecordStore implementations using synchronous I/O without unwraping, which will require all trait methods to return a Result and store::Error would need a constructor for io::Error. I left that for later as well.

Change the semantics of `Addresses::remove` so that the error case is unambiguous, instead of the success case. Use the `Result` for clearer semantics to that effect.

tomaka

Looks good to me.
I would normally wait for @twittner's approval, but since he's on holidays I guess we can merge this.

romanb · 2019-07-17T12:39:55Z

The last feedback I got from him on Friday was that the PR overall looks good. He just had some thoughts around the implementation of the periodic jobs, e.g. for splitting the PutRecordJob up to separate re-replication (default hourly) from (re)publishing (default daily). I thought it was neat to combine these into the same job for reasons of efficiency but it may be worthwhile to experiment with a split. I don't want to do that in the context of this PR, however. So I think he is fine with merging and may even have intentionally left approval up to you.

Thank you both for the review and suggestions.

This was referenced Jun 30, 2019

Update Kademlia API. Fix DiscoveryBehaviour delays. paritytech/substrate#2981

Closed

Kademlia: Address some TODOs - Refactoring - API updates. #1174

Merged

montekki reviewed Jul 2, 2019

View reviewed changes

romanb force-pushed the kad-records-providers branch from a1f3f71 to 28aee24 Compare July 3, 2019 12:52

Roman S. Borschel added 9 commits July 3, 2019 16:41

Tweak kad-ipfs example.

a2e74e6

Incorporate some feedback.

905eaa4

Add missing files.

434afdf

Ensure new delays are polled immediately.

2b0732a

To ensure task notification, since `NotReady` is returned right after.

Fix ipfs-kad example and use wasm_timer.

1024488

Small cleanup.

0d1706d

Adjustments after rebase.

f587039

romanb force-pushed the kad-records-providers branch from 28aee24 to 8111417 Compare July 3, 2019 15:42

Roman S. Borschel added 2 commits July 4, 2019 15:07

Speed up tests for CI.

f71ef7d

Merge branch 'master' into kad-records-providers

9bca48f

romanb mentioned this pull request Jul 4, 2019

Some documentation for discovery.rs paritytech/substrate#3024

Merged

twittner reviewed Jul 5, 2019

View reviewed changes

tomaka reviewed Jul 5, 2019

View reviewed changes

protocols/kad/dht.proto Show resolved Hide resolved

Roman S. Borschel added 10 commits July 8, 2019 15:20

Small refinements and more documentation.

28d7589

* Guard a node against overriding records for which it considers itself to be the publisher. * Document the jobs module more extensively.

More inline docs around removal of "unreachable" addresses.

47d29ff

Remove wildcard re-exports.

3f17016

Use NonZeroUsize for the constants.

8c88e29

Merge branch 'master' into kad-records-providers

56f2ff9

Re-add method lost on merge.

492120c

Add missing 'pub'.

e2e4731

Further increase the timeout in the ipfs-kad example.

f5df803

Merge remote-tracking branch 'upstream/master' into kad-records-provi…

62c6917

…ders

Readd log dependency to libp2p-kad.

3df3e3c

romanb mentioned this pull request Jul 9, 2019

libp2p-next paritytech/substrate#3076

Merged

Roman S. Borschel added 3 commits July 9, 2019 16:03

Simplify RecordStore API slightly.

615eb72

Some more commentary.

16c39c0

Merge remote-tracking branch 'upstream/master' into kad-records-provi…

89cfdd5

…ders

romanb mentioned this pull request Jul 11, 2019

Remove the various unimplemented!() from the code #295

Closed

twittner reviewed Jul 12, 2019

View reviewed changes

tomaka suggested changes Jul 17, 2019

View reviewed changes

Roman S. Borschel added 2 commits July 17, 2019 12:27

Change Addresses::remove to return Result<(),()>.

17dfa35

Change the semantics of `Addresses::remove` so that the error case is unambiguous, instead of the success case. Use the `Result` for clearer semantics to that effect.

Add some documentation to .

0fbc8db

tomaka approved these changes Jul 17, 2019

View reviewed changes

romanb merged commit cde93f5 into libp2p:master Jul 17, 2019

romanb deleted the kad-records-providers branch July 17, 2019 12:40

romanb mentioned this pull request Jul 24, 2019

Implement Kademlia record store #146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kademlia: Somewhat complete the records implementation. #1189

Kademlia: Somewhat complete the records implementation. #1189

romanb commented Jun 30, 2019 •

edited

Loading

montekki Jul 2, 2019

romanb Jul 3, 2019

romanb Jul 11, 2019

twittner left a comment

twittner Jul 5, 2019

romanb Jul 6, 2019

twittner Jul 12, 2019

twittner Jul 12, 2019

romanb Jul 12, 2019 •

edited

Loading

romanb Jul 12, 2019

twittner Jul 12, 2019

tomaka left a comment

tomaka Jul 17, 2019

tomaka Jul 17, 2019

romanb Jul 17, 2019

tomaka Jul 17, 2019

tomaka Jul 17, 2019

romanb Jul 17, 2019

tomaka Jul 17, 2019

romanb Jul 17, 2019

tomaka Jul 17, 2019

romanb Jul 17, 2019

tomaka Jul 17, 2019

tomaka Jul 17, 2019

romanb Jul 17, 2019

tomaka left a comment •

edited

Loading

romanb commented Jul 17, 2019


		let now = Instant::now();

		// Calculate the expiration exponentially inversely proportional to the

Kademlia: Somewhat complete the records implementation. #1189

Kademlia: Somewhat complete the records implementation. #1189

Conversation

romanb commented Jun 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twittner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanb Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka left a comment • edited Loading

Choose a reason for hiding this comment

romanb commented Jul 17, 2019

romanb commented Jun 30, 2019 •

edited

Loading

romanb Jul 12, 2019 •

edited

Loading

tomaka left a comment •

edited

Loading