Skip to content
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

Add alloc feature flag #25

Closed
str4d opened this issue Dec 7, 2021 · 2 comments · Fixed by #26
Closed

Add alloc feature flag #25

str4d opened this issue Dec 7, 2021 · 2 comments · Fixed by #26
Assignees

Comments

@str4d
Copy link
Contributor

str4d commented Dec 7, 2021

In #4 we introduced a std feature flag, to enable downstream users to disable anything relying on std. This happened to gate all of the traits, because both FieldExt and CurveAffine depended on std::io.

We removed std::io usage from FieldExt in #20, so it could now be removed from any feature flag. CurveAffine still relies on std::io, but I want to remove that as well. Then we're just left with the fact that CurveExt requires FieldExt, FieldExt is bound on SqrtRatio, and the only way we can implement SqrtRatio right now depends on SqrtTables, which requires alloc.

So, we should add an alloc feature flag, and move everything behind it. We might also be able to remove the std feature flag as a result (which has not been in a release yet). There's no point in having the traits themselves be no-std accessible if we can't implement them without alloc. Once we have a fallback for SqrtRatio we can consider making the other traits no-std usable.

@str4d str4d self-assigned this Dec 7, 2021
@str4d str4d added this to the Core Sprint 2021-48 milestone Dec 7, 2021
@krnak
Copy link
Contributor

krnak commented Dec 7, 2021

Another reason, why CurveExt cannot be accessible without alloc, is that hash_to_curve still depends on Box.

However I would like to be able to compile the crate without sqrt tables to safe memory of our tiny device. I remind you that these tables are only optimization. We already can calculate square roots via Tonelli-Shanks algorithm without tables. SqrtRatio can be implemented generically as

fn sqrt_ratio(num: &Self, div: &Self) -> (Choice, Self) {
    //generic implementation:

    //invert div
    let div_inv = (*div).invert().unwrap_or(Self::zero());

    let x = (*num) * div_inv; // x = num/div
    let y = x * Self::root_of_unity(); // y = lambda*num/div

    let x_sqrt = x.sqrt();
    let y_sqrt = y.sqrt();

    // Either x is square or y is square
    let normal_sqrt = CtOption::conditional_select(&y_sqrt, &x_sqrt, x_sqrt.is_some());
    let special_sqrt = CtOption::new(Self::zero(), 1.into());
    let result_sqrt = CtOption::conditional_select(&normal_sqrt, &special_sqrt, div.is_zero());

    let normal_bool = x_sqrt.is_some();
    let special_bool = !div.is_zero();
    let result_bool = Choice::conditional_select(&normal_bool, &special_bool, div.is_zero());

    (result_bool, result_sqrt.unwrap())
}

or a little bit more efficiently by separating first part of Tonelli-Shanks algorithm (where the squareness of an input is checked).

May we create a new feature flag to conditionally disable these tables?

@str4d
Copy link
Contributor Author

str4d commented Dec 7, 2021

Hmm, that's a good point about separating the table impl from alloc. Yeah, I think it would be fine to have a sqrt-table flag that depends on alloc to provide them. We can workshop the API beyond that later.

str4d added a commit that referenced this issue Dec 22, 2021
Now that we have a default implementation of `SqrtRatio::sqrt_ratio`, we
can use it and `FieldExt` in no-std environments.

We introduce an `alloc` feature flag to form a common feature dependency
between `std` and `sqrt-table`. It is currently unused directly, but
will be used after `CurveAffine` is refactored to remove the `std`
dependency.

Closes #25.
@str4d str4d closed this as completed in #26 Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants