code review 3065041: crypto/elliptic: add package

crypto/elliptic: add package

elliptic implements several standard elliptic curves over prime fields.

[r2, 2010-11-12 16:37:26]

Is "ec" a standard name? It's cryptic to me but perhaps not to others.

-rob

[agl1, 2010-11-12 16:39:51]

On Fri, Nov 12, 2010 at 11:37 AM, Rob 'Commander' Pike <r@google.com> wrote:
> Is "ec" a standard name? It's cryptic to me but perhaps not to others.

It's fairly standard (as in ECDH, ECDHA etc). But crypto/elliptic
might be clearer?


AGL

[agl1, 2010-11-12 17:04:01]

PTAL

[r, 2010-11-12 18:38:11]

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
File src/pkg/crypto/elliptic/elliptic.go (right):

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:5: // elliptic implements several standard
elliptic curves over prime fields
s/elliptic/The elliptic package/

(consistency with other presentations; avoids starting a sentence with a lower
case letter)

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:9: // Significant speedups could be obtained
by using either a projective or
s/speedups/speedup/ ?

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:26: func (curve *Curve) IsOnCurve(x, y
*big.Int) bool {
i wonder if Curve needs to be part of the name.   ellipse.IsOn(x, y) vs.
ellipse.IsOnCurve(x, y)

maybe it should be there. not sure.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:28: y2 := (new(big.Int)).Mul(y, y)
do you need the parens around new()? i don't think so and i'm surprised gofmt
didn't take them out

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:31: x3 := (new(big.Int)).Mul(x, x)
ditto

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:34: threeX := (new(big.Int)).Lsh(x, 1)
ditto

actually there's a lot. i'll stop dittoing here but please fix the others

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:44: // Add adds the points (x1, y1) and (x2,
y2) and returns their sum.
Add returns the sum of (x1,y1) and (x2,y2)
also you're inconsistent in your comments on whether there's a space after the
comma in the points. I think they're sharper without but it's up to you; just be
consistent

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:64: // y =
(2x1+x2)*(y2-y1)/(x2-x1)-(y2-y1)³/(x2-x1)³-y1
i love that you can use superscripts without some bluestocking demanding 7-bit
clean source

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:133: for bit := 0; bit < 8; bit++ {
b, bit, and byte are all mixed up here. i think you have at least one too many
variables.  plus counting up and down and left shift and right shift.
i've been staring at this loop for a while and i still am not certain which bit
goes where

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
File src/pkg/crypto/elliptic/elliptic_test.go (right):

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic_test.go:26: baseMultTest{
you don't need this stutter any more. delete the extra words or let gofmt -s do
that for you

[agl1, 2010-11-12 18:56:12]

PTAL

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
File src/pkg/crypto/elliptic/elliptic.go (right):

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:5: // elliptic implements several standard
elliptic curves over prime fields
On 2010/11/12 18:38:11, r wrote:
> s/elliptic/The elliptic package/
> 
> (consistency with other presentations; avoids starting a sentence with a lower
> case letter)

Done.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:9: // Significant speedups could be obtained
by using either a projective or
On 2010/11/12 18:38:11, r wrote:
> s/speedups/speedup/ ?

Done.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:26: func (curve *Curve) IsOnCurve(x, y
*big.Int) bool {
On 2010/11/12 18:38:11, r wrote:
> maybe it should be there. not sure.

Given:
  p224.IsOn(x, y)
I wonder which is the subject and which is the object. I think I like IsOnCurve
for that reason.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:28: y2 := (new(big.Int)).Mul(y, y)
On 2010/11/12 18:38:11, r wrote:
> do you need the parens around new()? i don't think so and i'm surprised gofmt
> didn't take them out

Done.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:31: x3 := (new(big.Int)).Mul(x, x)
On 2010/11/12 18:38:11, r wrote:
> ditto

Done.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:34: threeX := (new(big.Int)).Lsh(x, 1)
On 2010/11/12 18:38:11, r wrote:
> ditto
> 
> actually there's a lot. i'll stop dittoing here but please fix the others

Done.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:44: // Add adds the points (x1, y1) and (x2,
y2) and returns their sum.
On 2010/11/12 18:38:11, r wrote:
> Add returns the sum of (x1,y1) and (x2,y2)
> also you're inconsistent in your comments on whether there's a space after the
> comma in the points. I think they're sharper without but it's up to you; just
be
> consistent

Done.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:133: for bit := 0; bit < 8; bit++ {
On 2010/11/12 18:38:11, r wrote:
> b, bit, and byte are all mixed up here. i think you have at least one too many
> variables.  plus counting up and down and left shift and right shift.
> i've been staring at this loop for a while and i still am not certain which
bit
> goes where

Ok. I've killed |i|. The loops are basically walking each bit of the input in
order. |byte| contains the current byte and |b| is the current bit. I've renamed
|bit| to |bitNum| because it's just counting 8 bits in a byte I've killed |b| by
ANDing against 0x80 directly.

See if you like it any better now. I could also write func explode(a []byte)
[]bool to turn a byte array into a bit array and iterate over that. Slower but
possibly clearer if you like the idea.

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
File src/pkg/crypto/elliptic/elliptic_test.go (right):

http://codereview.appspot.com/3065041/diff/20001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic_test.go:26: baseMultTest{
On 2010/11/12 18:38:11, r wrote:
> you don't need this stutter any more. delete the extra words or let gofmt -s
do
> that for you

Done.

[r, 2010-11-12 19:03:05]

LGTM

http://codereview.appspot.com/3065041/diff/23001/src/pkg/crypto/elliptic/elli...
File src/pkg/crypto/elliptic/elliptic.go (right):

http://codereview.appspot.com/3065041/diff/23001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic.go:144: byte <<= 1
much much clearer

http://codereview.appspot.com/3065041/diff/23001/src/pkg/crypto/elliptic/elli...
File src/pkg/crypto/elliptic/elliptic_test.go (right):

http://codereview.appspot.com/3065041/diff/23001/src/pkg/crypto/elliptic/elli...
src/pkg/crypto/elliptic/elliptic_test.go:291: k, ok :=
(new(big.Int)).SetString(e.k, 10)
outer parens can go

[agl1, 2010-11-12 19:53:46]

Hello r, r2 (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[agl1, 2010-11-12 19:55:45]

*** Submitted as http://code.google.com/p/go/source/detail?r=a1c4f5b41b07 ***

crypto/elliptic: add package

elliptic implements several standard elliptic curves over prime fields.

R=r, r2
CC=golang-dev
http://codereview.appspot.com/3065041

[agl1, 2010-11-12 20:17:31]

Submitted.