|
|
Descriptiongo.crypto/sha3: new package
Added a pure Go implementation of SHA3 (Keccak) which implements the hash.Hash interface.
A test file is included with performance benchmarks and standard test vectors.
Patch Set 1 #Patch Set 2 : code review 7760044: sha3/: new package #Patch Set 3 : code review 7760044: sha3/: new package #Patch Set 4 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #
Total comments: 97
Patch Set 5 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #Patch Set 6 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #
Total comments: 36
Patch Set 7 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #
Total comments: 18
Patch Set 8 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #Patch Set 9 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #
Total comments: 8
Patch Set 10 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #Patch Set 11 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #Patch Set 12 : diff -r 8dd5caec1eae https://code.google.com/p/go.crypto/ #
MessagesTotal messages: 21
agl leads the go.crypto sub-repository, and https://groups.google.com/d/msg/golang-dev/clWZwaQz9cs/rJ5zORSKk20J suggests that he's not in favor of including SHA3 in go.crypto. You are of course free to publish your SHA3 implementation somewhere "go get"table: bitbucket, github, code.google.com, etc.
Sign in to reply to this message.
On Tue, Mar 19, 2013 at 8:56 PM, <nigeltao@golang.org> wrote: > suggests that he's not in favor of including SHA3 in go.crypto. I do rather wish that SHA-3 would disappear, but this is nicely done. I'll try and take a better look tomorrow. One thing that does give me pause is that NIST are still consulting on the parameters for the different `official' hashes (the capacity etc). So I don't think that SHA-3 is final yet. Cheers AGL
Sign in to reply to this message.
In general, your top-level // comment lines are uncomfortably long. I'd break it at somewhere between 80-100 bytes. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode1 sha3/sha3.go:1: // Copyright 2012 The Go Authors. All rights reserved. 2013, and similarly in the other files. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode13 sha3/sha3.go:13: Delete the blank line, so that the comment above is recognized by godoc as a package doc comment. Run "godoc code.google.com/p/go.crypto/sha3" to check that your godoc output looks good. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode74 sha3/sha3.go:74: func (d *digest) Capacity() int { Rename Capacity to capacity unless there's a reason to export this method. Similarly for Rate. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode78 sha3/sha3.go:78: // Rate returns number of bytes of the internal state which can be absorbed or squeezed in between calls to the permutation function. "returns the number" https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode80 sha3/sha3.go:80: return (stateSize - d.Capacity()) Outer parens are unnecessary. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode87 sha3/sha3.go:87: for x := 0; x < numLanes; x++ { for i := range d.a { d.a[i] = 0 } https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode92 sha3/sha3.go:92: // BlockSize, required by the Digest interface, does not have a standard intepretation for a sponge-based construction like SHA3. What is the Digest interface? https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode206 sha3/sha3.go:206: func (d *digest) dumpState() { If this is just for debugging then move it to sha3_test.go or delete it entirely. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode222 sha3/sha3.go:222: return &digest{outputSize: outputSize} I don't think this function is doing much (and "new" shadows a built-in function). Just inline it below: func New224() hash.Hash { &digest{outputSize: 224 / 8} } https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode225 sha3/sha3.go:225: // The NewX() constructors enable initializing a hash in any of the four standard sizes of output, specified in bits. Drop the parens. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode232 sha3/sha3.go:232: //Given no parameters, a new instance with the highest security level is provided. Go doc comments should begin with the name of the thing being commented upon. Thus: // New returns a new instance etc. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode5 sha3/sha3_test.go:5: // This file implements test code for the SHA3 package. I'd drop this sentence. It's obvious from the file name. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode6 sha3/sha3_test.go:6: // These tests are a subset of those provided by the Keccak web site: http://keccak.noekeon.org/ Place a file-level comment below the package clause, so that it's less likely to be confused for a package doc comment: -------- // Copyright 2013 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. package sha3 // These tests are a subset of those provided by the Keccak web site: http://keccak.noekeon.org/ import ( -------- https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode29 sha3/sha3_test.go:29: repeatCount int // If repeatCount is greater than 1, the input input is concatenated this many times. "input input" https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode36 sha3/sha3_test.go:36: func (t *hashTest) Test(testDigests map[int]*digest) (bool, string) { This should return error, not (bool, string), and the caller can test err != nil. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: switch t.inputHex { if t.inputHex { but instead of an inputHex field, I'd make a top-level func decodeHex(s string) string { b, err := hex.DecodeString(s) if err != nil { panic(err) } return string(b) } and make your test cases call that, instead of having this function do the hex-decoding. You could then do the string to []byte conversion for t.input outside the loop, which for TestLongVectors would avoid 16777216-1 unnecessary pieces of garbage. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode58 sha3/sha3_test.go:58: for size, expectedOutput := range t.expectedOutputs { The usual Go language is got and want, not computedOutput and expectedOutput. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode70 sha3/sha3_test.go:70: return false, fmt.Sprintf("SHA3[%d](%s) = %s want %s", testDigests[size].Size()*8, errorInput, computedOutput, strings.ToLower(expectedOutput)) Use fmt.Errorf instead of fmt.Sprintf to return an error. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode70 sha3/sha3_test.go:70: return false, fmt.Sprintf("SHA3[%d](%s) = %s want %s", testDigests[size].Size()*8, errorInput, computedOutput, strings.ToLower(expectedOutput)) You probably want %q instead of %s. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode80 sha3/sha3_test.go:80: for i := 0; i < len(result); i++ { for i := range result { result[i] = byte(i) } https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode142 sha3/sha3_test.go:142: // benchmarkBulkHashHelper tests the speed to hash a 16 kiB buffer. Drop the "Helper". The difference in "Benchmark" and "benchmark" is enough to distinguish these functions. Similarly for testVectorHelper. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode158 sha3/sha3_test.go:158: // benchmarkBulkHashSHA3_X test the speed to hash a 16 kiB buffer by calling benchmarkBulkHashHelper. kiB should be KiB. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode169 sha3/sha3_test.go:169: repeatCount: 16777216, "repeatCount: 16 * 1024 * 1024," might be clearer. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go File sha3/sha3internal.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode1 sha3/sha3internal.go:1: // Copyright 2012 The Go Authors. All rights reserved. I'd rename the file from sha3internal.go to internal.go. We're already in the sha3 directory. There's no need to stutter. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode5 sha3/sha3internal.go:5: // Package sha3internal implements the core Keccak permutation function necessary for computing SHA3. It's not package sha3internal. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode14 sha3/sha3internal.go:14: var RC = [...]uint64{ Rename RC to rc unless there's a reason to export these values. Similarly for R0. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode48 sha3/sha3internal.go:48: return w<<i ^ w>>(64-i) The compiler will optimize "w<<i ^ w>>(64-i)" as a rotate if i is constant, but not if i is a variable (or in this case, a function parameter). Instead of a rotateLeft function, it may be better to define r0_xx constants like so: const ( r0_00 = 0 r0_01 = 36 r0_02 = 3 // etc ) and below: d.b[0] = d.a[0]<<r0_00 ^ d.a[0]>>(64-r0_00) d.b[1] = d.a[6]<<r0_06 ^ d.a[6]>>(64-r0_06) // etc I filed https://code.google.com/p/go/issues/detail?id=5084 for the compiler bug.
Sign in to reply to this message.
https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go File sha3/sha3internal.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode1 sha3/sha3internal.go:1: // Copyright 2012 The Go Authors. All rights reserved. On 2013/03/20 02:17:43, nigeltao wrote: > I'd rename the file from sha3internal.go to internal.go. Or, if the purpose of this file is to implement the Keccak-f function, I'd call it keccakf.go. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode124 sha3/sha3internal.go:124: d.a[0] = d.b[0] ^ (^(d.b[1]) & d.b[2]) The inner-most parens are unnecessary.
Sign in to reply to this message.
https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode39 sha3/sha3.go:39: var padOpen = [laneSize]uint64{ Was this the result of benchmarking? I'd assume that a shift would be faster than a memory access for this. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode57 sha3/sha3.go:57: a [numLanes]uint64 //main state of the hash space after "//" https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode73 sha3/sha3.go:73: // Capacity returns the number of bytes not touched by directly during squeezing or absorbing, but which are only being updated by the permutation function. "by directly" might be a typo. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode93 sha3/sha3.go:93: // Currently we return the data rate, the number of bytes which can be absorbed per invocation of the permutation function. consider s/,/:/, but up to you. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode96 sha3/sha3.go:96: func (d *digest) BlockSize() int { return d.Rate() } Yes, BlockSize is an unfortunate wart in the Hash interface that was added in order to support HMAC. I think your choice here is reasonable. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode107 sha3/sha3.go:107: offset := (d.totalAbsorbed + absorbed) % d.Rate() I suspect this function could be simpler and faster with something like: var t uint64 for i := len(p) - 1; i >= 0; i-- { t |= p[i] t <<= 8 } t <<= 8*(offset%laneSize) d.a[lane] ^= t (There's probably a bug in there, just coding out loud.) https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode111 sha3/sha3.go:111: copy(tempBuf[offset%laneSize:(offset%laneSize)+len(p)], p) I think the text between ':' and ']' can be omitted. copy will only copy the minimum of the length of the source and destination, so it will already only copy len(p) bytes. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode118 sha3/sha3.go:118: // Since lanes are stored internall as type uin64, this requires converting the incoming bytes into uint64 using a little endian interpretation. 'y' on the end of internally. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode118 sha3/sha3.go:118: // Since lanes are stored internall as type uin64, this requires converting the incoming bytes into uint64 using a little endian interpretation. s/type uin64/uint64s/ and s/uint64/uint64s/ later on. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode122 sha3/sha3.go:122: No need for the blank line at the start of the function. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode136 sha3/sha3.go:136: for absorbed < len(p)-7 { This isn't really a problem, but this code doesn't look very canonical. Go code typically reslices when processing a []byte. i.e. lots of p = p[8:] and then testing len(p) > 0 etc. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode142 sha3/sha3.go:142: //fmt.Println(lane, absorbed, absorbed+lane*laneSize) looks like debugging left in. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode165 sha3/sha3.go:165: // The padding is 10*1 with enough 0s to fill out an entire block. "The padding starts and ends with a one bit, with one or more zero bits in between in order to fill out of entire block" (I read "10*1" as "ten times one" for a while before figuring it out.) https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode175 sha3/sha3.go:175: // Sum takes a (possibly nil) set of remaining bytes and "squeezes" out bytes from the state into an output buffer. Sum takes a slice to which the hash is appended. The slice is not input data! (This allows one to avoid allocating for every hash computation.) https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode178 sha3/sha3.go:178: Blank line not needed at start of function. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode191 sha3/sha3.go:191: rm blank line. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode232 sha3/sha3.go:232: //Given no parameters, a new instance with the highest security level is provided. I don't think we need this function. In other packages we have done something like this because the package is called, say, "sha256", therefore "sha256.New" is obvious. However, in this case, "sha3.New512" will be clear but "sha3.New" much less so.
Sign in to reply to this message.
Here are fixes for the issues you brought up. With some of the optimizations suggested it's now about twice as fast, and about 75% as fast as SHA512. Note that I added a new test function to find bugs in the suggested changes to the absorption code, and slightly re-factored the sum including fixing the fact that I badly misinterpreted the interface to Sum() Thanks! Joe https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode1 sha3/sha3.go:1: // Copyright 2012 The Go Authors. All rights reserved. On 2013/03/20 02:17:43, nigeltao wrote: > 2013, and similarly in the other files. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode13 sha3/sha3.go:13: Done. When I run godoc it picks up the Package doc okay but not most of the functions. Am I missing something? https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode39 sha3/sha3.go:39: var padOpen = [laneSize]uint64{ Not the result of benchmarking, but vestigal. I originally was going to pre-compute all padding blocks, before realizing that doesn't work since they may end up different lanes. Changed to a shift for padOpen. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode57 sha3/sha3.go:57: a [numLanes]uint64 //main state of the hash On 2013/03/20 14:48:50, agl1 wrote: > space after "//" Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode73 sha3/sha3.go:73: // Capacity returns the number of bytes not touched by directly during squeezing or absorbing, but which are only being updated by the permutation function. On 2013/03/20 14:48:50, agl1 wrote: > "by directly" might be a typo. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode74 sha3/sha3.go:74: func (d *digest) Capacity() int { On 2013/03/20 02:17:43, nigeltao wrote: > Rename Capacity to capacity unless there's a reason to export this method. > Similarly for Rate. Done for now. It may be useful to some clients to expose direct access to the sponge function (including absorbing and squeezing data). https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode78 sha3/sha3.go:78: // Rate returns number of bytes of the internal state which can be absorbed or squeezed in between calls to the permutation function. On 2013/03/20 02:17:43, nigeltao wrote: > "returns the number" Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode80 sha3/sha3.go:80: return (stateSize - d.Capacity()) On 2013/03/20 02:17:43, nigeltao wrote: > Outer parens are unnecessary. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode87 sha3/sha3.go:87: for x := 0; x < numLanes; x++ { On 2013/03/20 02:17:43, nigeltao wrote: > for i := range d.a { > d.a[i] = 0 > } Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode92 sha3/sha3.go:92: // BlockSize, required by the Digest interface, does not have a standard intepretation for a sponge-based construction like SHA3. On 2013/03/20 02:17:43, nigeltao wrote: > What is the Digest interface? Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode93 sha3/sha3.go:93: // Currently we return the data rate, the number of bytes which can be absorbed per invocation of the permutation function. On 2013/03/20 14:48:50, agl1 wrote: > consider s/,/:/, but up to you. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode96 sha3/sha3.go:96: func (d *digest) BlockSize() int { return d.Rate() } On 2013/03/20 14:48:50, agl1 wrote: > Yes, BlockSize is an unfortunate wart in the Hash interface that was added in > order to support HMAC. I think your choice here is reasonable. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode107 sha3/sha3.go:107: offset := (d.totalAbsorbed + absorbed) % d.Rate() Yes, your approach is a little bit faster (~10% or so). There was a bug in your attempt, the two lines of the loop need to be swapped or else you shift the buffer an extra time. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode111 sha3/sha3.go:111: copy(tempBuf[offset%laneSize:(offset%laneSize)+len(p)], p) On 2013/03/20 14:48:50, agl1 wrote: > I think the text between ':' and ']' can be omitted. copy will only copy the > minimum of the length of the source and destination, so it will already only > copy len(p) bytes. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode118 sha3/sha3.go:118: // Since lanes are stored internall as type uin64, this requires converting the incoming bytes into uint64 using a little endian interpretation. On 2013/03/20 14:48:50, agl1 wrote: > 'y' on the end of internally. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode118 sha3/sha3.go:118: // Since lanes are stored internall as type uin64, this requires converting the incoming bytes into uint64 using a little endian interpretation. On 2013/03/20 14:48:50, agl1 wrote: > s/type uin64/uint64s/ and s/uint64/uint64s/ later on. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode122 sha3/sha3.go:122: On 2013/03/20 14:48:50, agl1 wrote: > No need for the blank line at the start of the function. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode136 sha3/sha3.go:136: for absorbed < len(p)-7 { I re-wrote it in this idiom, it ended up cleaner. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode142 sha3/sha3.go:142: //fmt.Println(lane, absorbed, absorbed+lane*laneSize) On 2013/03/20 14:48:50, agl1 wrote: > looks like debugging left in. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode165 sha3/sha3.go:165: // The padding is 10*1 with enough 0s to fill out an entire block. On 2013/03/20 14:48:50, agl1 wrote: > "The padding starts and ends with a one bit, with one or more zero bits in > between in order to fill out of entire block" > > (I read "10*1" as "ten times one" for a while before figuring it out.) Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode175 sha3/sha3.go:175: // Sum takes a (possibly nil) set of remaining bytes and "squeezes" out bytes from the state into an output buffer. Yes, I interpreted the Interface very incorrectly. I reworked this and split the squeezing into a separate function-if necessary this could be exported to use SHA3 directly as a PRNG. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode178 sha3/sha3.go:178: On 2013/03/20 14:48:50, agl1 wrote: > Blank line not needed at start of function. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode191 sha3/sha3.go:191: On 2013/03/20 14:48:50, agl1 wrote: > rm blank line. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode206 sha3/sha3.go:206: func (d *digest) dumpState() { On 2013/03/20 02:17:43, nigeltao wrote: > If this is just for debugging then move it to sha3_test.go or delete it > entirely. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode222 sha3/sha3.go:222: return &digest{outputSize: outputSize} On 2013/03/20 02:17:43, nigeltao wrote: > I don't think this function is doing much (and "new" shadows a built-in > function). Just inline it below: > > func New224() hash.Hash { &digest{outputSize: 224 / 8} } Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode225 sha3/sha3.go:225: // The NewX() constructors enable initializing a hash in any of the four standard sizes of output, specified in bits. On 2013/03/20 02:17:43, nigeltao wrote: > Drop the parens. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode232 sha3/sha3.go:232: //Given no parameters, a new instance with the highest security level is provided. On 2013/03/20 02:17:43, nigeltao wrote: > Go doc comments should begin with the name of the thing being commented upon. > Thus: > // New returns a new instance etc. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3.go#newcode232 sha3/sha3.go:232: //Given no parameters, a new instance with the highest security level is provided. On 2013/03/20 14:48:50, agl1 wrote: > I don't think we need this function. In other packages we have done something > like this because the package is called, say, "sha256", therefore "sha256.New" > is obvious. However, in this case, "sha3.New512" will be clear but "sha3.New" > much less so. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode5 sha3/sha3_test.go:5: // This file implements test code for the SHA3 package. On 2013/03/20 02:17:43, nigeltao wrote: > I'd drop this sentence. It's obvious from the file name. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode6 sha3/sha3_test.go:6: // These tests are a subset of those provided by the Keccak web site: http://keccak.noekeon.org/ On 2013/03/20 02:17:43, nigeltao wrote: > Place a file-level comment below the package clause, so that it's less likely to > be confused for a package doc comment: > -------- > // Copyright 2013 The Go Authors. All rights reserved. > // Use of this source code is governed by a BSD-style > // license that can be found in the LICENSE file. > > package sha3 > > // These tests are a subset of those provided by the Keccak web site: > http://keccak.noekeon.org/ > > import ( > -------- Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode29 sha3/sha3_test.go:29: repeatCount int // If repeatCount is greater than 1, the input input is concatenated this many times. On 2013/03/20 02:17:43, nigeltao wrote: > "input input" Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode36 sha3/sha3_test.go:36: func (t *hashTest) Test(testDigests map[int]*digest) (bool, string) { On 2013/03/20 02:17:43, nigeltao wrote: > This should return error, not (bool, string), and the caller can test err != > nil. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: switch t.inputHex { On 2013/03/20 02:17:43, nigeltao wrote: > if t.inputHex { > > but instead of an inputHex field, I'd make a top-level > > func decodeHex(s string) string { > b, err := hex.DecodeString(s) > if err != nil { > panic(err) > } > return string(b) > } > > and make your test cases call that, instead of having this function do the > hex-decoding. > > You could then do the string to []byte conversion for t.input outside the loop, > which for TestLongVectors would avoid 16777216-1 unnecessary pieces of garbage. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode58 sha3/sha3_test.go:58: for size, expectedOutput := range t.expectedOutputs { On 2013/03/20 02:17:43, nigeltao wrote: > The usual Go language is got and want, not computedOutput and expectedOutput. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode70 sha3/sha3_test.go:70: return false, fmt.Sprintf("SHA3[%d](%s) = %s want %s", testDigests[size].Size()*8, errorInput, computedOutput, strings.ToLower(expectedOutput)) On 2013/03/20 02:17:43, nigeltao wrote: > You probably want %q instead of %s. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode70 sha3/sha3_test.go:70: return false, fmt.Sprintf("SHA3[%d](%s) = %s want %s", testDigests[size].Size()*8, errorInput, computedOutput, strings.ToLower(expectedOutput)) On 2013/03/20 02:17:43, nigeltao wrote: > Use fmt.Errorf instead of fmt.Sprintf to return an error. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode80 sha3/sha3_test.go:80: for i := 0; i < len(result); i++ { On 2013/03/20 02:17:43, nigeltao wrote: > for i := range result { > result[i] = byte(i) > } Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode142 sha3/sha3_test.go:142: // benchmarkBulkHashHelper tests the speed to hash a 16 kiB buffer. On 2013/03/20 02:17:43, nigeltao wrote: > Drop the "Helper". The difference in "Benchmark" and "benchmark" is enough to > distinguish these functions. > > Similarly for testVectorHelper. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode142 sha3/sha3_test.go:142: // benchmarkBulkHashHelper tests the speed to hash a 16 kiB buffer. On 2013/03/20 02:17:43, nigeltao wrote: > Drop the "Helper". The difference in "Benchmark" and "benchmark" is enough to > distinguish these functions. > > Similarly for testVectorHelper. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode158 sha3/sha3_test.go:158: // benchmarkBulkHashSHA3_X test the speed to hash a 16 kiB buffer by calling benchmarkBulkHashHelper. On 2013/03/20 02:17:43, nigeltao wrote: > kiB should be KiB. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode169 sha3/sha3_test.go:169: repeatCount: 16777216, On 2013/03/20 02:17:43, nigeltao wrote: > "repeatCount: 16 * 1024 * 1024," might be clearer. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go File sha3/sha3internal.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode1 sha3/sha3internal.go:1: // Copyright 2012 The Go Authors. All rights reserved. On 2013/03/20 02:22:15, nigeltao wrote: > On 2013/03/20 02:17:43, nigeltao wrote: > > I'd rename the file from sha3internal.go to internal.go. > > Or, if the purpose of this file is to implement the Keccak-f function, I'd call > it keccakf.go. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode1 sha3/sha3internal.go:1: // Copyright 2012 The Go Authors. All rights reserved. On 2013/03/20 02:17:43, nigeltao wrote: > I'd rename the file from sha3internal.go to internal.go. We're already in the > sha3 directory. There's no need to stutter. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode5 sha3/sha3internal.go:5: // Package sha3internal implements the core Keccak permutation function necessary for computing SHA3. On 2013/03/20 02:17:43, nigeltao wrote: > It's not package sha3internal. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode14 sha3/sha3internal.go:14: var RC = [...]uint64{ On 2013/03/20 02:17:43, nigeltao wrote: > Rename RC to rc unless there's a reason to export these values. Similarly for > R0. Done. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode48 sha3/sha3internal.go:48: return w<<i ^ w>>(64-i) Great catch! This roughly doubled the speed of the benchmarks. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3internal.go#newcode124 sha3/sha3internal.go:124: d.a[0] = d.b[0] ^ (^(d.b[1]) & d.b[2]) On 2013/03/20 02:22:15, nigeltao wrote: > The inner-most parens are unnecessary. Done.
Sign in to reply to this message.
https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: switch t.inputHex { On 2013/03/20 23:36:16, jcb wrote: > On 2013/03/20 02:17:43, nigeltao wrote: > > if t.inputHex { > > > > but instead of an inputHex field, I'd make a top-level > > > > func decodeHex(s string) string { > > b, err := hex.DecodeString(s) > > if err != nil { > > panic(err) > > } > > return string(b) > > } > > > > and make your test cases call that, instead of having this function do the > > hex-decoding. > > > > You could then do the string to []byte conversion for t.input outside the > loop, > > which for TestLongVectors would avoid 16777216-1 unnecessary pieces of > garbage. > > Done. Not (entirely) done? I would eliminate the t.inputHex field entirely. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode39 sha3/sha3.go:39: const padOpen = 0x0000000000000001 const ( padOpen = 0x0000000000000001 padClose = 0x8000000000000000 ) will let gofmt align the RHSs. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode136 sha3/sha3.go:136: lastLane := minInt(d.rate()/laneSize, firstLane+(len(p))/laneSize) Unnecessary parens around len(p). https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode205 sha3/sha3.go:205: func (d_original *digest) Sum(in []byte) []byte { Call the method receiver d, and the new variable a few lines below dup. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode206 sha3/sha3.go:206: Delete the blank line. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode29 sha3/sha3_test.go:29: repeatCount int // input will be concatenated the input this many times. I would s/repeatCount/repeat/ and s/expectedOutputs/want/. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode44 sha3/sha3_test.go:44: // It returns true or false based on the test passing, plus an error string for printing. The comment needs updating. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode45 sha3/sha3_test.go:45: func (t *hashTest) Test(testDigests map[int]*digest) error { Remove the argument, since it's always the global testDigests. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: Delete the blank line. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode60 sha3/sha3_test.go:60: for i := 0; i < t.repeatCount || i == 0; i++ { When is t.repeatCount zero? https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode68 sha3/sha3_test.go:68: got := strings.ToLower(hex.EncodeToString(testDigests[size].Sum(nil))) Call ToUpper instead of ToLower if want is always going to be upper case. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode70 sha3/sha3_test.go:70: errorInput := t.input Instead of constructing an errorInput string, I'd add a desc field to hashTest, containing a short human-readable description of that test (including the ×%d for the long test). For example, see http://golang.org/src/pkg/compress/zlib/reader_test.go https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode100 sha3/sha3_test.go:100: for size := range testDigests { for _, d := range testDigests { d.Reset() d.Write(buf) etc } https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode106 sha3/sha3_test.go:106: for i < len(buf) { for i := 0; i < len(buf); { https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode117 sha3/sha3_test.go:117: if !bytes.Equal(want, got) { The usual Go order is got and want, not want and got. Similarly on the next line. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode118 sha3/sha3_test.go:118: t.Fatalf(fmt.Sprintf("Unalighned SHA3[%d] = wanted %x, got %x", testDigests[size].Size()*8, want, got)) Drop the inner fmt.Sprintf. t.Fatalf is already an 'f' function. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode229 sha3/sha3_test.go:229: func TestLongVectors(t *testing.T) { I'd just roll this together with TestShortVectors, and then inline the testVector helper function: func TestVectors(t *testing.T) { testCases := append([]hashTest{}, shortTestVectors...) if !testing.Short { testCases = append(testCases, longTestVectors...) } loop: for _, tc := range testCases { if err := tc.Test(); err != nil { t.Errorf("%s: %v", tc.desc, err) } } } Or remove the hashTest.Test method and just put that code inside the for loop above, replacing "return fmt.Errorf(etc)" with "t.Errorf(etc); continue loop". https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode286 sha3/sha3_test.go:286: error := test.Test(testDigests) s/error/err/ https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode288 sha3/sha3_test.go:288: t.Fatalf(error.Error()) t.Fatal(err), otherwise you'll print garbage if e.g. the error string contains "%s". Also, change Fatal to Error. It's worth distinguishing whether all the vectors fail, or only the first one.
Sign in to reply to this message.
Also change the CL description opening line from: sha3/: new package to go.crypto/sha3: new package.
Sign in to reply to this message.
I think we're down to two issues of how the test vectors are stored-I think the current format is simplest but could be convinced otherwise. Thanks again! Joe https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: switch t.inputHex { I don't follow what you're suggesting here. To support string and hex input (and the standard tests include both) we have to store somewhere how to interpret a string. Are you suggesting hard-coding it by having the test function call it for some test vectors and not others? https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode39 sha3/sha3.go:39: const padOpen = 0x0000000000000001 Thinking about it further, abstracting these as named constants makes the code less readable so I inlined them. Their structure is straightforward and any change to the padding scheme would require changing the code anyways, there's no agility gained by abstracting them. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode136 sha3/sha3.go:136: lastLane := minInt(d.rate()/laneSize, firstLane+(len(p))/laneSize) On 2013/03/21 02:36:50, nigeltao wrote: > Unnecessary parens around len(p). Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode205 sha3/sha3.go:205: func (d_original *digest) Sum(in []byte) []byte { On 2013/03/21 02:36:50, nigeltao wrote: > Call the method receiver d, and the new variable a few lines below dup. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3.go#newcode206 sha3/sha3.go:206: On 2013/03/21 02:36:50, nigeltao wrote: > Delete the blank line. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode29 sha3/sha3_test.go:29: repeatCount int // input will be concatenated the input this many times. On 2013/03/21 02:36:50, nigeltao wrote: > I would s/repeatCount/repeat/ and s/expectedOutputs/want/. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode44 sha3/sha3_test.go:44: // It returns true or false based on the test passing, plus an error string for printing. On 2013/03/21 02:36:50, nigeltao wrote: > The comment needs updating. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode45 sha3/sha3_test.go:45: func (t *hashTest) Test(testDigests map[int]*digest) error { On 2013/03/21 02:36:50, nigeltao wrote: > Remove the argument, since it's always the global testDigests. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: On 2013/03/21 02:36:50, nigeltao wrote: > Delete the blank line. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode60 sha3/sha3_test.go:60: for i := 0; i < t.repeatCount || i == 0; i++ { On 2013/03/21 02:36:50, nigeltao wrote: > When is t.repeatCount zero? Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode68 sha3/sha3_test.go:68: got := strings.ToLower(hex.EncodeToString(testDigests[size].Sum(nil))) On 2013/03/21 02:36:50, nigeltao wrote: > Call ToUpper instead of ToLower if want is always going to be upper case. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode70 sha3/sha3_test.go:70: errorInput := t.input I don't think this is helpful for test vectors like these which don't admit any short human-readable summary. They're just random, they don't exercise any specific functionality. Adding a fixed error string would just mean hard-coding what's produced here, which would mean specifying the data in two places. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode100 sha3/sha3_test.go:100: for size := range testDigests { On 2013/03/21 02:36:50, nigeltao wrote: > for _, d := range testDigests { > d.Reset() > d.Write(buf) > etc > } Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode106 sha3/sha3_test.go:106: for i < len(buf) { On 2013/03/21 02:36:50, nigeltao wrote: > for i := 0; i < len(buf); { Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode117 sha3/sha3_test.go:117: if !bytes.Equal(want, got) { On 2013/03/21 02:36:50, nigeltao wrote: > The usual Go order is got and want, not want and got. Similarly on the next > line. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode118 sha3/sha3_test.go:118: t.Fatalf(fmt.Sprintf("Unalighned SHA3[%d] = wanted %x, got %x", testDigests[size].Size()*8, want, got)) On 2013/03/21 02:36:50, nigeltao wrote: > Drop the inner fmt.Sprintf. t.Fatalf is already an 'f' function. Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode229 sha3/sha3_test.go:229: func TestLongVectors(t *testing.T) { On 2013/03/21 02:36:50, nigeltao wrote: > I'd just roll this together with TestShortVectors, and then inline the > testVector helper function: > > func TestVectors(t *testing.T) { > testCases := append([]hashTest{}, shortTestVectors...) > if !testing.Short { > testCases = append(testCases, longTestVectors...) > } > loop: > for _, tc := range testCases { > if err := tc.Test(); err != nil { > t.Errorf("%s: %v", tc.desc, err) > } > } > } > > Or remove the hashTest.Test method and just put that code inside the for loop > above, replacing "return fmt.Errorf(etc)" with "t.Errorf(etc); continue loop". Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode286 sha3/sha3_test.go:286: error := test.Test(testDigests) On 2013/03/21 02:36:50, nigeltao wrote: > s/error/err/ Done. https://codereview.appspot.com/7760044/diff/20001/sha3/sha3_test.go#newcode288 sha3/sha3_test.go:288: t.Fatalf(error.Error()) On 2013/03/21 02:36:50, nigeltao wrote: > t.Fatal(err), otherwise you'll print garbage if e.g. the error string contains > "%s". > > Also, change Fatal to Error. It's worth distinguishing whether all the vectors > fail, or only the first one. Done.
Sign in to reply to this message.
LGTM with nits. If Nigel says LGTM then I'll land it. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode20 sha3/sha3.go:20: // Note that changing this to a size besides 8 requires using other than uint64 to store each lane. "using a type other than a uint64" or maybe "using other than a uint64". https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode23 sha3/sha3.go:23: // sliceSize represent the dimensions of the internal state, a square matrix of s on the end of "represent" https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode24 sha3/sha3.go:24: // sliceSize ** 2 lanes. This is both the size of "rows" and "columns" dimension in the missing "the": "size of the "rows" and ... https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode106 sha3/sha3.go:106: // Track the total number of bytes absorbed and update the hash state at the end. I don't think you want this variable, although it's your call. If you eliminate it, and just update d.totalAbsorbed, then you can remove an argument to unalignedAbsorb. You do need to change the += in the unaligned case to something like: n := d.unalignedAbsorb(... p = p[n:] d.totalAbsorbed += n Although this does remove the += in the original code which makes the p[absorbed:] look wrong, until one notes that absorbed must be zero beforehand. It also saves you lots of additions to d.totalAbsorbed. Since this function always processes the whole input, it can just have `total := len(p)` at the top to keep the value for the return. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode183 sha3/sha3.go:183: var buf [8]byte I'd remove buf because the multiple appends make Sum(nil) a worst-case, as append probably allocates and moves multiple times. if cap(in) >= toSqueeze { in = in[:toSqueeze] } else { in = make([]byte, toSqueeze) } then: binary.LittleEndian.PutUint64(in, d.a[i/laneSize]) in = in[laneSize:] https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode206 sha3/sha3.go:206: // The NewX constructors enable initializing a hash in any of the four standard sizes. These will probably need to be updated, right? NIST hasn't settled on the final capacities for the various sizes as I understand. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go#newcode26 sha3/sha3_test.go:26: // hashTest represents a test vector inputs and expected outputs at multiple sizes. s/inputs/input/ https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go#newcode109 sha3/sha3_test.go:109: for size := range testDigests { for _, digest := range testDigests { digest.Reset() } https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go#newcode112 sha3/sha3_test.go:112: // Convert input data to a byte array, decoding Hex if necessary s/decoding Hex/hex decoding/
Sign in to reply to this message.
Fixed nits plus two substantial items: *Changed the squeeze() function to only allocate one or fewer times. Please confirm that this is a reasonable implementation. *To hedge against future standardization changes, changed the names from New512 to NewKeccak512, etc. Whatever they call the final variants (there's still debate about this, they might just be named like SHA3-512 by output size or they might be named something ghastly like SHA3-512_1024 to indicate the underlying capacity parameter. If we put Keccak in the name it should be clear this may not be the eventual standard. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode20 sha3/sha3.go:20: // Note that changing this to a size besides 8 requires using other than uint64 to store each lane. On 2013/03/21 20:27:23, agl1 wrote: > "using a type other than a uint64" or maybe "using other than a uint64". Done. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode23 sha3/sha3.go:23: // sliceSize represent the dimensions of the internal state, a square matrix of On 2013/03/21 20:27:23, agl1 wrote: > s on the end of "represent" Done. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode24 sha3/sha3.go:24: // sliceSize ** 2 lanes. This is both the size of "rows" and "columns" dimension in the On 2013/03/21 20:27:23, agl1 wrote: > missing "the": "size of the "rows" and ... Done. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode106 sha3/sha3.go:106: // Track the total number of bytes absorbed and update the hash state at the end. Done. This made the code cleaner. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode183 sha3/sha3.go:183: var buf [8]byte I don't think your suggested code complies with the Hash interface, since the output needs to be appended to any input data (though I've mistaken the intention of this interface once already). Point taken about not calling multiple appends though-please review my updated implementation which performs only a single memory allocation (if needed). https://codereview.appspot.com/7760044/diff/27001/sha3/sha3.go#newcode206 sha3/sha3.go:206: // The NewX constructors enable initializing a hash in any of the four standard sizes. Indeed. To avoid issues later on, I changed the names from NewX to NewKeccakX, so that whatever the eventual standard SHA3 algorithm names won't be confused with these (which reflect the final Keccak proposal). They haven't settled on the final capacity/output combinations so I changed capacity to a free parameter. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go#newcode26 sha3/sha3_test.go:26: // hashTest represents a test vector inputs and expected outputs at multiple sizes. On 2013/03/21 20:27:23, agl1 wrote: > s/inputs/input/ Done. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go#newcode109 sha3/sha3_test.go:109: for size := range testDigests { This might call extraneous Resets (not that they're very expensive) if a testCase doesn't use all available hash versions. https://codereview.appspot.com/7760044/diff/27001/sha3/sha3_test.go#newcode112 sha3/sha3_test.go:112: // Convert input data to a byte array, decoding Hex if necessary On 2013/03/21 20:27:23, agl1 wrote: > s/decoding Hex/hex decoding/ Done.
Sign in to reply to this message.
https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: switch t.inputHex { On 2013/03/21 19:06:25, jcb wrote: > I don't follow what you're suggesting here. I'd write it like this: // testVector represents a test input and expected outputs from multiple algorithm variants. type testVector struct { desc string input []byte repeat int // The input will be written this many times. want map[string]string } // decodeHex converts an hex-encoded string into a raw byte string. func decodeHex(s string) []byte { b, err := hex.DecodeString(s) if err != nil { panic(err) } return b } // shortTestVectors stores a series of short testVectors. // Inputs of 8, 248, and 264 bits from http://keccak.noekeon.org/ are included below. // The standard defines additional test inputs of all sizes between 0 and 2047 bits. // Because the current implementation can only handle an integral number of bytes, // most of the standard test inputs can't be used. var shortKeccakTestVectors = []testVector{ { desc: "short8", input: decodeHex("CC"), repeat: 1, want: map[string]string{ "Keccak224": "A9CAB59EB40A10B246290F2D6086E32E3689FAF1D26B470C899F2802", "Keccak256": "EEAD6DBFC7340A56CAEDC044696A168870549A6A7F6F56961E84A54BD9970B8A", "Keccak384": "1B84E62A46E5A201861754AF5DC95C4A1A69CAF4A796AE405680161E29572641F5FA1E8641D7958336EE7B11C58F73E9", "Keccak512": "8630C13CBD066EA74BBE7FE468FEC1DEE10EDC1254FB4C1B7C5FD69B646E44160B8CE01D05A0908CA790DFB080F4B513BC3B6225ECE7A810371441A5AC666EB9", }, }, { desc: "short248", input: decodeHex("84FB51B517DF6C5ACCB5D022F8F28DA09B10232D42320FFC32DBECC3835B29"), repeat: 1, want: map[string]string{ "Keccak224": "81AF3A7A5BD4C1F948D6AF4B96F93C3B0CF9C0E7A6DA6FCD71EEC7F6", "Keccak256": "D477FB02CAAA95B3280EC8EE882C29D9E8A654B21EF178E0F97571BF9D4D3C1C", "Keccak384": "503DCAA4ADDA5A9420B2E436DD62D9AB2E0254295C2982EF67FCE40F117A2400AB492F7BD5D133C6EC2232268BC27B42", "Keccak512": "9D8098D8D6EDBBAA2BCFC6FB2F89C3EAC67FEC25CDFE75AA7BD570A648E8C8945FF2EC280F6DCF73386109155C5BBC444C707BB42EAB873F5F7476657B1BC1A8", }, }, { desc: "short264", input: decodeHex("DE8F1B3FAA4B7040ED4563C3B8E598253178E87E4D0DF75E4FF2F2DEDD5A0BE046"), repeat: 1, want: map[string]string{ "Keccak224": "F217812E362EC64D4DC5EACFABC165184BFA456E5C32C2C7900253D0", "Keccak256": "E78C421E6213AFF8DE1F025759A4F2C943DB62BBDE359C8737E19B3776ED2DD2", "Keccak384": "CF38764973F1EC1C34B5433AE75A3AAD1AAEF6AB197850C56C8617BCD6A882F6666883AC17B2DCCDBAA647075D0972B5", "Keccak512": "9A7688E31AAF40C15575FC58C6B39267AAD3722E696E518A9945CF7F7C0FEA84CB3CB2E9F0384A6B5DC671ADE7FB4D2B27011173F3EEEAF17CB451CF26542031", }, }, } // longTestVectors stores longer testVector (currently only one). // The computed test vector is about 413 MiB long, taken from http://keccak.noekeon.org/. var longKeccakTestVectors = []testVector{ { desc: "long-abc", input: []byte("abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmno"), repeat: 16 * 1024 * 1024, want: map[string]string{ "Keccak224": "C42E4AEE858E1A8AD2976896B9D23DD187F64436EE15969AFDBC68C5", "Keccak256": "5F313C39963DCF792B5470D4ADE9F3A356A3E4021748690A958372E2B06F82A4", "Keccak384": "9B7168B4494A80A86408E6B9DC4E5A1837C85DD8FF452ED410F2832959C08C8C0D040A892EB9A755776372D4A8732315", "Keccak512": "3E122EDAF37398231CFACA4C7C216C9D66D5B899EC1D7AC617C40C7261906A45FC01617A021E5DA3BD8D4182695B5CB785A28237CBB167590E34718E56D8AAB8", }, }, } // TestKeccakVectors checks that correct output is produced for a set of known testVectors. func TestKeccakVectors(t *testing.T) { testCases := append([]testVector{}, shortKeccakTestVectors...) if !testing.Short() { testCases = append(testCases, longKeccakTestVectors...) } for _, tc := range testCases { for alg, want := range tc.want { d := testDigests[alg] d.Reset() for i := 0; i < tc.repeat; i++ { d.Write(tc.input) } got := strings.ToUpper(hex.EncodeToString(d.Sum(nil))) if got != want { t.Errorf("%s, alg=%s\ngot %q\nwant %q", tc.desc, alg, got, want) d.dumpState() } } } } https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode103 sha3/sha3_test.go:103: // In short testing mode, the longer tests are skipped. I don't think this comment is adding much. https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode121 sha3/sha3_test.go:121: for alg := range tc.want { I'd reverse the order of the loops, and avoid 64 million map lookups. After doing that, you're ranging over tc.want three times; you can collapse that. On my workstation, that cuts the "go test" running time by 10%. https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode139 sha3/sha3_test.go:139: t.Errorf("Test vector failed for %s(%s): got %q, wanted %q", alg, errorInput, got, want) s/wanted/want/ https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode179 sha3/sha3_test.go:179: t.Errorf("Inconsistent output after unaligned writes to %s: got %x, wanted %x", alg, got, want) s/wanted/want/
Sign in to reply to this message.
Test code cleaned up. https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/7001/sha3/sha3_test.go#newcode46 sha3/sha3_test.go:46: switch t.inputHex { On 2013/03/22 04:21:21, nigeltao wrote: > On 2013/03/21 19:06:25, jcb wrote: > > I don't follow what you're suggesting here. > > I'd write it like this: > > // testVector represents a test input and expected outputs from multiple > algorithm variants. > type testVector struct { > desc string > input []byte > repeat int // The input will be written this many times. > want map[string]string > } > > // decodeHex converts an hex-encoded string into a raw byte string. > func decodeHex(s string) []byte { > b, err := hex.DecodeString(s) > if err != nil { > panic(err) > } > return b > } > > // shortTestVectors stores a series of short testVectors. > // Inputs of 8, 248, and 264 bits from http://keccak.noekeon.org/ are included > below. > // The standard defines additional test inputs of all sizes between 0 and 2047 > bits. > // Because the current implementation can only handle an integral number of > bytes, > // most of the standard test inputs can't be used. > var shortKeccakTestVectors = []testVector{ > { > desc: "short8", > input: decodeHex("CC"), > repeat: 1, > want: map[string]string{ > "Keccak224": "A9CAB59EB40A10B246290F2D6086E32E3689FAF1D26B470C899F2802", > "Keccak256": > "EEAD6DBFC7340A56CAEDC044696A168870549A6A7F6F56961E84A54BD9970B8A", > "Keccak384": > "1B84E62A46E5A201861754AF5DC95C4A1A69CAF4A796AE405680161E29572641F5FA1E8641D7958336EE7B11C58F73E9", > "Keccak512": > "8630C13CBD066EA74BBE7FE468FEC1DEE10EDC1254FB4C1B7C5FD69B646E44160B8CE01D05A0908CA790DFB080F4B513BC3B6225ECE7A810371441A5AC666EB9", > }, > }, > { > desc: "short248", > input: > decodeHex("84FB51B517DF6C5ACCB5D022F8F28DA09B10232D42320FFC32DBECC3835B29"), > repeat: 1, > want: map[string]string{ > "Keccak224": "81AF3A7A5BD4C1F948D6AF4B96F93C3B0CF9C0E7A6DA6FCD71EEC7F6", > "Keccak256": > "D477FB02CAAA95B3280EC8EE882C29D9E8A654B21EF178E0F97571BF9D4D3C1C", > "Keccak384": > "503DCAA4ADDA5A9420B2E436DD62D9AB2E0254295C2982EF67FCE40F117A2400AB492F7BD5D133C6EC2232268BC27B42", > "Keccak512": > "9D8098D8D6EDBBAA2BCFC6FB2F89C3EAC67FEC25CDFE75AA7BD570A648E8C8945FF2EC280F6DCF73386109155C5BBC444C707BB42EAB873F5F7476657B1BC1A8", > }, > }, > { > desc: "short264", > input: > decodeHex("DE8F1B3FAA4B7040ED4563C3B8E598253178E87E4D0DF75E4FF2F2DEDD5A0BE046"), > repeat: 1, > want: map[string]string{ > "Keccak224": "F217812E362EC64D4DC5EACFABC165184BFA456E5C32C2C7900253D0", > "Keccak256": > "E78C421E6213AFF8DE1F025759A4F2C943DB62BBDE359C8737E19B3776ED2DD2", > "Keccak384": > "CF38764973F1EC1C34B5433AE75A3AAD1AAEF6AB197850C56C8617BCD6A882F6666883AC17B2DCCDBAA647075D0972B5", > "Keccak512": > "9A7688E31AAF40C15575FC58C6B39267AAD3722E696E518A9945CF7F7C0FEA84CB3CB2E9F0384A6B5DC671ADE7FB4D2B27011173F3EEEAF17CB451CF26542031", > }, > }, > } > > // longTestVectors stores longer testVector (currently only one). > // The computed test vector is about 413 MiB long, taken from > http://keccak.noekeon.org/. > var longKeccakTestVectors = []testVector{ > { > desc: "long-abc", > input: > []byte("abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmno"), > repeat: 16 * 1024 * 1024, > want: map[string]string{ > "Keccak224": "C42E4AEE858E1A8AD2976896B9D23DD187F64436EE15969AFDBC68C5", > "Keccak256": > "5F313C39963DCF792B5470D4ADE9F3A356A3E4021748690A958372E2B06F82A4", > "Keccak384": > "9B7168B4494A80A86408E6B9DC4E5A1837C85DD8FF452ED410F2832959C08C8C0D040A892EB9A755776372D4A8732315", > "Keccak512": > "3E122EDAF37398231CFACA4C7C216C9D66D5B899EC1D7AC617C40C7261906A45FC01617A021E5DA3BD8D4182695B5CB785A28237CBB167590E34718E56D8AAB8", > }, > }, > } > > // TestKeccakVectors checks that correct output is produced for a set of known > testVectors. > func TestKeccakVectors(t *testing.T) { > testCases := append([]testVector{}, shortKeccakTestVectors...) > if !testing.Short() { > testCases = append(testCases, longKeccakTestVectors...) > } > for _, tc := range testCases { > for alg, want := range tc.want { > d := testDigests[alg] > d.Reset() > for i := 0; i < tc.repeat; i++ { > d.Write(tc.input) > } > got := strings.ToUpper(hex.EncodeToString(d.Sum(nil))) > if got != want { > t.Errorf("%s, alg=%s\ngot %q\nwant %q", tc.desc, alg, got, want) > d.dumpState() > } > } > } > } Done. https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go File sha3/sha3_test.go (right): https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode103 sha3/sha3_test.go:103: // In short testing mode, the longer tests are skipped. On 2013/03/22 04:21:21, nigeltao wrote: > I don't think this comment is adding much. Done. https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode121 sha3/sha3_test.go:121: for alg := range tc.want { On 2013/03/22 04:21:21, nigeltao wrote: > I'd reverse the order of the loops, and avoid 64 million map lookups. After > doing that, you're ranging over tc.want three times; you can collapse that. > > On my workstation, that cuts the "go test" running time by 10%. Done. https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode139 sha3/sha3_test.go:139: t.Errorf("Test vector failed for %s(%s): got %q, wanted %q", alg, errorInput, got, want) On 2013/03/22 04:21:21, nigeltao wrote: > s/wanted/want/ Done. https://codereview.appspot.com/7760044/diff/39001/sha3/sha3_test.go#newcode179 sha3/sha3_test.go:179: t.Errorf("Inconsistent output after unaligned writes to %s: got %x, wanted %x", alg, got, want) On 2013/03/22 04:21:21, nigeltao wrote: > s/wanted/want/ Done.
Sign in to reply to this message.
On Fri, Mar 22, 2013 at 12:15 PM, <jcb@google.com> wrote: > Test code cleaned up. I was just about to submit but: a) I need to put you in CONTRIBUTORS first. I can take care of that. b) the test is taking 70s to complete and I don't believe that it's worth the time. By reducing the long test from 1GB to 64MB, the test takes ~5 seconds to run, which is much more reasonable. Do you object to the latter? Obviously we're not taking values directly from the upstream reference tables any longer, which is a shame, but I think it's either that or delete the long test. Cheers AGL
Sign in to reply to this message.
Hmmm, alternative is that there are upstream test vectors of up to 4KB, but they are random. I'd say shipping those around is not worth the trouble either. I can cut the long vectors down to size and run them through the Python version to cross-check. On Fri, Mar 22, 2013 at 12:37 PM, Adam Langley <agl@golang.org> wrote: > On Fri, Mar 22, 2013 at 12:15 PM, <jcb@google.com> wrote: > > Test code cleaned up. > > I was just about to submit but: > > a) I need to put you in CONTRIBUTORS first. I can take care of that. > b) the test is taking 70s to complete and I don't believe that it's > worth the time. By reducing the long test from 1GB to 64MB, the test > takes ~5 seconds to run, which is much more reasonable. > > Do you object to the latter? Obviously we're not taking values > directly from the upstream reference tables any longer, which is a > shame, but I think it's either that or delete the long test. > > > Cheers > > AGL >
Sign in to reply to this message.
On Fri, Mar 22, 2013 at 12:47 PM, Joseph Bonneau <jcb@google.com> wrote: > Hmmm, alternative is that there are upstream test vectors of up to 4KB, but > they are random. I'd say shipping those around is not worth the trouble > either. > > I can cut the long vectors down to size and run them through the Python > version to cross-check. If you can confirm this with the Python code then I'll land it: var longKeccakTestVectors = []testVector{ »·······{ »·······»·······desc: "long-1GiB", »·······»·······input: []byte("abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmno"), »·······»·······repeat: 1024 * 1024, »·······»·······want: map[string]string{ »·······»·······»·······"Keccak224": "50E35E40980FEEFF1EA490957B0E970257F75EA0D410EE0F0B8A7A58", »·······»·······»·······"Keccak256": "5015A4935F0B51E091C6550A94DCD262C08998232CCAA22E7F0756DEAC0DC0D0", »·······»·······»·······"Keccak384": "7907A8D0FAA7BC6A90FE14C6C958C956A0877E751455D8F13ACDB96F144B5896E716C06EC0CB56557A94EF5C3355F6F3", »·······»·······»·······"Keccak512": "3EC327D6759F769DEB74E80CA70C831BC29CAB048A4BF4190E4A1DD5C6507CF2B4B58937FDE81D36014E7DFE1B1DD8B0F27CB7614F9A645FEC114F1DAAEFC056", »·······»·······}, »·······}, } Cheers AGL
Sign in to reply to this message.
Took 30 min, but the Python code confirmed these. On Fri, Mar 22, 2013 at 12:52 PM, Adam Langley <agl@golang.org> wrote: > On Fri, Mar 22, 2013 at 12:47 PM, Joseph Bonneau <jcb@google.com> wrote: > > Hmmm, alternative is that there are upstream test vectors of up to 4KB, > but > > they are random. I'd say shipping those around is not worth the trouble > > either. > > > > I can cut the long vectors down to size and run them through the Python > > version to cross-check. > > If you can confirm this with the Python code then I'll land it: > > var longKeccakTestVectors = []testVector{ > »·······{ > »·······»·······desc: "long-1GiB", > »·······»·······input: > []byte("abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmno"), > »·······»·······repeat: 1024 * 1024, > »·······»·······want: map[string]string{ > »·······»·······»·······"Keccak224": > "50E35E40980FEEFF1EA490957B0E970257F75EA0D410EE0F0B8A7A58", > »·······»·······»·······"Keccak256": > "5015A4935F0B51E091C6550A94DCD262C08998232CCAA22E7F0756DEAC0DC0D0", > »·······»·······»·······"Keccak384": > > "7907A8D0FAA7BC6A90FE14C6C958C956A0877E751455D8F13ACDB96F144B5896E716C06EC0CB56557A94EF5C3355F6F3", > »·······»·······»·······"Keccak512": > > "3EC327D6759F769DEB74E80CA70C831BC29CAB048A4BF4190E4A1DD5C6507CF2B4B58937FDE81D36014E7DFE1B1DD8B0F27CB7614F9A645FEC114F1DAAEFC056", > »·······»·······}, > »·······}, > } > > > > Cheers > > AGL >
Sign in to reply to this message.
Uploaded a final CL with these test vectors On Fri, Mar 22, 2013 at 2:22 PM, Joseph Bonneau <jcb@google.com> wrote: > Took 30 min, but the Python code confirmed these. > > > On Fri, Mar 22, 2013 at 12:52 PM, Adam Langley <agl@golang.org> wrote: > >> On Fri, Mar 22, 2013 at 12:47 PM, Joseph Bonneau <jcb@google.com> wrote: >> > Hmmm, alternative is that there are upstream test vectors of up to 4KB, >> but >> > they are random. I'd say shipping those around is not worth the trouble >> > either. >> > >> > I can cut the long vectors down to size and run them through the Python >> > version to cross-check. >> >> If you can confirm this with the Python code then I'll land it: >> >> var longKeccakTestVectors = []testVector{ >> »·······{ >> »·······»·······desc: "long-1GiB", >> »·······»·······input: >> >> []byte("abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmno"), >> »·······»·······repeat: 1024 * 1024, >> »·······»·······want: map[string]string{ >> »·······»·······»·······"Keccak224": >> "50E35E40980FEEFF1EA490957B0E970257F75EA0D410EE0F0B8A7A58", >> »·······»·······»·······"Keccak256": >> "5015A4935F0B51E091C6550A94DCD262C08998232CCAA22E7F0756DEAC0DC0D0", >> »·······»·······»·······"Keccak384": >> >> "7907A8D0FAA7BC6A90FE14C6C958C956A0877E751455D8F13ACDB96F144B5896E716C06EC0CB56557A94EF5C3355F6F3", >> »·······»·······»·······"Keccak512": >> >> "3EC327D6759F769DEB74E80CA70C831BC29CAB048A4BF4190E4A1DD5C6507CF2B4B58937FDE81D36014E7DFE1B1DD8B0F27CB7614F9A645FEC114F1DAAEFC056", >> »·······»·······}, >> »·······}, >> } >> >> >> >> Cheers >> >> AGL >> > >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=87c30004e36a&repo=crypto *** go.crypto/sha3: new package Added a pure Go implementation of SHA3 (Keccak) which implements the hash.Hash interface. A test file is included with performance benchmarks and standard test vectors. R=agl, nigeltao CC=golang-dev https://codereview.appspot.com/7760044 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|