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

feat(kad): configurable bucket size #5414

Merged
merged 6 commits into from
Aug 8, 2024
Merged

Conversation

guillaumemichel
Copy link
Contributor

@guillaumemichel guillaumemichel commented May 22, 2024

Description

Making bucket size configurable. Currently K_VALUE is used by default, and the only way to change the bucket size is to edit the const.

Resolves #5389

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@guillaumemichel guillaumemichel marked this pull request as ready for review May 23, 2024 14:54
protocols/kad/src/kbucket.rs Outdated Show resolved Hide resolved
protocols/kad/src/kbucket.rs Outdated Show resolved Hide resolved
@dariusc93
Copy link
Member

/CC @mxinden

@mxinden
Copy link
Member

mxinden commented May 27, 2024

I am not opposed to this change in specific, but want to warn you of introducing yet another configuration option, and thus yet another dimension in which bugs can occur.

Instead of introducing more nobs for users to tweak, I suggest improving the existing, alleviating users from the need to tune nobs in the first place.

libp2p-kad is already very hard to deploy. In my eyes, this makes it even harder. What is the upside of this change? In other words, what is the use-case?

@guillaumemichel
Copy link
Contributor Author

@mxinden Unfortunately this issue cannot be tackled by any existing nob.

The upside of this change is to allow users to modify the bucket size, which is a system parameter. Currently, it is set to the hardcoded K_VALUE = 20 constant. The Amino DHT has a bucket size of 20 but it isn't the case of all networks (e.g ethereum discv5 has a bucket size of 16).

The issue originated in #5371. TL;DR changing the bucket size can help debug other problems in the network.

Would it help to add to the documentation that this is an advanced nob?

@dariusc93
Copy link
Member

Would it help to add to the documentation that this is an advanced nob?

If there is no objections to it, it might be best to document it, but I also do agree that we should also improve what we have too. libp2p spec doesnt restrict us to just a bucket size of 20 as it is the recommended size, but there could be other use cases to increase or decrease it (assuming the end-user knows what they are doing)

@elecbug
Copy link
Contributor

elecbug commented Jul 17, 2024

I rewrite what I wrote in #5501 here.

To facilitate the discussion, I would like to add a little bit of what we have researched.

First, in an environment with K_VALUE of 20, one node gets approximately maxed 5K neighbors. It is useful in large IPFS-like environments where at least thousands of nodes communicate, but in small environments, including libp2p alone, when the total number of nodes is significantly less than 5K, a dense P2P network is formed that is meaningless with DHT.

It is thought that these networks can especially hinder the participation of small-sized researchers like us, who use libp2p to create and study nodes with their own server for a small scale, and in fact, we are currently considering a transfer to the Go language.

We started working with Rust, loved it, and would love to continue. I think these modifications will be useful to make people like us more interested in rust-libp2p.

Thanks for reading :)

@AgeManning
Copy link
Contributor

I agree. It seems users find this useful and different kad systems are using different bucket configurations.

It seems like making this configurable will extend the use of rust-libp2p to these discovery mechanisms,

I think its a good addition.

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks Gui

@jxs jxs added the send-it label Aug 8, 2024
@mergify mergify bot merged commit 417968e into master Aug 8, 2024
72 checks passed
@mergify mergify bot deleted the feat/custom-bucket-size branch August 8, 2024 12:16
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
Making bucket size configurable. Currently `K_VALUE` is used by default, and the only way to change the bucket size is to edit the const.

Resolves libp2p#5389

Pull-Request: libp2p#5414.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kad: make bucket size configurable
6 participants