-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(kad): make the bound of num closest peers returned configurable #6255
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
base: master
Are you sure you want to change the base?
Conversation
bd45dfc to
3f0fdb4
Compare
|
I don't see why such change can reduce network overhead, because no matter how many closest peers to list, the query path to the closest peer is the same, just the number of peers to record on the path gets changed. In other words, the depth of the query remain unchanged(until the last closest reachable branch of the BTree), but the depth of recording changes(higher number of peers to return means more branches are recorded). And I don't see benefit of returning more peers than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is correct @drHuangMHT
We cannot change how many peers are returned in a FIND_NODE request without modifying the spec, currently the number of returned peers is K, which is a system parameter.
More peers are needed for redundancy or faster discovery
Since the number of nodes returned by FIND_NODE RPC is fixed (K), the request will converge on the K closest peers to the target key. The other peers in the list aren't guaranteed to be the next closest (e.g K+1 closest) to the target key. Hence you could get more peers, but they won't be the closest.
Fewer peers are desired to reduce response size and network overhead
Since the number of peers returned by FIND_NODE RPC is fixed, this has no impact on response size. IIRC network overhead could be reduced by calling get_n_closest_peers() with a n smaller than the bucket size.
Custom DHT configurations require different peer list sizes
Nodes that don't share basic DHT configuration such as K, bucket size, number of peers in response shouldn't participate in the same DHT swarm. Each DHT swarm should have its own spec defining these important parameters.
@noamsp-starkware you may be interested in tuning the bucket size instead. Note that if you modify the bucket size, you should form a DHT swarm containing only peers with the same bucket size, and not try to join an existing DHT swarm with a different bucket size (e.g Amino DHT).
| { | ||
| // The inner code never expect higher than K_VALUE results to be returned. | ||
| // And removing such cap will be tricky, | ||
| // The inner code never expect higher than the configured num_closest_peers results to be | ||
| // returned. And removing such cap will be tricky, | ||
| // since it would involve forging a new key and additional requests. | ||
| // Hence bound to K_VALUE here to set clear expectation and prevent unexpected behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in this comment, having a cap higher than K_VALUE isn't trivial. It requires to send extra requests for forged keys.
The number of peers returned is a protocol limitation (it should be K_VALUE as per spec).
The max cap shouldn't be increased without either:
- A protocol change
- Implementing mechanism for exploring a keyspace area, which includes additional requests for forged keys.
Description
Make the number of closest peers returned in response to FIND_NODE requests configurable.
Previously, the Kademlia implementation always returned
K_VALUE(20) closest peers when responding to FIND_NODE requests. This change introduces a new configuration optionset_num_closest_peers()that allows users to customize this value while maintaining backward compatibility (defaults toK_VALUE).This is useful for use cases where:
The change affects:
Behaviour::get_closest_peers()- now uses the configurable valueBehaviour::get_closest_peers_with_results()- caps results to the configurable valueBehaviour::closest_peers()iterator - returns up to the configurable number of peersRelated Issues:
#5501
#5875
#6247
Notes & open questions
K_VALUE(20) for backward compatibilityChange checklist