search

libp2p / go-libp2p

libp2p implementation in Go
MIT License
6.12k stars 1.08k forks source link

Use newer PeerID as CID representation #976

Open aschmahmann opened 4 years ago

aschmahmann commented 4 years ago

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 https://github.com/ipfs/specs/issues/247). Two (of many) viable options here are:

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: https://github.com/ipfs/go-ipfs/issues/6916#issuecomment-652099251

^@raulk @Stebalien

Stebalien commented 4 years ago
  1. We'll need to make sure the new encoding is supported in all implementations first.
  2. We'll need to support the new encoding in go-multiaddr.

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

aschmahmann commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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