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: blake3 support #9073

Closed

Conversation

laudiacay
Copy link
Contributor

@laudiacay laudiacay commented Jul 1, 2022

Accepting the relevant blake3 PRs on go-multihash, go-cid, and go-verifcid will make this one ready to go- it's just version bumps that make blake3 work on ipfs. all validation happens in called libraries, the ipfs code more or less doesn't care what hash function multihash is using as far as i can tell.

Closes #8650

@welcome

This comment was marked as resolved.

@Jorropo
Copy link
Contributor

Jorropo commented Jul 1, 2022

pls run go mod tidy

@laudiacay
Copy link
Contributor Author

Closes #8650.

prereqs:
PR 1: multiformats/go-multihash#158
PR 2: ipfs/go-cid#140
PR 3: ipfs/go-verifcid#15

@laudiacay
Copy link
Contributor Author

pls run go mod tidy

won't run until the other version bump PRs are live- will do before merging.

@Jorropo
Copy link
Contributor

Jorropo commented Jul 1, 2022

won't run until the other version bump PRs are live- will do before merging.

Yes mb, what we do most of the time is use replace rules or go get repo@PRCommit (PRCommit is literally just the head commit of your PR) so we can test them together and see the green CI passing.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

We need to be extra careful here

IIUC this PR introduces blake3 with implicit length of 256 bits (32 bytes), however it is very likely at some point in the future we will be asked to add support for longer versions.

This means the bar here is higher than it would be for existing functions: we not only need to guard the behavior at the implicit default length proposed in this PR, but also have a plan for supporting alternative lengths under the same name.

UX challenges

Custom length can be passed to block put via --mhlen. afaik it was intended for digest truncation in contexts where data comes from legacy third party systems, but in the context of blake3 could be used for producing longer digests.

Other commands like ipfs dag put and ipfs add do not have --hash-len. We need to decide if we want to add them as part of this PR, or leave that for future PR, when a need for longer blake3 is raised by our users. I think leaving it for the future is fine, but we should anchor the default behavior by adding sharness tests for dag put and add.

TODO

Things that needs to happen before we can merge this PR:

  • decide if this PR will also add --hash-len to add and dag put commands (only for blake3)
  • add sharness tests listed in Allow blake3 #8650 (comment)

@lidel lidel added need_tests status/blocked Unable to be worked further until needs are met labels Jul 1, 2022
@laudiacay laudiacay force-pushed the bump-cid-dep-versions-for-blake3 branch from 284c70c to d0a9453 Compare July 6, 2022 21:44
@laudiacay
Copy link
Contributor Author

@lidel some relevant discussion here: ipfs/go-verifcid#15 (comment)

also discussed with @aschmahmann in DM about this.

tl;dr: supporting multiple lengths in any manner except literally having a bunch of different multihash codes for them is going to require a major refactor of the multihash library and interface. i don't have time to implement that right now, it's gonna be really painful... but 32-bytes is standard and gets the work that's needed for my usecase (and probably for many others') done.

right now i have it throwing an error on trying to parse blake3 hashes that are not 32 bytes long, no way to create them without somehow subverting the multihash interface, and tests for that behavior in the verifcid library...

assuming I get tests passing... how do people feel about merging it like this and worrying about the interface whenever someone wants to go through and actually refactor multihash because they need something beyond 32 bytes?

@laudiacay
Copy link
Contributor Author

i think we're also pending some sharness. adding now...

@laudiacay laudiacay force-pushed the bump-cid-dep-versions-for-blake3 branch from d0a9453 to ce1f804 Compare July 28, 2022 11:44
@laudiacay laudiacay force-pushed the bump-cid-dep-versions-for-blake3 branch 4 times, most recently from 5ed56c0 to d0a73fa Compare July 28, 2022 13:37
@laudiacay laudiacay force-pushed the bump-cid-dep-versions-for-blake3 branch from d0a73fa to 7d4fe18 Compare July 28, 2022 13:55
@laudiacay laudiacay marked this pull request as ready for review August 1, 2022 15:49
@laudiacay laudiacay requested a review from lidel August 1, 2022 16:06
@lidel lidel changed the title Blake3 support (just a few version bumps) feat: blake3 support Aug 5, 2022
@Jorropo
Copy link
Contributor

Jorropo commented Aug 12, 2022

replaced #9187

@Jorropo Jorropo closed this Aug 12, 2022
@Jorropo
Copy link
Contributor

Jorropo commented Aug 12, 2022

@laudiacay thx for your patches, this has been merged in #9187 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need_tests status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow blake3
3 participants