-
Notifications
You must be signed in to change notification settings - Fork 275
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
Ambient peer discovery protocol #590
base: master
Are you sure you want to change the base?
Conversation
## Protocol | ||
|
||
1. Node _A_ opens a new stream to node _B_ with the protocol name `/libp2p/ambient-peers`. | ||
1. Node _B_ chooses a subset of at most 5 known peer records received from other peers. |
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.
1. Node _B_ chooses a subset of at most 5 known peer records received from other peers. | |
1. Node _B_ MUST choose a subset of known peer records received from other peers. Node _B_ SHOULD limit the subset to a maximum of 5 peers. |
We shouldn't enforce number of peers returned in the spec, but provide a recommendation
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.
But how do you know, if the other side configured a different number? Unless we make this a runtime parameter of the protocol, I think enforcing a number is easier. A node can always run the same protocol again to get more peers.
For example, if node _A_ and node _B_ are connected via WebRTC, node _B_ SHOULD select 5 peer records where each one of them has at least one WebRTC address. | ||
1. Node _B_ SHOULD NOT be currently connected to any of these nodes. | ||
1. Node _B_ writes these peer records onto the stream in their [protobuf encoding](https://github.com/libp2p/specs/blob/master/RFC/0003-routing-records.md#address-record-format), each record being length-prefixed using an unsigned varint and closes the stream after the last one. | ||
1. Node _A_ reads peer records from the stream until EOF or 5 have been received, whichever comes earlier. |
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.
1. Node _A_ reads peer records from the stream until EOF or 5 have been received, whichever comes earlier. | |
1. Node _A_ reads peer records from the stream until EOF or <num> have been received, whichever comes earlier. Implementations MAY allow overriding of <num> but SHOULD use a default of 5. |
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
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'm not sure if @vyzo's concerns have been properly addressed here.
I'd also see a clear use case for this before continuing work on this proposal. We shouldn't go around and specify protocols without a very clear need.
1. Node _B_ chooses a subset of at most 5 known peer records received from other peers. | ||
1. The chosen peer records SHOULD at least have one address that share the same transport technology as the the connection between node _A_ and node _B_. | ||
For example, if node _A_ and node _B_ are connected via WebRTC, node _B_ SHOULD select 5 peer records where each one of them has at least one WebRTC address. | ||
1. Node _B_ SHOULD NOT be currently connected to any of these nodes. |
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 is phrased in a misleading way.
- Returning stale peers seems less useful.
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.
Returning stale peers seems less useful.
Agreed that it is less useful but it is also a lot safer. We landed on this compromise because there was a lot of strong feedback that returning your current peers is unacceptable from a privacy PoV.
|
||
1. Node _A_ opens a new stream to node _B_ with the protocol name `/libp2p/ambient-peers`. | ||
1. Node _B_ chooses a subset of at most 5 known peer records received from other peers. | ||
1. The chosen peer records SHOULD at least have one address that share the same transport technology as the the connection between node _A_ and node _B_. |
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.
Making the supported transports explicit seems like a strictly superior way.
This protocol requires nodes to store records of peers they used to be connected to. | ||
This is useful independently of this protocol to e.g. reconnect to a peer you've once been connected to. |
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 adds a lot of complexity to implementations.
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.
Can you elaborate on why this adds complexity? I appreciate that the various implementations differ greatly in their internal design but fundamentally, this can easily be satisfied with an LRU cache. Really, all we are doing is accessing the "peer store". I can make that more explicit if you want?
Agreed! The work was primarily motivated by universal-connectivity which as far as I understand, is the flagship of the whole connectivity story of libp2p. At the moment however, it is quite limited in that when you connect to another browser peer using it, there is no libp2p-native way to discover further nodes from it. This proposal aims to solve this. I've also updated the PR description with the issues that have been created to fill this and similar needs. It is also listed on the roadmap. |
In general, I think a minimal peer discovery protocol like the one proposed here is useful.
Agreed. Without a clear use-case in mind, I don't think we will design a good protocol.
I see the universal-connectivity app as a showcase application only. I don't think it suffices as a clear use-case to justify a new libp2p protocol. To me a simple hack, e.g. one Gossipsub topic where everyone periodically sends their address, is good enough for universal-connectivity. All that said, in case there is a real-world use-case out there and they are willing to drive this effort, let's get a first implementation in place and then iterate on the specification. |
I agree that it is a showcase. But it also clearly surfaces a limitation of the current libp2p protocol suite: In a browser-to-browser setting, application developers need to roll their own solutions for discovering new peers.
Given the simplicity of the protocol, how about we implement it within universal-connectivity as a first state? I am happy to implement the Rust version of this protocol. Perhaps @achingbrain would be interested in prototyping a JS implementation? I also want to highlight that this spec is marked as Working Draft. It is by no means meant to be perfect and / or complete but I do think it is in a coherent state and something that is interesting to experiment with. Having it as a module in universal-connectivity would:
If I understand the spec life-cycle correctly, then this is what working drafts are for. I'd like to ask to taking this into consideration when reviewing the proposal. |
Why not? It demonstrates a pretty common hub-spoke network topology with browsers being ephemeral peers that come and go and one or two Rust/Go peers are constantly running and act like boostrappers that assist browser peers with establishing p2p connectivity. It's the use case laid out here: libp2p/universal-connectivity#95 |
FYI: usecases that this spec helps to resolve are asked about in ip-js, and other filecoin slack channels, often.
also https://github.com/shovelers/network-monorepo is one specific app that comes to mind as a usecase for this. I don't understand how there's confusion about the value of making it easier to discover peers in a (the) p2p lib.. |
This PR attempts to specify an ambient peer discovery protocol for libp2p. Lots of feedback from the discussion at #587 has already been incorporated. Thanks to everyone for getting involved.
I am putting this up for a more structured review.
Resolves: #222.
Resolves: libp2p/notes#3.
Resolves: libp2p/notes#3.