core/vm, crypto/bn256: switch over to cloudflare library #16203
Conversation
|
It doesn't build on Go 1.7. |
|
Let's drop support for Go 1.7. It's ancient and we wanted to |
karalabe
force-pushed
the
bn256-optimizations
branch
from
f35236e to
231a8e2
Compare
| } | ||
|
|
||
| func (c *curvePoint) String() string { | ||
| c.MakeAffine() |
There was a problem hiding this comment.
This modifies c. I don't like it that printing a variable modifies it. I suggest :
func (c *curvePoint) String() string {
d := &curvePoint{}
d.Set(c)
d.MakeAffine()
x, y := &gfP{}, &gfP{}
montDecode(x, &d.x)
montDecode(y, &d.y)
return "(" + x.String() + ", " + y.String() + ")"
}There was a problem hiding this comment.
This is what both the Google and Cloudflare code does. I'd rather not screw around with modifying the internals of a crypto library we don't understand.
|
|
||
| // IsOnCurve returns true iff c is on the curve. | ||
| func (c *curvePoint) IsOnCurve() bool { | ||
| c.MakeAffine() |
There was a problem hiding this comment.
This also modifies the curve c. It us sysed from bn256.go:Unmarshal. Is this correct, in all cases?
There was a problem hiding this comment.
This is what both the Google and Cloudflare code does. I'd rather not screw around with modifying the internals of a crypto library we don't understand.
|
Why is the lattice branch of https://github.com/cloudflare/bn256/tree/lattices not used, since it speeds up arithmetic on the individual curves. |
|
@ValarDragon because it's not implemented for our particular curve (...yet..) |
|
Is there an operation (scalar multiplication and point addition) reduction planned after these speed ups? Also, this “lattice” branch mentioned above should have brought further speed up of non-pairing ops, @karalabe what was a reason not to use it at the first place? |
We'll evaluate the speed with the new code and compared to the other clients too. Would be nice, yes.
It needs special curve parameter calculations that need a cryptographer who can actually do it :) We have it since then, but hit a few snags. It may be included in the next release. |
|
Hello @karalabe Are those parameters actually endomorphism parameters? Sincerely, Alex |
crypto/bn256/google/constants.go
Outdated
| } | ||
|
|
||
| // u is the BN parameter that determines the prime: 1868033³. | ||
| var u = bigFromBase10("4965661367192848881") |
There was a problem hiding this comment.
May I know why those value of constant variables are not same as the cloudflare/google branch, for example, var u = bigFromBase10("6518589491078791937"). Based on the comment of this line, 1868033³ also equal to 6518589491078791937, not 4965661367192848881
This PR switches over our bn256 implementation from the legacy one from the Go crypto libs to an optimized one from Cloudflare.
Only
amd64is switched for now, since the Cloudflare library is currently missing the pure Go implementations of the Montgomery arithmetics. We can either keep it as is, or do a complete switch over when those methods are done too.The numbers speak for themselves: