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

feat: support v0 identity cids #91

Closed
wants to merge 1 commit into from

Conversation

achingbrain
Copy link
Member

When dealing with PeerIDs we commonly encode them as v0 CIDs using base58btc encoded multihashes with the identity hash.

The change here is to support 1 prefixed CIDv0 as a special case (e.g. the identity hash) similar to how we support Q prefixed CIDv0 as a special case (e.g. sha2-256).

This lets us parse and validate peer IDs out of multiaddrs, among other things.

Fixes #90

When dealing with PeerIDs we commonly encode them as v0 CIDs using
base58btc encoded multihashes with the identtiy hash.

The change here is to support `1` prefixed CIDv0 as a special case
(e.g. the identity hash) similar to how we support `Q` prefixed
CIDv0 as a special case (e.g. sha2-256).

Fixes #90
@achingbrain achingbrain requested review from Gozala and rvagg June 2, 2021 11:41
achingbrain added a commit to libp2p/js-libp2p-delegated-peer-routing that referenced this pull request Jun 2, 2021
Depends on:

- [ ] multiformats/js-multiformats#91

BREAKING CHANGE: uses the CID class from the new multiformats module
@@ -331,7 +331,8 @@ export class CID {

let version = next()
let codec = DAG_PB_CODE
if (version === 18) { // CIDv0

if (version === 18 || version === 0) { // CIDv0 sha2-256 or identity
Copy link

@aschmahmann aschmahmann Jun 2, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

PeerIDs are not CIDs by spec

Hmm: https://github.com/libp2p/specs/blob/master/RFC/0001-text-peerid-cid.md

The peer-id module carries a lot of crypto baggage with it that makes it unsuitable for use in the http client as it's too large, to the point that we don't use it in the http client, only in in-process or daemon js-ipfs/js-libp2p.

For whatever reason we decided instead to pass peer IDs around as CIDs as a 'lighter weight' version of peer-id so it has methods to convert to and from a CID (v0 identity or v1 libp2p-key).

Maybe it shouldn't but that's a much bigger scope change than upgrading multiformats to get the new @ipld/dag-cbor @ipld/dag-pb module goodness.

All this PR really does is add a feature that was in the cids module but is missing from this one.

Copy link

@aschmahmann aschmahmann Jun 2, 2021

Choose a reason for hiding this comment

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

Hmm: https://github.com/libp2p/specs/blob/master/RFC/0001-text-peerid-cid.md

This is a spec detailing how to encode peerIDs as CIDs in text, which is a subset of the peerID spec (e.g. peerIDs are represented as multihashes, not CIDs, in byte encoding and peerIDs are also representable as base58btc multihashes in text).

All this PR really does is add a feature that was in the cids module but is missing from this one.

🤷‍♀️ if the previous implementation (js-cid) wasn't spec compliant and the idea is you want to make this one match the other one then I guess that's up to you + others familiar with the js codebase. You should probably file an issue then to make a peer-id type that you can use so that you can make the CID type spec compliant again.

achingbrain added a commit to libp2p/js-libp2p-delegated-content-routing that referenced this pull request Jun 2, 2021
Depends on:

- [ ] multiformats/js-multiformats#91

BREAKING CHANGE: uses the CID class from the new multiformats module
achingbrain added a commit to ipfs-shipyard/is-ipfs that referenced this pull request Jun 2, 2021
Depends on:

- [ ] multiformats/js-multiformats#91

BREAKING CHANGE: uses the CID class from the new multiformats module
@rvagg
Copy link
Member

rvagg commented Jun 3, 2021

Weird, I didn't know this was a thing. As @aschmahmann says this is not in the spec, it's explicitly always a sha2-256: https://github.com/multiformats/cid#cidv0

the multihash is written as is but is always a full (length 32) sha256 hash.

go-cid is pretty explicit about this: https://github.com/ipfs/go-cid/blob/979bf3fb8572224c2b2fbfaf153f94b98734807c/cid.go#L646-L667

js-cid explicitly won't let you turn a CID that isn't sha2-256 into a CIDv0: https://github.com/multiformats/js-cid/blob/13647819ec1256bab6bd0e885a304d74bce138d6/src/index.js#L242-L244

It doesn't look like this was completely intentional in js-cid either, it just hands off to multihashes to decode whatever's there and assumes it's going to be correct: https://github.com/multiformats/js-cid/blob/13647819ec1256bab6bd0e885a304d74bce138d6/src/index.js#L125

This means that you can even do silly things like this:

c = new CID('Yn1VgDzJSXCEaTcxeUfKdU13EeQrF1VHCjLDbTFntRtQQmHF4WZVjUCC')
> c.version
0
> c.multibaseName
'base58btc'
> c.codec
'dag-pb'
> multihash.decode(c.multihash)
{
  code: 45584,
  name: 'blake2b-128',
  length: 37,
  digest: <Buffer 08 02 12 21 03 36 7f 38 08 9c 1b aa 2b de 5e 58 f7 0c 49 09 7a 9b 16 af 46 33 c2 cd 9a 32 c1 03 9d 5d 05 6c 65>
}

I could do PJ3dBWyYd2N1bUNvCzrWGGCeQXn45H4bohbCdeVWYXXoCyc1a16Sx as a murmur3-128 CID and even 8G7hRq6CZgo2Z3seEjHYf2mfGPHJQJdDSk3HXVLRasRAQWRNPukTaeY as a sha2-256-trunc254-padded Filecoin CommP hash CIDv0. I can even do new CID('4jdwdZ') for a single zero byte sha2-256-trunc254-padded dag-pb CIDv0, fun fun! But.. probably not correct.

How much pain would there be in removing CID from the Peer ID path? Can they just be a multihash instead? Or is it tightly tied up in the system? This seems to be one of those things that was done because it happened to be possible in the code, but it probably shouldn't have.

@achingbrain
Copy link
Member Author

Wild. Anyway this shouldn't be merged as a string v0 identity CID isn't a CID, it's a multibase encoded multihash with the identity multicodec so we shouldn't treat it like a CID. Phew.

How much pain would there be in removing CID from the Peer ID path?

I think most of it would be swapping out use of PeerID.createFromCID for PeerID.createFromB58String and constraining the input types accordingly - we seem to pass strings into createFromCID as the js-cids constructor supports that when we should really be using the other method.

I don't think we can remove .createFromCID entirely as from https://github.com/libp2p/specs/blob/master/RFC/0001-text-peerid-cid.md we can expect to encounter Peer IDs as strings of v1 CIDs with the libp2p-key codec, so we'd create a CID from the string, then a PeerID from the CID.

@achingbrain achingbrain closed this Jun 3, 2021
@achingbrain achingbrain deleted the fix/support-v0-identity-cids branch June 3, 2021 10:22
@Gozala
Copy link
Contributor

Gozala commented Jun 14, 2021

The peer-id module carries a lot of crypto baggage with it that makes it unsuitable for use in the http client as it's too large, to the point that we don't use it in the http client, only in in-process or daemon js-ipfs/js-libp2p.

We should probably address this

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.

How to parse a base58 implicit CID with the identity hash?
4 participants