-
Notifications
You must be signed in to change notification settings - Fork 187
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
k256: type safety for non-normalized field elements #531
Comments
I think it's a great idea for preventing errors, but yes, trait stuff might get difficult. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
k256
crate uses lazy normalization of field elements. While not a user-facing concern as we deliberately encapsulateFieldElement
, there is a potential for bugs in code ink256
itself in the event one "forgets" to normalize a field element prior to returning it as the result of some computation. See #530 as an example (although there have been others).One potential way to eliminate this class of bugs is to use separate types for normalized vs non-normalized field elements. For example, we could introduce something like
LazyFieldElement
on which all of the arithmetic operations are defined, while retainingFieldElement
for the normalized form and defining all serialization operations on that.LazyFieldElement::normalize
could returnFieldElement
, and we could additionally have bidirectionalFrom
conversions between the two.Things would be a bit tricky in regard to the the
ff::Field
traits. I imagine for compatibility reasons they would need to be impl'd onFieldElement
, converting to aLazyFieldElement
to perform the arithmetic, and then callingnormalize()
to get aFieldElement
again as a result, which would mean that the performance benefits of lazy normalization wouldn't be available under such an API. However, that is the only safe usage pattern since the traits are designed to abstract over different fields/field implementations, and not all of them have lazy normalization.cc @fjarri
The text was updated successfully, but these errors were encountered: