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

Implement the rendezvous protocol #2107

Merged
merged 250 commits into from
Sep 7, 2021
Merged

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jun 19, 2021

This is an attempt of implementing the rendezvous protocol. This also fully includes #2081. The first commits are pretty messy but later down the track, they are quite atomic in case you prefer looking at things patch-by-patch.

A couple of design decisions that are worth mentioning:

  • In-memory storage only. I see this more as a temporary thing to get an MVP out the door. Whether we want persistent storage in the form of an SQL database can be discussed in a follow-up. For example, I am not fully sold that we need it due to the ephemeral nature of registrations.
  • PoW is implemented as per the drafted extension to the protocol. The extension itself is still very much WIP, so any feedback is highly appreciated.
  • PoW is done on a separate thread. I did this because depending on the requested difficulty and machine power, performing the PoW can take several seconds and doing it as part of a task would essentially be blocking. I am not super happy with spawning threads inside the handler though because it makes the code non-portable to platforms like wasm which would be a shame because I think all the rest is.
  • We only support one inbound and one outbound substream to a specific peer at any point.
  • The test suite and substream management may appear a bit over-engineered but I wanted to experiment with some designs that might be generalizable for other protocols to make implementing protocols easier.

Things that I think we need to discuss before this can be merged:

  1. How should we handle keep-alive in the handler? This is a part of rust-libp2p that I am not super familiar with so any guidance here would be much appreciated :)
  2. Usage of tokio for timers. I know that rust-libp2p usually prefers async-std so we will need to come to an agreement of how this can be solved.
  3. Do we want to have different modes for the behaviour such that a client could disallow registrations of other peers or should every node always also act as a rendezvous node?
  4. Is the split between behaviour and handler good? Currently, the ProtocolsHandler is quite "smart" and handles all the validation (including PoW!) and substream state management. This felt right to me because it allows the behaviour to deal with decision-making only.
  5. Do we want an "auto-renew" behaviour for registrations? As a behaviour that registers with a rendezvous point, we have all the necessary information to automatically renew the registration as it comes close to expiry. This might be something that all implementations want and hence should be provided together with the protocol. I am thinking of extending the register API with an argument Renew::{Never, AfterRelativeElapsed(Precent)} where AfterRelativeElapsed describes the relative time elapsed of the effective ttl after which we should renew (i.e. 50% of TTL elapsed).
  6. Registering with a rendezvous node currently assumes that we want to register all of our external addresses as reported by other behaviours. This is quite opinionated but I like how it makes the interface very simple! I think we should change this so that we the user can choose between registering external addresses or providing a fixed list. Possibly in a similar way as the auto-renew functionality suggested above where the user can pass an enum Addresses::{AllExternal, Given(Vec<Multiaddress>)}

For (5) and (6), consider using a builder for a NewRegistration struct to avoid explosion of parameters.

Things we are still planning on doing (in order of importance):

  • Transfer TODOs / FIXMEs from the code to this list
  • Add examples for a client, a server and an application that registers itself with a rendezvous point on startup
  • Fix panic on subsequent substream being injected inside inject_fully_negotiated_*
  • Fix panic on of "invalid handler state" in inject_event
  • Rip out PoW (see [Rendezvous] Draft proof-of-work requirement specs#334 (comment))
  • Implement limit for DISCOVER requests
  • Emit event for expired registrations
  • Validate that incoming registration is from requesting node
  • Have repeated registrations refresh existing registrations instead of duplicating them
  • Replace tokio::time with wasm-timer
  • Implement connection_keep_alive logic
  • Handle bad cookie being sent from client
  • Check the spec again and implement things like max namespace length etc (consider doing this by creating new-types for namespace and ttl to be more descriptive)
  • Implement basic connection handling
    • allow kicking off a registration whilst not being connected yet, similar to libp2p-request-response
    • take Discovered responses and make the information available via addresses_of_peer to help with connection management
  • Audit dependencies and remove stuff like thiserror
  • Client-only mode to disallow incoming connections
    • Clarify with spec how clients should handle this (disallowing upgrades vs sending NotAvailable error?)
  • Add proptests for converting between wire messages and codec messages
  • Allow the user to filter the list of external addresses being registered in Rendezvous::register
  • Extend spec with max message size?
  • Clarify why we need the id field in Unregister message ([Rendezvous] How exactly does Unregister work? specs#335)
  • Extend PeerRegistered with registration object
  • Clarify with spec if there is an UnregisterResponse ([Rendezvous] How exactly does Unregister work? specs#335)

rishflab and others added 30 commits May 25, 2021 00:16
Pull in all the code that creates and connects the test swarms that include the behaviour to be tested.
A first initial test stub is there, but no test functionality added yet.
@thomaseizinger
Copy link
Contributor Author

Can you confirm that using the same protocol string in two different handlers works if one is used for inbound and one for outbound?

I can't confirm, given that, if I am not mistaken, none of our existing protocols cover this case. E.g. #2059 uses two protocols (hop and stop), thus never running into this case.

That said, nothing comes to my mind that would prevent two handlers using the same protocol string.

Okay, I'll see if I can find the bug in my implementation then :)

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

What do you see as critical before we can merge this?

Couple of comments below. Nothing big.

Following up on #2107 (comment), whether the swarm should hand out signed peer records, or whether each behaviour should sign their own and thus own a private key:

I would suggest we proceed here as is, with libp2p-rendezvous signing the record. Once another behaviour has the need for signed peer records, I think we should consider once more moving the logic to libp2p-swarm or libp2p-core.

protocols/rendezvous/Cargo.toml Outdated Show resolved Hide resolved
protocols/rendezvous/src/client.rs Show resolved Hide resolved

pub fn from_wire_encoding(mut bytes: Vec<u8>) -> Result<Self, InvalidCookie> {
// check length early to avoid panic during slicing
if bytes.len() < 16 {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, thanks. I somehow assumed other entities in a network to treat this in some other way than an opaque list of bytes.

protocols/rendezvous/src/client.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger force-pushed the rendezvous branch 2 times, most recently from 97f6e46 to 3359875 Compare September 1, 2021 04:28
@thomaseizinger
Copy link
Contributor Author

@mxinden This is ready for another review!

I think I addressed all the feedback and managed to get everything working with a client/server split. Also dropped UUID to make wasm support easier.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the overall work and the recent follow-ups.

Would you mind adding an entry to core/CHANGELOG.md, initialize protocols/rendezvous/CHANGELOG.md and reference it in ./CHANGELOG.md?

- Changelogs
- No default features of `libp2p-core`
- Cargo metadata update
@thomaseizinger
Copy link
Contributor Author

This looks good to me. Thanks for the overall work and the recent follow-ups.

Would you mind adding an entry to core/CHANGELOG.md, initialize protocols/rendezvous/CHANGELOG.md and reference it in ./CHANGELOG.md?

Done!

@@ -109,7 +109,7 @@ where
/// A connection may close if
///
/// * it encounters an error, which includes the connection being
/// closed by the remote. In this case `error` is `Some`.
/// closed by the remote. In this case `error` is `ome`.
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
/// closed by the remote. In this case `error` is `ome`.
/// closed by the remote. In this case `error` is `Some`.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #2218. Unfortunately I can not apply suggestions on a branch on the comit-network repository.

@mxinden mxinden merged commit adcfdc0 into libp2p:master Sep 7, 2021
dvc94ch added a commit to dvc94ch/rust-libp2p that referenced this pull request Sep 14, 2021
* protocols/gossipsub: Fix inconsistency in mesh peer tracking (libp2p#2189)

Co-authored-by: Age Manning <Age@AgeManning.com>

* misc/metrics: Add auxiliary crate to record events as OpenMetrics (libp2p#2063)

This commit adds an auxiliary crate recording protocol and Swarm events
and exposing them as metrics in the OpenMetrics format.

* README: Mention security@ipfs.io

* examples/: Add file sharing example (libp2p#2186)

Basic file sharing application with peers either providing or locating
and getting files by name.

While obviously showcasing how to build a basic file sharing
application, the actual goal of this example is **to show how to
integrate rust-libp2p into a larger application**.

Architectural properties

- Clean clonable async/await interface ([`Client`]) to interact with the
network layer.

- Single task driving the network layer, no locks required.

* examples/README: Give an overview over the many examples (libp2p#2194)

* protocols/kad: Enable filtering of (provider) records (libp2p#2163)

Introduce `KademliaStoreInserts` option, which allows to filter records.

Co-authored-by: Max Inden <mail@max-inden.de>

* swarm/src/protocols_handler: Impl ProtocolsHandler on either::Either (libp2p#2192)

Implement ProtocolsHandler on either::Either representing either of two
ProtocolsHandler implementations.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

* *: Make libp2p-core default features optional (libp2p#2181)

Co-authored-by: Max Inden <mail@max-inden.de>

* core/: Remove DisconnectedPeer::set_connected and Pool::add (libp2p#2195)

This logic seems to be a leftover of
libp2p#889 and unused today.

* protocols/gossipsub: Use ed25519 in tests (libp2p#2197)

With f2905c0 the secp256k1 feature is
disabled by default. Instead of enabling it in the dev-dependency,
simply use ed25519.

* build(deps): Update minicbor requirement from 0.10 to 0.11 (libp2p#2200)

Updates the requirements on [minicbor](https://gitlab.com/twittner/minicbor) to permit the latest version.
- [Release notes](https://gitlab.com/twittner/minicbor/tags)
- [Changelog](https://gitlab.com/twittner/minicbor/blob/master/CHANGELOG.md)
- [Commits](https://gitlab.com/twittner/minicbor/compare/minicbor-v0.10.0...minicbor-v0.11.0)

---
updated-dependencies:
- dependency-name: minicbor
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): Update salsa20 requirement from 0.8 to 0.9 (libp2p#2206)

* build(deps): Update salsa20 requirement from 0.8 to 0.9

Updates the requirements on [salsa20](https://github.com/RustCrypto/stream-ciphers) to permit the latest version.
- [Release notes](https://github.com/RustCrypto/stream-ciphers/releases)
- [Commits](RustCrypto/stream-ciphers@ctr-v0.8.0...salsa20-v0.9.0)

---
updated-dependencies:
- dependency-name: salsa20
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

* *: Bump pnet to v0.22

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Max Inden <mail@max-inden.de>

* *: Dial with handler and return handler on error and closed (libp2p#2191)

Require `NetworkBehaviourAction::{DialPeer,DialAddress}` to contain a
`ProtocolsHandler`. This allows a behaviour to attach custom state to its
handler. The behaviour would no longer need to track this state separately
during connection establishment, thus reducing state required in a behaviour.
E.g. in the case of `libp2p-kad` the behaviour can include a `GetRecord` request
in its handler, or e.g. in the case of `libp2p-request-response` the behaviour
can include the first request in the handler.

Return `ProtocolsHandler` on connection error and close. This allows a behaviour
to extract its custom state previously included in the handler on connection
failure and connection closing. E.g. in the case of `libp2p-kad` the behaviour
could extract the attached `GetRecord` from the handler of the failed connection
and then start another connection attempt with a new handler with the same
`GetRecord` or bubble up an error to the user.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

* core/: Remove deprecated read/write functions (libp2p#2213)

Co-authored-by: Max Inden <mail@max-inden.de>

* protocols/ping: Revise naming of symbols (libp2p#2215)

Co-authored-by: Max Inden <mail@max-inden.de>

* protocols/rendezvous: Implement protocol (libp2p#2107)

Implement the libp2p rendezvous protocol.

> A lightweight mechanism for generalized peer discovery. It can be used for
bootstrap purposes, real time peer discovery, application specific routing, and
so on.

Co-authored-by: rishflab <rishflab@hotmail.com>
Co-authored-by: Daniel Karzel <daniel@comit.network>

* core/src/network/event.rs: Fix typo (libp2p#2218)

* protocols/mdns: Do not fire all timers at the same time. (libp2p#2212)

Co-authored-by: Max Inden <mail@max-inden.de>

* misc/metrics/src/kad: Set query_duration lowest bucket to 0.1 sec (libp2p#2219)

Probability for a Kademlia query to return in less than 100 milliseconds
is low, thus increasing the lower bucket to improve accuracy within the
higher ranges.

* misc/metrics/src/swarm: Expose role on connections_closed (libp2p#2220)

Expose whether closed connection was a Dialer or Listener.

* .github/workflows/ci.yml: Use clang 11 (libp2p#2233)

* protocols/rendezvous: Update prost (libp2p#2226)

Co-authored-by: Max Inden <mail@max-inden.de>

* *: Fix clippy warnings (libp2p#2227)

* swarm-derive/: Make event_process = false the default (libp2p#2214)

Co-authored-by: Max Inden <mail@max-inden.de>

Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Age Manning <Age@AgeManning.com>
Co-authored-by: Ruben De Smet <ruben.de.smet@rubdos.be>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: rishflab <rishflab@hotmail.com>
Co-authored-by: Daniel Karzel <daniel@comit.network>
Co-authored-by: David Craven <david@craven.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants