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

Use newer PeerID as CID representation #976

Open
aschmahmann opened this issue Jul 2, 2020 · 5 comments
Open

Use newer PeerID as CID representation #976

aschmahmann opened this issue Jul 2, 2020 · 5 comments
Labels
kind/enhancement A net-new feature or improvement to an existing feature status/ready Ready to be worked

Comments

@aschmahmann
Copy link
Collaborator

aschmahmann commented Jul 2, 2020

As described in the peerID spec there are two ways to encode peerIDs into text.

  1. As a base58 encoded multihash
  2. Embed the peerID into a CID where the codec is libp2p-key and then encode using multibase

Currently the Encode(peerID) function returns the legacy base58 encoding. It would be nice to start using the CID representation as it will mean that the default representation of Ed25519 keys will be as CIDs. Some implications include:

  1. Existing CID tooling could be used to convert PeerIDs into relevant representations (e.g. a lowercase base36 representation for web browsers)
  2. Older applications that mistakenly assumed PeerIDs were CIDs will still actually work, even with Ed25519 keys

We'd have to choose what base to encode the CIDs into by default (cc ipfs/specs#247). Two (of many) viable options here are:

  • Encode all PeerIDs as CIDv1s using a standard encoding
    • Upside: Consistent representation, can choose nice defaults like base36
    • Downside: Could be problematic for applications that interact with libp2p implementations that don't yet have CID support
  • Encode CIDv0 PeerIDs as a standard CIDv0 which is implictly base58, and everything else as CIDv1
    • Upside: Doesn't change anything for applications that are using CIDv0 PeerIDs
    • Downside: Conversion of CIDv0s required if users want to use them in lowercase-only environments. Non-consistent base encoding between standard RSA and Ed25519 keys.

Note: Changing the defaults here may cause problems for applications that interact with libp2p implementations that don't yet have CID support AND are using Ed25519 keys by default.

@lidel has added some context on this here: ipfs/kubo#6916 (comment)

^@raulk @Stebalien

@aschmahmann aschmahmann added the kind/enhancement A net-new feature or improvement to an existing feature label Jul 2, 2020
@Stebalien
Copy link
Member

Stebalien commented Jul 2, 2020 via email

@aschmahmann
Copy link
Collaborator Author

We'll need to support the new encoding in go-multiaddr.

go-multiaddr has no dependence on PeerID (or transitively CID) at all, which it really needs to per the spec in order to support /p2p multiaddrs. We should probably fix this ASAP, likely by depending on go-libp2p-core since PeerIDs are defined there.

At the moment, I think the best approach is to make any relevant changes
in go-ipfs only.

What would you think about adding some flag or function that can define the default String() representation of a PeerID? Otherwise either go-ipfs will end up with b36 representations of peerIDs in some places and b58 in others.

In particular I'm concerned about logging where the logs will all get b58 peerIDs, but the CLI commands will output b36. How would we deal with logging peer IDs in libp2p components like the DHT?

@Stebalien
Copy link
Member

go-multiaddr has no dependence on PeerID (or transitively CID) at all, which it really needs to per the spec in order to support /p2p multiaddrs. We should probably fix this ASAP, likely by depending on go-libp2p-core since PeerIDs are defined there.

It's just that that dependency is a little backwards. I wonder if this is a case where a little bit of copy/paste would go a long way. All we need to do is:

  1. Verify that binary peer IDs have the form <varint><length-prefix><bytes>.
  2. Convert to/from the textual representation (which we can special case).

@Stebalien
Copy link
Member

What would you think about adding some flag or function that can define the default String() representation of a PeerID? Otherwise either go-ipfs will end up with b36 representations of peerIDs in some places and b58 in others.

I generally prefer to avoid flags like this but it's up to you. I think we'll need to let users specify the desired encoding anyways so I'm not sure how much it would help. We'll likely need an encoding system like we have for CIDs.

In particular I'm concerned about logging where the logs will all get b58 peerIDs, but the CLI commands will output b36. How would we deal with logging peer IDs in libp2p components like the DHT?

Ah, I see. I think we can just "deal" with this for now.

@ribasushi
Copy link

@aschmahmann this seems like it is close-able... yes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

4 participants