Routines for working with threshold signatures #138
Conversation
Other changes: Cosmetic fixes bringing the project closer to the Status style guide.
| export exportUncompressed | ||
| export | ||
| exportUncompressed, | ||
| ID, recover, genSecretShare, fromUint32, add |
There was a problem hiding this comment.
I think add is a problematic proc name in a security sensitive context, it makes it really hard to grep it or find out what collection it actually operates on.
The argument that it allows generic processing is moot because there is likely no reason to have something generic over a seq a table and say a polynomial.
| func fromP2*(s: typedesc[Signature], pt: blst_p2): Signature = | ||
| var transformed: blst_p2_affine | ||
| transformed.blst_p2_to_affine(pt) | ||
| Signature(point: transformed) |
There was a problem hiding this comment.
I would be very wary about returning large types on the stack, especially the SecretKey.
Nim used to (or still does) not optimize correctly the allocation with extra copies or zeroMem (flaky RVO or NRVO).
| a.blst_p2_mult(a, s.toScalar(), 255) | ||
|
|
||
| func `*`(a: blst_p2; s: blst_fr): blst_p2= | ||
| result.blst_p2_mult(a, s.toScalar(), 255) |
There was a problem hiding this comment.
Same remark, returning large values should be avoided for internal implementations, it's unnecessary costly in terms of stack space if RVO or NRVO fails.
A result allocation, a zeroMem
And then another copy at call site:
See also:
- Out-of-place functions lead to bad codegen Out-of-place functions lead to bad codegen mratsim/constantine#145
- Internal API: in-place vs result Internal API: in-place vs result mratsim/constantine#21
There was a problem hiding this comment.
Frankly, this looks like a problem that's trivial to fix in Nim. It's a bit sad that it's informing the design of blscurve and Constantine.
| func toP2(s: Signature): blst_p2 = | ||
| result.blst_p2_from_affine(s.getPoint) | ||
|
|
||
| func add*(a: SecretKey, b: SecretKey): SecretKey = |
There was a problem hiding this comment.
A function operating on a secret key should not be called add.
| func add*(a: SecretKey, b: SecretKey): SecretKey = | ||
| var r: blst_fr | ||
| blst_fr_add(r, a.toFr, b.toFr) | ||
| SecretKey.fromFr(r) |
There was a problem hiding this comment.
Unnecessary copies, we can directly add in the buffer
| # We will calculate a0 + X(a1 + X(a2 + ..X(an-1 + Xan)) | ||
| var y = cfs[count - 1] | ||
| for i in 2 .. count: | ||
| y = y * x + cfs[count - i] |
There was a problem hiding this comment.
Lots of extra stack space used here
|
|
||
| return y | ||
|
|
||
| func lagrangeInterpolation[T, U](yVec: openArray[T], xVec: openArray[U]): Result[T, cstring] = |
There was a problem hiding this comment.
Result is not cryptographically hardened, a better API here would be to return an enum and have the result be a var parameter.
It also adds to the large copies issue.
| SecretKey.fromFr evaluatePolynomial(cfs, id.toFr) | ||
|
|
||
| func recover*(secrets: openArray[SecretKey], | ||
| ids: openArray[ID]): Result[SecretKey, cstring] = |
There was a problem hiding this comment.
SecretKeys should not go through Result
| ## Recover original secret key from N points generated by genSecretShare | ||
| let ys = secrets.mapIt(it.toFr) | ||
| let xs = ids.mapIt(it.toFr) | ||
| ok SecretKey.fromFr(? lagrangeInterpolation(ys, xs)) |
There was a problem hiding this comment.
Heap allocation of secret data can and should be avoided here. A for-loop is enough.
| ## produced by at least N different secret shares generated by genSecretShare | ||
| let ys = signs.mapIt(it.toP2) | ||
| let xs = ids.mapIt(it.toFr) | ||
| ok Signature.fromP2(? lagrangeInterpolation(ys, xs)) |
There was a problem hiding this comment.
Extra unnecessary heap allocation
Other changes:
Cosmetic fixes bringing the project closer to the
Status style guide.