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

Present codebase does not allow opening root-less .car files #26

Open
ribasushi opened this issue Mar 25, 2020 · 8 comments
Open

Present codebase does not allow opening root-less .car files #26

ribasushi opened this issue Mar 25, 2020 · 8 comments

Comments

@ribasushi
Copy link

https://github.com/ipld/go-car/blob/v0.0.4/car.go#L120-L122 seems somewhat superfluous. In the tests for ipfs/kubo#7011 I worked around this by sticking an "empty CID" \x01\x55\x00\x00 into the root array, but there got to be a better way...?

If there is compelling argument to not accepting root-less .car files: please clarify it in this issue.

@ribasushi
Copy link
Author

@whyrusleeping @mikeal let me know your thoughts on just removing this limitation ( strictly on read )

@rvagg
Copy link
Member

rvagg commented Mar 26, 2020

https://github.com/ipld/specs/blob/master/block-layer/content-addressable-archives.md#number-of-roots

I allowed for rootless in the JavaScript version, but the problem we have now is that there's existing code in the wild that assumes it's receiving a root because of that line of code, so zero-roots disallowed is at the level of an almost-spec feature and maybe should be considered locked in for CARv1.

@ribasushi
Copy link
Author

@rvagg fair enough. Do you have thoughts on

I worked around this by sticking an "empty CID" \x01\x55\x00\x00 into the root array

?

@rvagg
Copy link
Member

rvagg commented Mar 31, 2020

That CID isn't valid in JavaScript unfortunately, there's a mismatch between go-multihash and js-multihash.

> new CID(Buffer.from([0x01, 0x55, 0x00, 0x00]))
Uncaught Error: multihash too short. must be > 3 bytes.

Maybe [0x01, 0x55, 0x00, 0x01, 0x00]? a length=1 identity multihash using raw? I'll put in a PR to js-multihash to see whether this is something that should be done there or go-multihash. This isn't going to propagate quickly, though, so your shorter version might not be safe for a while.

What's the use-case with zero-root CARs here. Is this just for constructing test fixtures? Or is it for something that would be used in production? I'm sure you've gone over this before but it's not stuck in my head, sorry.

Maybe it's fine to just adopt this as a "whatever works" approach for CARv1 and get this fixed in CARv2 where we could explicitly state that zero roots is OK. It could even go in as a recommendation for CARv1 if you want zero roots.

@ribasushi
Copy link
Author

No worries, lots of concepts flying around, keeping track of all is HARD.

The use case for root-less cars is streaming out a dag into a car. You can not possibly know the root of that dag until the very end.

new CID(Buffer.from([0x01, 0x55, 0x00, 0x00]))
Uncaught Error: multihash too short. must be > 3 bytes.

That looks like an outright bug and should be fixed regardless

@rvagg
Copy link
Member

rvagg commented Mar 31, 2020

multiformats/js-multihash#75

@rvagg
Copy link
Member

rvagg commented Mar 31, 2020

discussion about zero roots "hack" vs "fix": ipld/specs#255

@rvagg
Copy link
Member

rvagg commented Apr 1, 2020

Fixed and released already!

> new CID(Buffer.from([0x01, 0x55, 0x00, 0x00])) 
CID(bafkqaaa)

Still unsure if this is a great idea or not. Starting to lean toward removing the zero-roots restriction here instead now.

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

No branches or pull requests

2 participants