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

x/crypto/sha3: add Keccak 256 support #19709

Closed
pascaldekloe opened this issue Mar 25, 2017 · 9 comments
Closed

x/crypto/sha3: add Keccak 256 support #19709

pascaldekloe opened this issue Mar 25, 2017 · 9 comments

Comments

@pascaldekloe
Copy link
Contributor

@pascaldekloe pascaldekloe commented Mar 25, 2017

Ethereum and other crypto chains use the "original" Keccak hash. The difference is a dsbyte of 1 instead of 6 as in func NewKeccak256() hash.Hash { return &state{rate: 136, outputLen: 32, dsbyte: 0x01} }.

Can we support this Hash in the library or allow the specification of the domain separation byte in some way?

@gopherbot gopherbot added this to the Unreleased milestone Mar 25, 2017
@odeke-em odeke-em changed the title x/crypto/sha3: Keccak 256 support x/crypto/sha3: add Keccak 256 support Mar 27, 2017
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 27, 2017

/cc @agl @aead and other crypto folks.

@aead
Copy link
Contributor

@aead aead commented Mar 27, 2017

I'm not a member of the Go team and this my personal opinion.

@pascaldekloe
I think adding more functions to the sha3 package isn't a good idea. The package already contains some complexity for its users (see the long doc). Further the package is named sha3 (not keccak), so it should implement the SHA-3 standard (and the "improved version" SHAKE).

Adam (@agl) once said:

Go only tries to meet the 90%-need in order to keep things simple.

I'm not sure whether it's worth to add a new keccak package - probably not. It would be basically a copy of the sha3 package - but of course @agl has to decide about this.

@odeke-em odeke-em added the Proposal label Mar 27, 2017
@ghost
Copy link

@ghost ghost commented Sep 16, 2017

Keccak256 is just very small modification (1 byte modification!) to sha3, similar to shake better to extend sha3 package, not create a new one.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 4, 2018

Based on discussion with @agl, @bradfitz, @ianlancetaylor, accepted with symbol name NewLegacyKeccak256.

Let us know if Ethereum also uses the 128 variant.

@splix
Copy link

@splix splix commented May 4, 2018

@FiloSottile Ethereum's Go code uses only 256 variant

for reference https://github.com/ethereumproject/go-ethereum/blob/master/crypto/sha3/hashes.go

@pascaldekloe
Copy link
Contributor Author

@pascaldekloe pascaldekloe commented May 4, 2018

Shall CL106462 be used to make the change or should I create a new one?

@leonklingele
Copy link
Contributor

@leonklingele leonklingele commented May 4, 2018

s/NewKeccak/NewLegacyKeccak/g and jettison the non-256bit variants?

leonklingele added a commit to leonklingele/crypto that referenced this issue May 4, 2018
Keccak uses a different domain separation byte as the NIST-
standardized SHA-3 hashing function.

Fixes golang/go#19709
@gopherbot
Copy link

@gopherbot gopherbot commented May 4, 2018

Change https://golang.org/cl/106462 mentions this issue: sha3: add support for Keccak hash function

leonklingele added a commit to leonklingele/crypto that referenced this issue May 4, 2018
Keccak uses a different domain separation byte as the NIST-
standardized SHA-3 hashing function.

Fixes golang/go#19709
leonklingele added a commit to leonklingele/crypto that referenced this issue May 5, 2018
Keccak uses a different domain separation byte as the NIST-
standardized SHA-3 hashing function.

Fixes golang/go#19709
@karalabe
Copy link
Contributor

@karalabe karalabe commented May 29, 2018

@FiloSottile Just saw this, wow :)

That said, could we also have NewLegacyKeccak512? The ethash PoW uses that too for the DAG calculation. https://github.com/ethereum/go-ethereum/blob/master/consensus/ethash/algorithm.go#L169

PS: @splix The link posted above is not from the upstream Ethereum project, rather the ETC fork of it.

heronhaye pushed a commit to keybase/go-crypto that referenced this issue Nov 27, 2018
Keccak uses a different domain separation byte as the NIST-
standardized SHA-3 hashing function.

Fixes golang/go#19709

Change-Id: I1b45afce9b7719241b24bbdc9b67718d73b457d3
GitHub-Last-Rev: 4f2a701
GitHub-Pull-Request: golang#41
Reviewed-on: https://go-review.googlesource.com/106462
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@golang golang locked and limited conversation to collaborators May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.