Update the spec from the implementation. #14

Stebalien · 2017-08-30T20:42:01Z

CIDv0 only supports SHA256 multihashes.
~~2. In CIDv0, the multibase can be specified but defaults to base58btc.~~

This commit also describes the proper algorithm for decoding CIDs as it's non-obvious.

Fixes #11

Stebalien · 2017-08-30T20:43:50Z

@kevina, @whyrusleeping, @jbenet

Stebalien · 2017-08-30T20:44:48Z

Also, @diasdavid how does this work in JavaScript?

Kubuxu · 2017-08-31T05:21:13Z

README.md

+     * The CID's version is 0.
+   * Otherwise, let `N` be the first varint in `cid`. This is the CID's version.
+     * If `N == 1` (CIDv1):
+       * THe CID's multicodec is the second varint in `cid`


s/THe/The

Here is the algo JS is doing: https://github.com/ipld/js-cid/blob/master/src/index.js#L18-L87 Some different heuristics, it trusts less the length and relies more on value checking.

daviddias · 2017-08-31T08:59:53Z

README.md

@@ -86,10 +86,10 @@ For example:
 ### CIDv0

 CIDv0 is a backwards-compatible version, where:
- the `multibase` is always `base58btc` and implicit (not written)
+- the `multibase` defaults to `base58btc` (if not written)


👍🏽 same in JS, although ipfs/kubo#4143 might become a thing soon

👎🏽 ah wait, read this wrong. CIDv0 is always base58, there is no way to change that.

daviddias · 2017-08-31T09:04:31Z

In CIDv0, the multibase can be specified but defaults to base58btc.

Could you point me to the discussion where this decision 'was made'?

Me, @jbenet and @whyrusleeping talked about this extensively and the agreement was not to convert CIDv0 to CIDv0b (which is what is happening with that proposal), instead convert CIDv0 to CIDv1 if a different base is required to be used.

kevina · 2017-08-31T09:36:36Z

A decision was never "made" it is just what works with go-cid and there is very little harm in allowing a multibase prefix to a CidV0 string and I personally think it will likely be required if we switch to a different base. The reasons it is likely required are discussed in ipfs/go-cid#34, in particular ipfs/go-cid#34 (comment) from @lgierth:

@diasdavid what Kevin and I are thinking about is the next step after receiving a gateway request with a CIDv0-in-CIDv1: content routing and bitswap, where we need the original CIDv0 because that's what provider records are published for, and what's used as a key in the blockstore of the provider nodes. It's kind of a matter of backward compatibility: there will always be nodes who continue to create CIDv0 stuff.

and ipfs/go-cid#34 (comment) by me:

The base used to encode a CID is for the most part an interface issue. The base is is not stored in the key of objects stored as part of a link or in the blockstore. The Cid version and hash used is, on the other hand, part of the key.

You can't retrieve an Cidv0 object if it is reformatted to be Cidv1 and sometimes you may still want to store a Cidv0 key in another object.

daviddias · 2017-08-31T09:39:19Z

@kevina we posted at the same time, see here to learn about the actual issues: ipfs/go-cid#34 (comment)

1. CIDv0 only supports SHA256 multihashes. 2. In CIDv0, the multibase *can* be specified but defaults to base58btc. This commit also describes the proper algorithm for decoding CIDs as it's non-obvious. Fixes multiformats#11

Stebalien · 2017-08-31T19:36:13Z

I've updated it to not allow non-base58 CIDv0s.

whyrusleeping · 2017-09-01T03:03:44Z

No, we shouldn't allow this. Cidv0 is explicitly base58 bare multihash representing a protobuf object.

…

On Thu, Aug 31, 2017, 12:36 PM Steven Allen ***@***.***> wrote: I've updated it to not allow non-base58 CIDv0s. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL4HG0mk3RU3Lsq0ypQ_KLfA0TmZIeSks5sdwsvgaJpZM4PH8qu> .

…ded CIDv0s

Stebalien · 2017-09-01T03:29:52Z

I've now updated the algorithm to explicitly not support non-base58 v0 CIDs. We'll need to fix go-cid (and maybe js-cid) to match this as go-cid, at least, will currently decode multibase CIDv0 CIDs just fine.

kevina · 2017-09-01T03:37:18Z

Can I please ask the reason for explicitly forbidding a multibase encoded Cidv0? Also see this comment ipfs/go-cid#34 (comment) I made on relevant issue.

daviddias · 2017-09-01T03:55:24Z

@Stebalien js-cid limits CIDv0 to base58 encoding

whyrusleeping · 2017-09-01T05:14:05Z

The reason is because cidv0 is a legacy thing, it's very strictly defined to prevent potential confusion or ambiguity. We don't want to support it anymore than is necessary.

…

On Thu, Aug 31, 2017, 8:55 PM David Dias ***@***.***> wrote: @Stebalien <https://github.com/stebalien> js-cid limits CIDv0 to its base58 form — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL4HECqZ5K12qCwlmJDVdBMLauYEMlvks5sd4AugaJpZM4PH8qu> .

daviddias

thank you @Stebalien :)

Stebalien mentioned this pull request Aug 30, 2017

Provide Way of Representing CidV0 objects in an alternative base ipfs/go-cid#34

Closed

Kubuxu reviewed Aug 31, 2017

View reviewed changes

daviddias reviewed Aug 31, 2017

View reviewed changes

Update the spec from the implementation.

bbee2b1

1. CIDv0 only supports SHA256 multihashes. 2. In CIDv0, the multibase *can* be specified but defaults to base58btc. This commit also describes the proper algorithm for decoding CIDs as it's non-obvious. Fixes multiformats#11

Stebalien force-pushed the feat/multibase-cidv0 branch from b3215dc to bbee2b1 Compare August 31, 2017 19:35

Update the CID decoding algorithm to explicitly forbid multibase enco…

177f8d3

…ded CIDv0s

daviddias approved these changes Sep 16, 2017

View reviewed changes

daviddias added the ready label Mar 19, 2018

daviddias approved these changes May 12, 2018

View reviewed changes

daviddias merged commit ef1b200 into multiformats:master May 12, 2018

Stebalien deleted the feat/multibase-cidv0 branch May 15, 2018 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the spec from the implementation. #14

Update the spec from the implementation. #14

Stebalien commented Aug 30, 2017 •

edited by daviddias

Loading

Stebalien commented Aug 30, 2017

Stebalien commented Aug 30, 2017

Kubuxu Aug 31, 2017

daviddias Aug 31, 2017

daviddias Aug 31, 2017 •

edited

Loading

daviddias Aug 31, 2017 •

edited

Loading

daviddias commented Aug 31, 2017 •

edited

Loading

kevina commented Aug 31, 2017

daviddias commented Aug 31, 2017

Stebalien commented Aug 31, 2017

whyrusleeping commented Sep 1, 2017 via email

Stebalien commented Sep 1, 2017

kevina commented Sep 1, 2017

daviddias commented Sep 1, 2017 •

edited

Loading

whyrusleeping commented Sep 1, 2017 via email

daviddias left a comment

Update the spec from the implementation. #14

Update the spec from the implementation. #14

Conversation

Stebalien commented Aug 30, 2017 • edited by daviddias Loading

Stebalien commented Aug 30, 2017

Stebalien commented Aug 30, 2017

Kubuxu Aug 31, 2017

Choose a reason for hiding this comment

daviddias Aug 31, 2017

Choose a reason for hiding this comment

daviddias Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

daviddias Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

daviddias commented Aug 31, 2017 • edited Loading

kevina commented Aug 31, 2017

daviddias commented Aug 31, 2017

Stebalien commented Aug 31, 2017

whyrusleeping commented Sep 1, 2017 via email

Stebalien commented Sep 1, 2017

kevina commented Sep 1, 2017

daviddias commented Sep 1, 2017 • edited Loading

whyrusleeping commented Sep 1, 2017 via email

daviddias left a comment

Choose a reason for hiding this comment

Stebalien commented Aug 30, 2017 •

edited by daviddias

Loading

daviddias Aug 31, 2017 •

edited

Loading

daviddias Aug 31, 2017 •

edited

Loading

daviddias commented Aug 31, 2017 •

edited

Loading

daviddias commented Sep 1, 2017 •

edited

Loading