feat(kad_dht): configurable subnet-diversity limits + table-wide IP-group cap - #1442
Conversation
…roup cap Make the per-bucket subnet-diversity limit runtime-configurable and add an opt-in table-wide cap, both on RoutingTable (propagated to KBucket). - max_peers_per_subnet (KBucket/RoutingTable): overrides the per-bucket cap. Kept as a None sentinel resolved against the module MAX_PEERS_PER_SUBNET at read time (KBucket._subnet_limit), so patching the constant still works and default behavior is unchanged. (libp2p#1422) - max_peers_per_subnet_table (RoutingTable): rejects a new peer once its subnet holds that many peers across ALL buckets. Skips resident and exempt (subnet None) peers. Defaults to 0 = disabled, so behavior is unchanged. (libp2p#1421) New params are keyword-defaulted and appended, so no call signature breaks. Closes libp2p#1421 Closes libp2p#1422
|
@yashksaini-coder : Thank you for opening this PR, Yash, and really appreciate the contribution to py-libp2p. This is a nice initiative and a useful improvement to the routing-table implementation. The combination of a runtime-configurable per-bucket subnet-diversity limit and an opt-in table-wide IP-group cap is particularly valuable. I like that the changes preserve the existing default behaviour while giving applications more control over their routing-table diversity policies. The handling of the Great to see the backward-compatible API design, the additional unit and integration coverage, and all the checks passing. The scope is well aligned with the existing subnet-diversity work and the issues being addressed. Would also like to keep @acul71 and @sumanjeet0012 in the loop here, particularly given the Kademlia/DHT implications and the existing review context. Please coordinate with them as you continue the review. Overall, really nice work and a strong contribution. Thanks again for taking this on and for the thorough implementation and testing. CCing @johannamoran and @mishmosh. |
Resolve conflicts with libp2p#1426: re-apply the per-bucket subnet cap (_subnet_limit) inside the lock-wrapped KBucket.add_peer, and keep both appended test blocks.
acul71
left a comment
There was a problem hiding this comment.
Thanks for this contribution — the routing-table work is well scoped and the backward-compatible API design (appended kwargs, None sentinel for _subnet_limit(), propagation through bucket splits) is thoughtful. Unit test coverage for the new limits is solid and lint/typecheck are clean on the reviewed branch.
I have two items that should be addressed before merge:
1. Wire limits through KadDHT (issue #1422)
Issue #1422 proposes runtime configuration via a KadDHT/RoutingTable kwarg so operators can tune subnet limits without patching the library. This PR adds max_peers_per_subnet and max_peers_per_subnet_table on RoutingTable/KBucket, but KadDHT still constructs the table with defaults only:
self.routing_table = RoutingTable(self.local_peer_id, host)Please add optional kwargs on KadDHT.__init__ and forward them to RoutingTable(...). A short docstring example (similar to the existing enable_random_walk pattern) would help.
2. Resolve conflicting newsfragment for #1421
main already has newsfragments/1421.bugfix.rst with yamux/bitswap resource-leak content (unrelated to kad-dht subnet diversity). This PR adds newsfragments/1421.feature.rst for the actual #1421 table-wide cap feature.
Towncrier will emit two changelog entries under the same issue number with contradictory narratives. Please coordinate on renumbering or relocating the existing 1421.bugfix.rst (or consolidating fragments) before merge.
Optional (non-blocking)
- Add a unit test with both caps enabled (
max_peers_per_subnet=2,max_peers_per_subnet_table=3) — the realistic go-libp2p-style configuration. - Note in
RoutingTabledocs that go-libp2p defaultsmaxForTableto 3 while this PR defaults to0(disabled) for backward compatibility.
Full review artifacts: local downloads/AI-PR-REVIEWS/1442/AI-PR-REVIEW-1442-0.md.
…libp2p#1421/libp2p#1422 newsfragment Review follow-ups on libp2p#1442: KadDHT.__init__ accepts max_peers_per_subnet / max_peers_per_subnet_table and forwards them to RoutingTable; fold the libp2p#1421 table-wide cap note into 1422.feature.rst (main already has an unrelated 1421.bugfix.rst); add a both-caps test and note go-libp2p's maxForTable=3 default. Refs libp2p#1421 libp2p#1422
|
@acul71 Both items addressed in |
dfcfb05 to
b758d34
Compare
The multi-line subnet-limits call in the class docstring tripped docutils
('Unexpected indentation') in the docs build; 'Example::' makes the whole
example a literal block, which is what it was meant to be.
b758d34 to
886ddeb
Compare
acul71
left a comment
There was a problem hiding this comment.
Re-review: both requested changes are addressed.
KadDHT.__init__forwardsmax_peers_per_subnet/max_peers_per_subnet_tabletoRoutingTable(docstring example + propagation test).- #1421 changelog note consolidated into
1422.feature.rst; unrelated1421.bugfix.rston main left untouched. - Optional follow-ups included: both-caps (2/3) test and go-libp2p
maxForTable=3doc note.
All CI checks green (Windows re-run passed). LGTM.
Closes #1422 and #1421.
Two related enhancements to the routing table's IP/subnet-diversity enforcement (#1383), both non-breaking:
max_peers_per_subnetonRoutingTableandKBucket. Kept as aNonesentinel resolved against the moduleMAX_PEERS_PER_SUBNETat read time (KBucket._subnet_limit), so patching the constant still works and the default is unchanged. Propagated to every KBucket, including split children.max_peers_per_subnet_tableonRoutingTablerejects a new peer once its subnet holds that many peers across all buckets (closing the table-wide evasion of the per-bucket cap). Skips resident and exempt (subnetNone) peers. Defaults to 0 = disabled, so behavior is unchanged unless enabled.Both params are keyword-defaulted and appended, so no call signature breaks. Local: ruff + mypy clean; 165 kad_dht unit tests (incl. 9 new + the existing
test_subnet_opt_out_disables_check) and 14 integration tests green.