This repository has been archived by the owner on Jun 20, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
Closes #13 btw |
@titusz wanna review? |
Jorropo
reviewed
Jun 30, 2022
I believe I fixed the versions here- before I merge this, we need to get my fixes into go-multihash in (future) version 0.2.1. doing that now. |
Kubuxu
approved these changes
Jun 30, 2022
mergeable pending this: multiformats/go-multihash#158 |
aschmahmann
reviewed
Jul 1, 2022
aschmahmann
added
the
status/blocked
Unable to be worked further until needs are met
label
Jul 1, 2022
blocked pending confirmation this is ready to go in upstreams |
Jorropo
reviewed
Jul 6, 2022
Jorropo
reviewed
Jul 6, 2022
aschmahmann
reviewed
Jul 6, 2022
Problem: if we do that I’m pretty sure most of the out of the box
applications of cool features of blake3 break. Like for sure we’d at least
have to modify the bao verified implementation to make the final hash
longer then truncated, and it may straight up break other stuff. Happy to
add 64 and 128 variants but I don’t think truncation down to 32 is a great
answer for this application necessarily?
…On Wed, Jul 6, 2022 at 10:00 PM Steven Allen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In validate.go
<#15 (comment)>:
> return ErrBelowMinimumHashLength
}
+ if pref.MhType == mh.BLAKE3 && pref.MhLength != 32 {
We can solve this one of two ways:
1. A refactor. But there's no reason this refactor would need to touch
more than the multihash crate.
2. Make the blake3 hasher output "maximum size" digests, then truncate
(which go-multihash will already do).
What I would do to do so is in github.com/multiformats/go-multihash I
would make blake3's hasher produce hashes of the length you are comparing
it to.
As long as the default blake3 hasher produces, say, 64 or 128 byte hash
digests by default, we don't need to worry. go-ipfs will call Sum(data,
code, length) which will truncate to the desired length. If the requested
digest size exceeds the maximum, we'll get a ErrLenTooLarge error.
For now, I'd be in favor of *not* refactoring unless there's a large
performance penalty to producing larger hashes.
—
Reply to this email directly, view it on GitHub
<#15 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDPMJBURNFU3LLZMBEQHBLVSY24ZANCNFSM52KQP6SA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Please clarify? A short blake3 hash digest of a file is always a prefix of larger hash digests. |
Ok lol my bad- I’ll do that, let’s call the max 128
…On Wed, Jul 6, 2022 at 10:23 PM Steven Allen ***@***.***> wrote:
Please clarify? A short blake3 hash digest of a file is always a prefix of
larger hash digests.
—
Reply to this email directly, view it on GitHub
<#15 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDPMJDDKM2G2WBFTDCY5F3VSY5SDANCNFSM52KQP6SA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
laudiacay
force-pushed
the
add-blake3
branch
3 times, most recently
from
July 28, 2022 13:19
86c9c8c
to
f01c968
Compare
…g blake3 to 32 bytes of length, improving error messages, fixing deprecation warnings
Jorropo
suggested changes
Jul 28, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost LGTM
Merged
Replaced by #16 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See title- this is so that IPFS can support verified streaming feature of BLAKE3, as people mentioned in ipfs/kubo#8650
Not sure if you need to do like, a version bump here, so that it can be integrated into new IPFS builds... Let me know how I can fix that/get in line with y'all's versioning.