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 missing variations of sha2 #206

Closed
wants to merge 1 commit into from
Closed

Add missing variations of sha2 #206

wants to merge 1 commit into from

Conversation

warpfork
Copy link
Contributor

Per discussion in #205 .

I've inserted these entries at the first available numbers following the other sha2 variants. It's unfortunate we can't put them all in a row, but no space is available since some sha3 variants immediately follow the already-entabled sha2 variants.

Per discussion in #205 .

I've inserted these entries at the first available numbers following the other sha2 variants.  It's unfortunate we can't put them all in a row, but no space is available since some sha3 variants immediately follow the already-entabled sha2 variants.
@vmx
Copy link
Member

vmx commented Feb 15, 2021

I would move sha2-512-224 and sha2-512-256 to higher ranges. But I don't have a strong opinion on that.

dccp, multiaddr, 0x21,
murmur3-128, multihash, 0x22,
murmur3-32, multihash, 0x23,
sha2-512-224, multihash, 0x24, aka SHA-512/224; specified by FIPS 180-4.
Copy link
Member

Choose a reason for hiding this comment

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

IMO these should be up in the 0x1012 range. The single byte range is precious. sha2-224 is unlikely to get wide usage too so I'd prefer that to be higher as well. I can see the case for 384 though.

fwiw I don't think FIPS 180-4 is a strong claim to their level of importance at this stage, in crypto terms it's ancient, if you're making new software then it's not highly likely that you're reaching for it as your standard. And FIPS-202 is all about SHA3 I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I'm definitely not claiming a FIPS number expresses anything about their importance :) That's just there to remove any possible ambiguity about the naming vs what the thing is.

@mvdan
Copy link

mvdan commented Sep 30, 2021

I also don't have a strong opinion, but I have to admit I'd avoid taking spots in the single-byte range unless the added multicodec will be very common :)

@vmx
Copy link
Member

vmx commented Sep 30, 2021

I also lean towards putting it into higher ranges (as @rvagg suggested) and it seems most people involved lean towards higher ranges, lets put it into higher ranges and be done. I guess that most important thing is that it is somewhere.

@warpfork
Copy link
Contributor Author

warpfork commented Sep 30, 2021

I'm pretty strongly held that I want sha2-384 to be in the single-byte range because it is the "best" out of these hashes (see discussion in issue) that is also available in the standard libraries we tend to notice first (js, in node and browsers, and golang). So I think it will be quite common.

I guess I'll bump up the sha2-512-* numbers to two-byte if we like.

I could also see bumping up sha2-224 to a higher range because I don't think it'll be commonly used. Fragments things that are related, but I guess avoiding that isn't really a goal we need to prioritize, so okay.

@warpfork
Copy link
Contributor Author

... I seem to have started this PR using the github edit feature, and then deleted the fork it creates in the meanwhile, so I no longer know how to amend this PR. Going to close it and open a new one shortly.

@warpfork warpfork closed this Sep 30, 2021
warpfork added a commit that referenced this pull request Sep 30, 2021
Per discussion in #205 ,
and further discussion of the numbers,
in #206 .

Most of the variants are shifted into larger number ranges.
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.

4 participants