-
Notifications
You must be signed in to change notification settings - Fork 245
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
Remove SquareRootField
, and move functionality to Field
#422
Remove SquareRootField
, and move functionality to Field
#422
Conversation
…lgebra into sjoseph/unify_field_traits
- `TWO_ADICITY` - `TRACE_MINUS_ONE_DIV_TWO` - `QUADRATIC_NONRESIDUE_TO_T` needed to compute sqrt in cubic extensions
@@ -446,7 +411,7 @@ where | |||
two_inv.div2(); | |||
|
|||
let two_inv = P::BasePrimeField::from(two_inv); | |||
let two_inv = P::BaseField::from(two_inv); | |||
let two_inv = P::BaseField::from_base_prime_field_elems(&[two_inv]).unwrap(); |
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.
Why the reason for this change?
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.
The calling method, sqrt
, was previously on a SquareRootField
trait, and in the implementation for quadratic extension we were restricting P: QuadExtConfig
to have:
where P::BaseField: From<P::BasePrimeField>
Now that the method is on Field
trait, we don't have these bounds, so I need to explicitly construct a base field element from BasePrimeField
elements.
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.
Let's add a BaseField: From<BasePrimeField>
bound in the config struct; maybe as a different PR?
I had discussed this with Solomon in the past, but a good way to unify the preprocessing for all the various square root algorithms would be to make an enum of the form
And then, in
The |
For now I've added only a |
Likely to add more variants in the future
d8a5634
to
ef0262d
Compare
SquareRootField
, and move functionality to Field
Description
Rather than implementing optimised algorithms for each case from the start, let's begin with Tonelli-Shanks, which should work in each instance, then benchmark that and continue to implement optimisations for each case one-by-one. While not fastest from the start, it already unifies the
SqrtField
andField
traits and makes using ofsqrt()
method cleaner.In this draft, I propose moving the following to
CubicExtField
(prev. only onFp3
):TWO_ADICITY
TRACE_MINUS_ONE_DIV_TWO
QUADRATIC_NONRESIDUE_TO_T
This puts a small overhead on implementers of
CubicExtField
which are notFp3
(e.g.Fq6
), requiring these to define the above constants.TODOs:
Credit to @solomonjoseph for the initial work on this.
closes: #387
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the GitHub PR explorer