Skip to content
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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 16, 2023

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.

ambient-peer/README.md Outdated Show resolved Hide resolved
## 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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>
Copy link
Contributor

@marten-seemann marten-seemann left a 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is phrased in a misleading way.
  2. Returning stale peers seems less useful.

Copy link
Contributor Author

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_.
Copy link
Contributor

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.

Comment on lines +58 to +59
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.
Copy link
Contributor

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.

Copy link
Contributor Author

@thomaseizinger thomaseizinger Oct 22, 2023

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?

@marten-seemann marten-seemann requested a review from vyzo October 20, 2023 04:56
@thomaseizinger
Copy link
Contributor Author

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.

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.

@mxinden
Copy link
Member

mxinden commented Oct 27, 2023

In general, I think a minimal peer discovery protocol like the one proposed here is useful.

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.

Agreed. Without a clear use-case in mind, I don't think we will design a good protocol.

The work was primarily motivated by universal-connectivity which as far as I understand, is the flagship of the whole connectivity story of libp2p.

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.

@thomaseizinger
Copy link
Contributor Author

The work was primarily motivated by universal-connectivity which as far as I understand, is the flagship of the whole connectivity story of libp2p.

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.

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.

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.

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:

  • Allow us to gain experience in how well the current proposal works.
  • Allow users to have something to copy-paste into their own projects.
  • Allow users to give us feedback on the idea on whether or not it works for them.
  • Clearly communicate that this isn't something the libp2p project is committed to supporting long-term because it is not part of the respective libp2p implementations.

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.

@2color
Copy link
Contributor

2color commented Nov 12, 2023

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.

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

@SgtPooki
Copy link
Member

SgtPooki commented Apr 15, 2024

FYI: usecases that this spec helps to resolve are asked about in ip-js, and other filecoin slack channels, often.

If I wanted to use WebRTC in a browser-to-browser app, do I need to setup my own relay node(s)? Or is there a public list of relay IDs?

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

Simple, minimal peer exchange protocol Local Peer Exchange Protocol
5 participants