Skip to content

feat: allow setting Kad K_VALUE using feat flag #5988

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mickvandijke
Copy link
Contributor

@mickvandijke mickvandijke commented Apr 15, 2025

Description

Introduces new feature flags to change the K_VALUE for Kademlia. The new feat flags are: k-4, k-8, k-12 and k-16.

Related issues:

Notes & open questions

I know that Config::set_kbucket_size is a thing, but that doesn't change the K_VALUE, which influences many parts of the code.

I have tried making the K_VALUE configurable using a compile time environment variable and a build script, which would arguably be a cleaner solution, but I couldn't get it to work.

So I thought using feat flags would be a nice, although maybe a bit unorthodox alternative, that doesn't require significant code refactoring.

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

@mickvandijke mickvandijke force-pushed the feat-k-value-feat-flags branch from af0abea to 4fdc5dc Compare April 15, 2025 14:01
@mickvandijke
Copy link
Contributor Author

Amended the commit to mutex all k-n features and default to K = 20 if all features are enabled.

@mickvandijke mickvandijke force-pushed the feat-k-value-feat-flags branch 3 times, most recently from 0a39dec to 78514ce Compare April 15, 2025 14:28
@mickvandijke
Copy link
Contributor Author

Updated the CI to run the libp2p-kad tests with all the k-n features separately.

Introduces new feature flags to change the `K_VALUE` for Kademlia. The new feat flags are: `k-4`, `k-8`, `k-12` and `k-16`.
@mickvandijke mickvandijke force-pushed the feat-k-value-feat-flags branch from 78514ce to 0a1880e Compare April 16, 2025 07:27
@mickvandijke mickvandijke force-pushed the feat-k-value-feat-flags branch from 0a1880e to e9b7c94 Compare April 16, 2025 08:09
@elenaf9
Copy link
Contributor

elenaf9 commented Apr 17, 2025

Hi @mickvandijke, thank you for your PR!

Citing @guillaumemichel from #5501 (comment):

On a note, this magic K_VALU E is having 3 different roles, that could be decoupled from each other:

  1. Size of the routing table's buckets
  2. Number of replications of provider records when they are published
  3. Number of closer peers that are returned during a kademlia lookup

#5414 enables to set a custom bucket size without updating the other parameters. Unless there is a need to tweak the other 2 parameters, we should stick with using a single K_VALUE for all 3 parameters and users can set a custom bucket size if required once #5414 is merged.

Can you expand a little bit on why you need to modify 2) or 3)?


Implementation wise @guillaumemichel is probably best to review this, but I don't think we should use feature-flags.

  1. They only allow a fixed list of possible K_VALUES. If we support custom K_VALUE IMO we should support all non-zero ints
  2. Features in Rust should be additive, which is not the case with these new features. E.g. enabling both k-4 and k-8 will cause a conflict.

Two alternative solutions that came to my mind:

  • Make it a Config option. We'd then have to change a few types from K_VALUE-length arrays into Vecs
  • Make it a const generic on Behavior and other affected types, that defaults to 20. Not sure how many types this would affect.

@guillaumemichel
Copy link
Contributor

I agree with @elenaf9, feature flags are probably not the correct way to add configurability for K_VALUE.

Two alternative solutions that came to my mind:

  • Make it a Config option. We'd then have to change a few types from K_VALUE-length arrays into Vecs
  • Make it a const generic on Behavior and other affected types, that defaults to 20. Not sure how many types this would affect.

Both suggested solutions seem like a better way to proceed.

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.

3 participants