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

Making the /p2p protocol type-safe #73

Closed
thomaseizinger opened this issue Dec 16, 2022 · 5 comments · Fixed by #83
Closed

Making the /p2p protocol type-safe #73

thomaseizinger opened this issue Dec 16, 2022 · 5 comments · Fixed by #83
Milestone

Comments

@thomaseizinger
Copy link
Contributor

The /p2p protocol can only be followed by a PeerId: https://github.com/libp2p/specs/blob/master/addressing/README.md#the-p2p-multiaddr

As per spec, a PeerId can only be a multihash with either SHA256 or the identity hash in case the encoded public key is less than 42 bytes.

Currently, /p2p exposes a Multihash which is more general than that.

I see several options of how we can improve this situation:

  1. Extract the PeerId type from rust-libp2p into a dedicated crate and have multiaddr depend on that
  2. Move the entire identity module of libp2p-core into its own crate (keys + PeerId)
  3. Move the rust-libp2p PeerId type into multiaddr
  4. Define a minimal PeerId type within multiaddr that encodes the above invariants

All of the above have their pros and cons.

Extracting PeerId into its own crate feels a bit odd because it would be a very small crate. On the other hand, it would encode a very important concept in a concise form so it may be worth it.

The next step up would be a crate that encapsulates everything around keys in libp2p into its own crate, i.e. libp2p-identity. That is basically this module + the peer_id module.

Personally, I'd be in favor of option (2). I think it makes sense to break out this part into a separate crate. We can heavily feature-flag that one to the point where the multiaddr crate itself only depends on the bits that define the PeerId and doesn't come with any other dependencies.

Input welcome!

cc @mxinden @dignifiedquire

@thomaseizinger
Copy link
Contributor Author

Cross referencing a discussion about API stability and extracting more crates in rust-libp2p: libp2p/rust-libp2p#3072 (comment)

@thomaseizinger
Copy link
Contributor Author

(2) also makes sense when you consider that keys and PeerId are described in a single document in the spec: https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md

@dignifiedquire
Copy link
Member

My preference would be (2)

@mxinden
Copy link
Member

mxinden commented Dec 30, 2022

I am in favor of making /p2p type safe. I am indifferent on option 1 - 3, i.e. I am in support for all of them. I would consider (4) a compromise worth avoiding.

@thomaseizinger
Copy link
Contributor Author

I will likely push (2) forward because it goes well with the planned modularization of libp2p-core.

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 a pull request may close this issue.

3 participants