feat: support encoding/decoding peer IDs as CIDs in text #41

Stebalien · 2019-07-15T22:01:22Z

What:

Supports decoding CIDs (of libp2p keys) as peer IDs.
Continues to encode peer IDs as base58 multihashes by default.
Adds functions for converting between peer IDs and CIDs.
Deprecates IDB58{Decode,Encode}, replacing them with {Decode,Encode}.

Why:

We need to support multibase somehow so we can put peer IDs in domains.
This makes peer IDs fully self describing. That is, the CID itself indicates that it's a hash of a libp2p public key.
It's much easier to upgrade wire protocols than text. This change punts
pids-as-cids on the wire down the road but that's something we can revisit if it ever becomes relevant.

Deviations from that issue:

This retains the current peer ID inlining of ed25519 keys. That turned out to be a nightmare, one I'd like to punt a bit more down the road.
Likewise, this punts the question of embedding the multi-hash algorithm in the public key.

Alternative: Hack this into go-ipfs and make it only valid in /ipns links. Honestly, we can do that first and then merge this later.

hsanjuan · 2019-07-18T08:31:27Z

Continues to encode peer IDs as base58 multihashes by default.

Why don't we change this default now already?

jacobheun · 2019-07-22T18:00:59Z

This change makes sense to me. If we change the default now we wouldn't give the network any time to upgrade. This would cause the node to announce its peer id as a cid hash wouldn't it?

One question, right now we're using the libp2p-key multicodec. Would we get any benefit for being more granular with this to know what type of libp2p key it is?

Stebalien · 2019-07-22T18:58:25Z

@hsanjuan

Why don't we change this default now already?

Compatibility.

@jacobheun

This would cause the node to announce its peer id as a cid hash wouldn't it?

We're not changing the wire format, just the format in text.

Would we get any benefit for being more granular with this to know what type of libp2p key it is?

The key already specifies that internally. libp2p-key just means that we're pointing to a protobuf with the structure {type: int, data: [bytes]}.

jacobheun · 2019-07-23T09:05:53Z

Great, thanks for clarifying. 👍

hsanjuan · 2019-07-23T11:17:52Z

Compatibility.

OK, but is there an upgrade timeline? It says in the future which is not super helpful... also there does not seem to be a way to enforce CID-encoding (other than going manual with ToCid().String() so it is hard to test if anything breaks.

If we are supposed to move from IDB58Encode to Encode all over our code, and libp2p will one day flip the switch for us, then it may just flip the switch now in the Encode function. Why not making the move from IDB58Encode to Encode should be actual opt-in to work with CIDs?

Stebalien · 2019-07-23T15:37:38Z

This is a two step upgrade:

Add support for decoding CIDs as peer IDs everywhere.
Flip the switch and start encoding them that way. When we do that, I'd like to make IDB58Encode also encode peer IDs as CIDs. The only reason I'm deprecating it in favor of Encode is that the function name IDB58Encode will be incorrect.

If we start encoding as CIDs now, peers using the old version won't be able to understand peer IDs encoded by the new version. This won't break the network but it will confuse users.

OK, but is there an upgrade timeline?

Sure. How about 3 months after the last libp2p implementation upgrades and releases?

lidel · 2019-08-19T16:13:38Z

PSA: proposed change to libp2p specs to default text representation to CID in Base32:

RFC 0001: Text Peer Ids as CIDs specs#209 (RFC 0001: Text Peer Ids as CIDs)

Stebalien · 2019-10-02T15:46:31Z

peer/peer.go

@@ -155,10 +158,66 @@ func IDHexDecode(s string) (ID, error) {
 }

 // IDHexEncode returns the hex-encoded multihash representation of the ID.
+//
+// Deprecated: Don't raw-hex encode peer IDs, use base16 CIDs.
 func IDHexEncode(id ID) string {


We should really remove these. But that's a breaking change and we can do that later.

raulk

LGTM. Just two nits about usage of acronyms which I'd like addressed. Since these two methods are in the public API, let's try to be as idiomatic as possible.

raulk · 2019-10-28T17:05:29Z

peer/peer.go

+// ToCid encodes a peer ID as a CID of the public key.
+//
+// If the peer ID is invalid (e.g., empty), this will return the empty CID.
+func ToCid(id ID) cid.Cid {


nit: go style is to capitalise acronyms. We should create an alias cid.CID => cid.Cid.

Suggested change

func ToCid(id ID) cid.Cid {

func ToCID(id ID) cid.Cid {

@raulk I don't think we should introduce new convention here.
All our codebases already use things like NewCidV1 and FromCid.

If we choose to change them to use capitalization, that needs to happen across all repos (in separate, coordinated PRs), otherwise we just introduce noise by mixing conventions.

raulk · 2019-10-28T17:06:44Z

peer/peer.go

+}
+
+// FromCid converts a CID to a peer ID, if possible.
+func FromCid(c cid.Cid) (ID, error) {


Suggested change

func FromCid(c cid.Cid) (ID, error) {

func FromCID(c cid.Cid) (ID, error) {

Stebalien · 2019-10-28T17:32:59Z

I'm fine with those changes but they are inconsistent with the rest of go-ipfs/go-libp2p which we're not going to change as following a style guideline isn't worth the confusion of having two ways to do the same thing.

lidel · 2019-12-09T15:37:25Z

@raulk Would it be ok to rebase it on master and merge as-is? 🙏
I believe we want to keep PRs related to CID changes small and on topic.
See my note in #41 (comment)

raulk · 2019-12-09T19:16:59Z

Go ahead!

License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel · 2019-12-10T12:44:51Z

@raulk @Stebalien I am missing permission to push to this branch – mind adding me, or rebasing and applying below fix yourself?

ERROR: Permission to libp2p/go-libp2p-core.git denied to lidel.
fatal: Could not read from remote repository.

The only conflict with master is go version in .travis.yml, the fix is:

-   - 1.12.x
+   - 1.13.x

Stebalien · 2019-12-10T13:01:48Z

Welcome to the go-libp2p team!

Stebalien mentioned this pull request Jul 15, 2019

Subdomain Gateway ipfs/kubo#6498

Closed

lidel mentioned this pull request Jul 16, 2019

IPNS: should work with case-insensitive identifiers (Base32) ipfs/kubo#5287

Closed

Stebalien requested a review from raulk July 25, 2019 21:37

raulk requested a review from bigs July 29, 2019 17:30

raulk mentioned this pull request Jul 30, 2019

Framework specification: formalising the RFC process libp2p/specs#198

Open

This was referenced Aug 19, 2019

fix: add support for libp2p-key multicodec multiformats/cid-utils-website#14

Merged

RFC 0001: Text Peer Ids as CIDs libp2p/specs#209

Merged

Stebalien mentioned this pull request Oct 1, 2019

Tracking issue for peer IDs as CIDs libp2p/specs#216

Open

22 tasks

Stebalien force-pushed the feat/parse-cid-as-pid branch from d6d45cb to 3c85be0 Compare October 1, 2019 18:50

Stebalien added 2 commits October 2, 2019 08:45

feat: support encoding/decoding peer IDs as CIDs _in text_

dc718fa

ci: bump to go 1.12

0bdeee6

Stebalien force-pushed the feat/parse-cid-as-pid branch from 4cc27af to 0bdeee6 Compare October 2, 2019 15:45

Stebalien commented Oct 2, 2019

View reviewed changes

raulk approved these changes Oct 28, 2019

View reviewed changes

Merge branch 'master' into feat/parse-cid-as-pid

4184c18

License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel changed the title ~~feat: support encoding/decoding peer IDs as CIDs _in text_~~ feat: support encoding/decoding peer IDs as CIDs in text Dec 10, 2019

lidel merged commit ba9101b into master Dec 10, 2019

lidel deleted the feat/parse-cid-as-pid branch December 10, 2019 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support encoding/decoding peer IDs as CIDs in text #41

feat: support encoding/decoding peer IDs as CIDs in text #41

Stebalien commented Jul 15, 2019 •

edited

Loading

hsanjuan commented Jul 18, 2019

jacobheun commented Jul 22, 2019

Stebalien commented Jul 22, 2019

jacobheun commented Jul 23, 2019

hsanjuan commented Jul 23, 2019 •

edited

Loading

Stebalien commented Jul 23, 2019

lidel commented Aug 19, 2019

Stebalien Oct 2, 2019

raulk left a comment

raulk Oct 28, 2019

lidel Dec 9, 2019

raulk Oct 28, 2019

Stebalien commented Oct 28, 2019

lidel commented Dec 9, 2019 •

edited

Loading

raulk commented Dec 9, 2019

lidel commented Dec 10, 2019 •

edited

Loading

Stebalien commented Dec 10, 2019 via email

	func FromCid(c cid.Cid) (ID, error) {
	func FromCID(c cid.Cid) (ID, error) {

feat: support encoding/decoding peer IDs as CIDs in text #41

feat: support encoding/decoding peer IDs as CIDs in text #41

Conversation

Stebalien commented Jul 15, 2019 • edited Loading

hsanjuan commented Jul 18, 2019

jacobheun commented Jul 22, 2019

Stebalien commented Jul 22, 2019

jacobheun commented Jul 23, 2019

hsanjuan commented Jul 23, 2019 • edited Loading

Stebalien commented Jul 23, 2019

lidel commented Aug 19, 2019

Stebalien Oct 2, 2019

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

raulk Oct 28, 2019

Choose a reason for hiding this comment

lidel Dec 9, 2019

Choose a reason for hiding this comment

raulk Oct 28, 2019

Choose a reason for hiding this comment

Stebalien commented Oct 28, 2019

lidel commented Dec 9, 2019 • edited Loading

raulk commented Dec 9, 2019

lidel commented Dec 10, 2019 • edited Loading

Stebalien commented Dec 10, 2019 via email

Stebalien commented Jul 15, 2019 •

edited

Loading

hsanjuan commented Jul 23, 2019 •

edited

Loading

lidel commented Dec 9, 2019 •

edited

Loading

lidel commented Dec 10, 2019 •

edited

Loading