-
Notifications
You must be signed in to change notification settings - Fork 79
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
Define CIDv0 to allow sha256 multhashes only. #13
Comments
This very much makes sense. |
Although that is true. The hash is the only one thing in CIDv0 that uses a self-describable format, so it should be an issue to support multiple. |
Because CIDv0 isn't a self describing format, the only way to tell if something is a CIDv0 is to check the first two bytes for If we say that CIDv0 addresses should be SHA256 only and never assign a multibase prefix of Q, we'll never have a problem. Note: Accepting non-base58 multihashes would also be a backwards incompatible change due to how we currently decode CIDs... |
Not true. If the CID is a String, then we can check the first two bytes If the CID is a Buffer, then we can check the first byte for the multihash table, if it matches one codec there, then it is a CIDv0. Check how we do the matching in JS: https://github.com/ipld/js-cid/blob/master/src/index.js#L24-L40 |
The JS algorithm is already broken:
If we limit the valid hashes to sha256-256, we can just never assign |
Without wanting to claim what is the right solution for this case, a possible one is to catch this edge case (and edge as in, there had to be a block out there that uses that format, which is quite unlikely and there is no reason for it to exist now that there is CIDv1) is to check the rest of the the parser breaking and then adding the rule check if we are in this scenario.
Good catch. This is definitely an issue introduced when the Identity multihash became a thing. Similar to above, a not perfect solution but probably safe unless someone intentionally wants to break it, is to assume that CIDv0 will not use the Identity multihash. These kinds of things should be included in the migration strategy and why it is good to move everyone to CIDv1 as fast as possible than trying to let CIDsv0 continue to be created with new iterations. |
I just pulled that one out of a hat. There are infinitely many collisions with different hash functions and different bases.
This is the internet...
We'll still have to support these for the foreseeable future, we might as well restrict them as much as possible. I really don't see any reason not to forbid everything but sha256-256 CIDv0s (given that this exists solely for backwards compatibility purposes). go-cid has only ever supported sha256-256 CIDv0 CIDs anyways. |
So, apparently this was supposed to be part of the spec already...
|
All of our code assumes this by checking for the
Qm
prefix. Furthermore, the argument for CIDv0 is backwards compatibility and, as far as I know, nobody is using non-sha256 CIDv0 CIDs at the moment (at least I hope not as our code can't really handle them...).The text was updated successfully, but these errors were encountered: