-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add ECDSA #310
feat: Add ECDSA #310
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think you should copy input scalars to JointScalarMultiplication. In usual ScalarMul, you get the scalars from GLV decomposition and they are safe to modify locally. For ECDSA it is safe as you create big.Int inputs freshly, but if anyone would use JointScalarMul separately, then it would mutate its inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To record conversation - maybe it is better to use fr.Element
in Signature
and PrivateKey
types.
Also, it seems params
type isn't strictly necessary - we have all the information in the curve template anyway. And add add signature/ecdsa
package which calls the actual implementation depending on the curve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds ECDSA scheme using gnark-crypto primitives on all curves. This follows "crypto/ecdsa", FIPS 186-4 and SEC1 v2.
todos:
fr.Inverse()
instead ofbigInt.ModInverse()
(experimented speedup is just ~1-2% so need to dig more why)[a]P + [b]Q
for fast verification (might also look into JSF encoding of scalars if perf is critical)