-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add "getclosestpeers" support (IPIP-476) #124
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: main
Are you sure you want to change the base?
Conversation
GetClosestPeers endpoint was returning empty results or errors: 1. composableRouter.dht field was nil - the server never created a dht router, so GetClosestPeers always returned empty. Fixed by calling getCombinedRouting to create dhtRouters and passing it to the handler. 2. libp2pRouter type switch didn't handle bundledDHT (the default when SOMEGUY_DHT=accelerated), causing "cannot call GetClosestPeers on DHT implementation" error. Fixed by adding bundledDHT case that delegates to the active DHT (fullRT or standard). 3. dual.DHT case failed entirely when LAN lookup errored, even if WAN succeeded. Fixed to log LAN errors but continue with WAN results.
- add 10 test cases for GET /routing/v1/dht/closest/peers/{key} - verify 20 peers returned with unique addresses (127.0.0.1-127.0.0.20) - test JSON and NDJSON response formats - test empty results, ErrNotFound, and invalid key handling - test different key formats (CID, PeerID as CID) - test Accept header handling (default, wildcard) - verify response headers (Cache-Control, Content-Type, Vary, Last-Modified) - add makePeerRecords helper to generate test fixtures
- use cid.Parse instead of cid.Decode for consistency with other CLI commands - remove TODO comment in cachedRouter.GetClosestPeers - clarify UsageText to specify DHT-closest peers - add private address filtering in sanitizeRouter.GetClosestPeers
add missing addrs to GetClosestPeers to ensure light clients do not have to do extra lookups after addrs expired in regular libp2p peerbook. - add addrQueryOriginClosestPeers constant for metrics tracking - make queryOrigin configurable in cacheFallbackIter - create applyPeerRecordCaching helper to abstract type conversions - update GetClosestPeers to apply caching via helper - add tests for cache hit and FindPeers fallback scenarios
LAN DHT contains private network peers that should not be exposed via public HTTP Routing API. This fix ensures only WAN DHT results are returned to external clients.
- add parseKey() helper that accepts CIDs and legacy peer ID formats - change default endpoint for CLI commands to delegated-ipfs.dev - keep cid.contact as default for daemon mode
- document new /routing/v1/peers/closest/{key} endpoint (IPIP-476) - note CLI default endpoint change to delegated-ipfs.dev - update dependency versions
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.
i've pushed some changes (fixed router wiring, added tests, changelog) so would be good to have 👍 from @guillaumemichel or @hsanjuan
i also tested it locally as a demon and cli and works as expected (cli will work out of the box once we deploy this to delegated-ipfs.dev)
// Only use WAN DHT for public HTTP Routing API. | ||
// LAN DHT contains private network peers that should not be exposed publicly. | ||
peers, err = v.WAN.GetClosestPeers(ctx, keyStr) |
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.
ℹ️ This (WAN-only) feels like a safer behavior, than falling back to LAN on error, which could lead to mixing unrelated DHTs.
return nil, err | ||
} | ||
|
||
return r.applyPeerRecordCaching(it, ctx, addrQueryOriginClosestPeers), nil |
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.
ℹ️ I think we need/want to reuse the same cache we have in /routing/v1/providers
because normally, addrs will expire in ~15-30min, and we would be returning peers without addrs, and then browser client would have to do extra lookup to /routing/v1/peers
.
This line ensures we opportunistically add any cached addrs for peers we've seen before, to reduce the work light client has to do (they will get some addrs to try, and likely they will be correct).
TODO