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

protocols/kad: Expose kbucket distance range #1680

Merged
merged 8 commits into from
Aug 6, 2020

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jul 27, 2020

Instead of only returning non-empty kbuckets via Kademlia::kbuckets,
return empty ones as well.

Doing so enables consumers to (1) reason about each and every bucket and
(2) depend on a stable order of the buckets.

Add KBucketRef::range exposing the minimum inclusive and maximum
inclusive Distance for the bucket

More specifically within substrate I would like to expose the following metric:

HELP Number of nodes per protocol and Kademlia kbucket
substrate_sub_libp2p_kademlia_num_nodes_per_kbucket{protocol="sub", bucket="255"} 20

Instead of only returning non-empty kbuckets via `Kademlia::kbuckets`,
return empty ones as well.

Doing so enables consumers to (1) reason about each and every bucket and
(2) depend on a stable order of the buckets.
@mxinden mxinden requested a review from romanb July 27, 2020 13:46
@romanb
Copy link
Contributor

romanb commented Jul 27, 2020

The only small issue I have with this is that it leaks implementation details. Ideally, the implementation strategy for the buckets should not leak in the public API, but that is what happens when not filtering out empty buckets - empty buckets only exist because of the 256-fixed-array implementation, e.g. I don't think they would be present in an implementation based on a dynamically growing "bucket-tree", what is actually suggested in the paper. Of course, one could artificially "fill up" the gaps in the iterator with empty buckets in that case, but even then there would no longer always be 256 buckets because the ranges covered by the buckets vary in that case, if I remember correctly.

Isn't what you're mainly interested in the distances covered by the buckets? The order should always be stable. Maybe exposing the ranges covered by each bucket would be sufficient? If you know which distance each non-empty bucket covers, you also know the "empty space" in-between.

@mxinden
Copy link
Member Author

mxinden commented Jul 28, 2020

I agree that my suggestion leaks implementation details. E.g. making NUM_BUCKETS configurable in the future would be a breaking change.

@romanb are you suggesting a change along the lines of the following?

diff --git a/protocols/kad/src/kbucket.rs b/protocols/kad/src/kbucket.rs
index 2ea7f362..715eedde 100644
--- a/protocols/kad/src/kbucket.rs
+++ b/protocols/kad/src/kbucket.rs
@@ -447,6 +447,14 @@ where
     TKey: Clone + AsRef<KeyBytes>,
     TVal: Clone
 {
+    pub fn min_distance(&self) -> Distance {
+        unimplemented!()
+    }
+
+    pub fn max_distance(&self) -> Distance {
+        unimplemented!()
+    }
+

@romanb
Copy link
Contributor

romanb commented Jul 28, 2020

@mxinden Yes, something like that. Or just a single method along the lines of fn range(&self) -> (Distance, Distance).

mxinden added 2 commits July 31, 2020 09:41
Add `KBucketRef::range` exposing the minimum inclusive and maximum
inclusive `Distance` for the bucket
@mxinden mxinden changed the title protocols/kad: Kademlia::kbuckets include all protocols/kad: Expose kbucket distance range Jul 31, 2020
@mxinden
Copy link
Member Author

mxinden commented Jul 31, 2020

@romanb would you mind taking another look?

protocols/kad/src/kbucket.rs Show resolved Hide resolved
protocols/kad/src/kbucket.rs Outdated Show resolved Hide resolved
@romanb
Copy link
Contributor

romanb commented Aug 3, 2020

Feel free to update the changelog and merge at your convenience.

@mxinden mxinden merged commit 7601514 into libp2p:master Aug 6, 2020
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.

2 participants