nknorg / nkn

Official Go implementation of NKN full node.
https://www.nkn.org
Apache License 2.0
474 stars 158 forks source link

Consider Update of `edwards25519.go` #923

Open ghost opened 1 year ago

ghost commented 1 year ago

the file edwards25519.go is from 2013:

https://github.com/nknorg/nkn/blob/aa832aae9a4001c70484c9b3001de89fc3f31eb8/crypto/ed25519/edwards25519/edwards25519.go#L1-L2252

It contains a high amount of duplication, but is very difficult to reduce.

Suggestion

Consider switching to a more modern version:

The project https://github.com/FiloSottile/edwards25519 is and option, and it mentions several other options, like using the version include in go:

(see the commit comments for more reasons to update to a newer version)

https://go-review.googlesource.com/c/go/+/276272

Suggested Process

yilunzhang commented 1 year ago

As you can see in the comment section, this file is part of the official Go code (before 1.17). Upgrading to the latest Go internal version (filippo.io/edwards25519) should be good idea.

ghost commented 1 year ago

As you can see

???

I saw it, assumed it was copied in early dev of nkn/ancestor-project, thought "it cannot be the same version today" and naturally aborted refactoring (I aborted ongoing work, without code-level result)

So I found https://github.com/FiloSottile/edwards25519 and finally https://go-review.googlesource.com/c/go/+/276272, with the possibly most important

Intel(R) Core(TM) i5-7400 CPU @ 3.00GHz

name              old time/op    new time/op    delta
KeyGeneration-4     59.5µs ± 1%    26.1µs ± 1%  -56.20%  (p=0.000 n=10+10)
NewKeyFromSeed-4    59.3µs ± 1%    25.8µs ± 1%  -56.48%  (p=0.000 n=9+10)
Signing-4           60.4µs ± 1%    31.4µs ± 1%  -48.05%  (p=0.000 n=10+10)
Verification-4       169µs ± 1%      73µs ± 2%  -56.55%  (p=0.000 n=10+10)

Filed issue here, and expected... An ACK and a starting budget to go on with the general part of this update task.