feat: initial implementation #1
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1 +/- ##
=========================================
Coverage ? 96.29%
=========================================
Files ? 2
Lines ? 54
Branches ? 0
=========================================
Hits ? 52
Misses ? 2
Partials ? 0 Continue to review full report at Codecov.
|
|
@vasco-santos @mkg20001 still need to do some cleanup but this is the general thought for a discovery service coupled with a relay server to replace stardust/websocket-star. I am planning to work on a relay server repo tomorrow and do some integration testing with this, but wanted to share out my initial draft of this. Planning to have a working version of this by Monday and get some RCs/examples out some time this week. |
There was a problem hiding this comment.
@jacobheun I made a first pass through this PR
I will make a second pass once we have the relay server
| It is worth noting that this module does not include any message signing for queries. The reason for this is that libp2p-pubsub supports message signing and enables it by default, which means the message you received has been verified to be from the originator, so we can trust that the peer information we have received is indeed from the peer who owns it. This doesn't mean the peer can't falsify its own records, but this module isn't currently concerned with that scenario. | ||
|
|
||
| ## Usage | ||
|
|
There was a problem hiding this comment.
Should we add a Usage example? or would you prefer to create an issue and get it done on a follow up PR?
There was a problem hiding this comment.
I'll create an issue for a followup PR. I think this should mostly be a reference to the libp2p configuration documentation with a list of available options.
There was a problem hiding this comment.
Tracking at #2, the rest of the suggestions have been applied/fixed.
Co-Authored-By: Vasco Santos <vasco.santos@ua.pt>
|
|
||
| const protons = require('protons') | ||
| const schema = ` | ||
| message Query { |
There was a problem hiding this comment.
Just wondering why there's a query, queryresponse.
We're not really querying that way. We're just brodcasting.
Also is the query id being used?
Over pubsub we can't do request/response anyway
There was a problem hiding this comment.
I forgot to add the actual response code working on that now but got distracted with some other things. In reality, nothing is actually using the id at the moment, but the response will include it. We could potentially just drop the id as pubsub should give us enough information around the message if we wanted to do any abuse checking in the future.
Just wondering why there's a query, queryresponse.
We need more than just a broadcast, because we need other peers to respond in order to find out about them if they've been on the network.
There was a problem hiding this comment.
We need more than just a broadcast, because we need other peers to respond in order to find out about them if they've been on the network.
If we brodcast every X seconds, shouldn't that be enough?
If not, how do you imagine doing request/responses on pubsub? This sounds more like something rendezvous protocol should do and this should be just a "simpler version of it" then.
There was a problem hiding this comment.
I pushed up the code in https://github.com/libp2p/js-libp2p-pubsub-peer-discovery/pull/4/files to show the current process.
If we broadcast every X seconds, shouldn't that be enough?
We could. Right now, whenever you get a Query you publish your response, similar to MDNS. So it's like a solicited broadcast. The interval based broadcast could run unnecessarily often, but it's also more resistant to bad peers spam broadcasting and causing a flood of responses... so yeah, you're right, we should just make this an interval broadcast.
There was a problem hiding this comment.
The major downside to the interval is that there will be a delay in us learning about all of the peers. If the interval is say, 10seconds, and we join at the start of a new interval, it will take 10 seconds for us to discover all peers. This is a pretty minor downside though, especially since we are working on persistent peer store support.
This creates a libp2p PeerDiscovery compliant module that leverages Pubsub to discover other peers across the pubsub mesh. The intention is to allow browser peers (and other peers) to connect to discover one another through a mutual pubsub network.
For browser nodes, this could mean connecting to a relay server on startup. If the relay server is also subscribed to the discovery topic, it would cause all connected peers to discover one another when a peer joins the pubsub topic.
TODO
Caveats
Currently libp2p isn't passing itself to the discovery modules when it instantiates them. This should be a trivial addition to libp2p though and is entirely reasonable to do.
Prior Art