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 compatibility note on sha3 as alias for sha3-512 #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

candeira
Copy link
Contributor

No description provided.

@@ -83,6 +83,9 @@ code name
# 0x14 formerly had the name "sha3", now deprecated
```

## Compatibility note

Implementations should make sure keep the string code `sha3` as an alias for `sha3-512` so that `encode('sha3', digest) == encode('sha3-512', digest)`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is right. can someone else comment on this? cc @lgierth @whyrusleeping

Copy link
Member

Choose a reason for hiding this comment

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

Should be: "...should make sure to keep the..."

Content sounds right, if 512 is the default bit-size of the digest. But I'm not sure where that assumption is coming from, as there are 3 other SHA-3 hashing functions, and two more if you count the XOFs.

Copy link
Member

Choose a reason for hiding this comment

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

@RichardLitt the comment here is that there are multiple variants of the hash function, not the output size. similar to how sha2-256 and sha2-512 are different functions. (the default output size matches the classification)

Copy link
Member

Choose a reason for hiding this comment

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

Unless I misread the spec correctly, they differ mainly in the size of the digest. What I'm not sure about is whether or not sha3 should by default mean sha3-512, as opposed to one of the other five (like sha3-256).

Copy link
Member

Choose a reason for hiding this comment

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

This looks correct to me.

Copy link
Member

Choose a reason for hiding this comment

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

See https://en.m.wikipedia.org/wiki/SHA-3 sections: instances (show
different postfixes) and examples (show totally different hash values for
same input, not just a truncation of same value)

On Wed, Aug 3, 2016 at 16:57 Richard Littauer [email protected]
wrote:

In README.md
#22 (comment):

@@ -83,6 +83,9 @@ code name

0x14 formerly had the name "sha3", now deprecated


+## Compatibility note
+
+Implementations should make sure keep the string code `sha3` as an alias for `sha3-512` so that `encode('sha3', digest) == encode('sha3-512', digest)`.

Unless I misread the spec correctly, they differ mainly in the size of the
digest. What I'm not sure about is whether or not sha3 should by default
mean sha3-512, as opposed to one of the other five (like sha3-256).


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/multiformats/multihash/pull/22/files/0638385dd25a1f5b186819b50c9999a4621117b2#r73419387,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcoc4EDpDrw_azWOSUeeTT6sg51rL3ks5qcQC4gaJpZM4HLQD1
.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the digest is different; you're right. It is not simply a matter of length.

However, I still am not sure - why is this PR suggesting we use sha3 as an alias for sha3-512, and not some other one of the sha3 functions?

Copy link
Member

Choose a reason for hiding this comment

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

I believe because that's what was used by an implementation.

Copy link
Member

Choose a reason for hiding this comment

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

@candeira Can you back up this decision?

Copy link

Choose a reason for hiding this comment

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

This multihash repo defines a spec, not an API, so the proposed wording that references a "string code" feels weird to me (many implementations will not use a string to specify the hash type). What about something like this instead:

Implementations should be aware that a previous version of this spec used the label SHA3 for the SHA3-512 hash (code 0x14). If your implementation allows hash types to be specified as a string, you should consider making SHA3 and SHA3-512 aliases, or disallow the label SHA3 entirely.

@jbenet
Copy link
Member

jbenet commented Jul 30, 2016

sorry for the months delay on review

@jbenet
Copy link
Member

jbenet commented Jul 30, 2016

this should probably modify the table? see #12

@jbenet jbenet mentioned this pull request Jul 30, 2016
@kumavis kumavis mentioned this pull request Oct 18, 2016
@celeduc
Copy link
Contributor

celeduc commented Sep 4, 2017

This PR is out of date and should be closed.

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

Successfully merging this pull request may close these issues.

None yet

6 participants