-
Notifications
You must be signed in to change notification settings - Fork 138
routing/http: add support for GetClosestPeers (IPIP-476) #1021
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
bd45b83 to
c70f108
Compare
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1021 +/- ##
==========================================
+ Coverage 60.75% 60.82% +0.06%
==========================================
Files 268 268
Lines 33593 33741 +148
==========================================
+ Hits 20411 20522 +111
- Misses 11510 11538 +28
- Partials 1672 1681 +9
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
6ddfda9 to
3bb649c
Compare
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.
IIUC this PR isn't linked with the DHT yet, and there is no implementation of GetClosestPeers(ctx, pid, closerThanPid, count) (that doesn't depend on another GetClosestPeers(ctx, pid, closerThanPid, count)).
Is the bridge to the DHT going to be implemented in Kubo? I would like to see how everything fits together before merging this one.
Right. Yes, it is in Kubo where the bridge to an actual content routing thing happens. At which point we will need to call the DHT method with the DHT we have in Kubo. Perfectly fine to wait for that before merging. |
This allows Kubo to respond to the GetClosestPeers() http routing v1 endpoint as spec'ed here: ipfs/specs#476 It is based on work from ipfs/boxo#1021 We let IpfsNode implmement the contentRouter.Client interface with the new method. We use our DHTs to get the closest peers. We try to respect the count/closerThan options here. We then trigger FindPeers lookups to fill-in information about the peers (addresses) and return the result. Tests are missing and will come up once discussions around the spec and the boxo pr have settled.
This allows Kubo to respond to the GetClosestPeers() http routing v1 endpoint as spec'ed here: ipfs/specs#476 It is based on work from ipfs/boxo#1021 We let IpfsNode implmement the contentRouter.Client interface with the new method. We use our DHTs to get the closest peers. We try to respect the count/closerThan options here. We then trigger FindPeers lookups to fill-in information about the peers (addresses) and return the result. Tests are missing and will come up once discussions around the spec and the boxo pr have settled.
|
Triage:
|
I see no reason for it to be exported.
This adds the SERVER-side for GetClosestPeers. Since FindPeers also returns PeerRecords, it is essentially a copy-paste, minus things like addrFilters which don't apply here, plus `count` and `closerThan` parsing from the query URL. The tests as well. We leave all logic regarding count/closerThan to the ContentRouter (the DHT, or the Kubo wrapper around it). Spec: ipfs/specs#476
As the server part, this is heavily inspired in FindPeers.
Changes query parameter from 'closerThan' to 'closer-than' in routing/http client and server to match IPIP-0476 specification.
- clarified parameter behavior in godoc - documented amino dht limitation of 20 peers max - use amino.DefaultBucketSize constant for default count
- introduce DelegatedRouter interface for delegated routing operations - keep ContentRouter as deprecated alias for backward compatibility - update internal server struct to use DelegatedRouter - clarify interface focuses on querying with IPNS publishing support - note that additional publishing methods may be added via IPIP process this better aligns with the Delegated Routing V1 HTTP API spec and clarifies that this interface handles all routing operations (content, peers, values, DHT), not just content routing
This aligns it with latest version of the spec.
dbed850 to
0a8bc14
Compare
This allows Kubo to respond to the GetClosestPeers() http routing v1 endpoint as spec'ed here: ipfs/specs#476 It is based on work from ipfs/boxo#1021 We let IpfsNode implmement the contentRouter.Client interface with the new method. We use our DHTs to get the closest peers. We try to respect the count/closerThan options here. We then trigger FindPeers lookups to fill-in information about the peers (addresses) and return the result. Tests are missing and will come up once discussions around the spec and the boxo pr have settled.
|
Triage notes:
|
| _ routing.ValueStore = (*contentRouter)(nil) | ||
| _ routinghelpers.ProvideManyRouter = (*contentRouter)(nil) | ||
| _ routinghelpers.ReadyAbleRouter = (*contentRouter)(nil) | ||
| _ routinghelpers.DHTRouter = (*contentRouter)(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.
This seems to be the only use case of routinghelpers.DHTRouter (introduced in libp2p/go-libp2p-routing-helpers#93). The kubo PR isn't using it.
Since Client (above) implements GetClosestPeers() it should be enough, I don't think we need a DHTRouter interface.
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.
It breaks consistency? do we need a ReadyAbleRouter etc? IUUC the contentRouter is not a Client, but a Router implementation, the problem being that Router doesn't have a unified interface, which is ugly, but placing this new interface in a different place than the others is also weird.
This allows Kubo to respond to the GetClosestPeers() http routing v1 endpoint as spec'ed here: ipfs/specs#476 It is based on work from ipfs/boxo#1021 We let IpfsNode implmement the contentRouter.Client interface with the new method. We use our DHTs to get the closest peers. We try to respect the count/closerThan options here. We then trigger FindPeers lookups to fill-in information about the peers (addresses) and return the result. Tests are missing and will come up once discussions around the spec and the boxo pr have settled.
- parseKey() utility tries peer.Decode() first, then cid.Decode() - tests use hardcoded examples from libp2p specs - tests verify digest matching (not exact CID) via hex-encoded values - only the multihash digest matters for DHT operations, not CID codec - error messages include both parse failures for better debugging
- routing/http/server accepts CID or PeerID for GetClosestPeers - references IPIP-476
fixes copy-paste error where GetClosestPeers endpoint was logging errors as "FindPeers" instead of "GetClosestPeers"
|
Thank you @lidel . Latest changes LGTM. Would you be so kind to call the shot regarding the DHTRouter interface in libp2p-routing-helpers (commit above from @guillaumemichel)? We can move it here or leave it there. Probably it is the last thing to resolve. |
| // GetClosestPeers returns the DHT closest peers to the given peer ID. | ||
| // If empty, it will use the content router's peer ID (self). `closerThan` (optional) forces resulting records to be closer to `PeerID` than to `closerThan`. `count` specifies how many records to return ([1,100], with 20 as default when set to 0). | ||
| // GetClosestPeers returns the DHT closest peers to the given key (CID or Peer ID). | ||
| GetClosestPeers(ctx context.Context, key cid.Cid) (iter.ResultIter[*types.PeerRecord], error) |
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.
Should the key argument be a cid.Cid or a string? Seems like it needs to be a string if accepting cid or peer id.
Server side
Since FindPeers also returns PeerRecords, it is essentially a copy-paste, minus things like addrFilters which don't apply here, plus
countandcloserThanparsing from the query URL. The tests as well. We leave all logic regarding count/closerThan to the ContentRouter (the DHT, or the Kubo wrapper around it).Client side
As the server part, this is heavily inspired in FindPeers.
Content router side:
Added GetClosestPeers support, depends on libp2p/go-libp2p-routing-helpers#93