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

Add WithTrustedCar() reader option #381

Merged
merged 4 commits into from
Mar 6, 2023
Merged

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Feb 27, 2023

Attempt to make CAR traversal a little bit faster by not forcing users to hash every single block in a CAR.

Attempt to make CAR traversal a little bit faster by not forcing users to hash every single in a CAR.
@rvagg
Copy link
Member

rvagg commented Feb 28, 2023

I think this would probably be better as an option that can be passed to NewBlockReader(), then we can use it elsewhere and then we don't need the extra method. Some variation on Trusted would probably be appropriate and match the LinkSystem language: https://github.com/ipld/go-ipld-prime/blob/02cafab33a85414419e990e143238719979445be/linking/types.go#L38

I think that this might be the only section read path that currently checks block hash matches CID; which either seems like an oversight or an unfortunate waste of resources--whichever view you want to take! Having the ability to turn on validation for each of the interfaces might be the best way to deal with the inconsistency, we'd just have to change the contract on one side of this (i.e. default to untrusted and then make blockstore and storage start validating unless told otherwise? or default to trusted and make this method skip validating unless it's told otherwise).

@willscott what's your take?

@willscott
Copy link
Member

agree in that i suspect you won't be mixing these two within a read so you can probably do it as you create the iterator rather than a separate method on the iterator

@hsanjuan
Copy link
Contributor Author

Like this? Agree it is better as an Option.

@rvagg
Copy link
Member

rvagg commented Mar 6, 2023

👌 very nice

Would you mind adding a test for this in block_reader_test.go? If you look in TestMaxSectionLength you'll see the manual construction of a basic CAR; I'd suggest just doing the same, but writing bytes in the section that doesn't match the CID and then running two read variations of the test (like in TestMaxSectionLength) - one that reads the block as-is and one that fails to read due to CID mismatch.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Mar 6, 2023

@rvagg there you go...

@hsanjuan hsanjuan changed the title Add NextInsecure() method Add WithTrustedCar() reader option Mar 6, 2023
v2/block_reader_test.go Outdated Show resolved Hide resolved
v2/block_reader_test.go Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌

@rvagg rvagg merged commit e9a77cb into ipld:master Mar 6, 2023
@hsanjuan hsanjuan deleted the feat/next-insecure branch March 7, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

3 participants