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

elliptic_curve: add BatchInvert and BatchNormalize traits #1376

Merged
merged 42 commits into from
Nov 14, 2023

Conversation

ycscaly
Copy link
Contributor

@ycscaly ycscaly commented Nov 9, 2023

elliptic-curve/src/ops.rs Outdated Show resolved Hide resolved
@ycscaly
Copy link
Contributor Author

ycscaly commented Nov 9, 2023

Also, I'd like to write tests but I couldn't as dev::Scalar doesn't implement Invert, and importing k256 doesn't work. What should I do

@ycscaly
Copy link
Contributor Author

ycscaly commented Nov 9, 2023

@tarcieri what do you think of this design? this allows for both a const-generic implementation and a dynamically alloced one while reusing the code. If you like it, I'd like to do the same with lincomb

@ycscaly ycscaly changed the title Introduced InvertBatch and implemented elliptic_curve: Batch Inversion Nov 9, 2023
elliptic-curve/src/ops.rs Outdated Show resolved Hide resolved
elliptic-curve/src/ops.rs Outdated Show resolved Hide resolved
elliptic-curve/src/ops.rs Outdated Show resolved Hide resolved
elliptic-curve/src/ops.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

tarcieri commented Nov 12, 2023

I'm reconsidering whether InvertBatch being a trait is a good idea. It seems like it could just be two free functions, e.g.:

pub fn invert_batch_generic<T, const N: usize>(field_elements: &[T; N]) -> CtOption<[T; N]> {
where
    T: Invert<Output = CtOption<Self>>
        + Mul<T, Output = T>
        + Copy
        + Default
        + ConditionallySelectable,

It just seems a little weird that there's invert_batch which is gated on the trait by alloc, but it's really the implementation that requires alloc.

@tarcieri
Copy link
Member

As another option: they could potentially be provided methods on the Invert trait

@ycscaly
Copy link
Contributor Author

ycscaly commented Nov 12, 2023

As another option: they could potentially be provided methods on the Invert trait

Can't do that, as the output from the Invert trait is a generic type Output, I need to work with CtOption<Self>

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now, although I'd like to see RustCrypto/elliptic-curves#971 green before merging

@tarcieri
Copy link
Member

What issues did you run into with the AsRef bound? It seems handy for writing generic code

@ycscaly
Copy link
Contributor Author

ycscaly commented Nov 14, 2023

What issues did you run into with the AsRef bound? It seems handy for writing generic code

The Rust compiler complained that it didn't provided it. We can try to fight it if its important

@tarcieri tarcieri changed the title elliptic_curve: Batch Inversion elliptic_curve: add BatchInvert and BatchNormalize traits Nov 14, 2023
@tarcieri tarcieri merged commit 2af0350 into RustCrypto:master Nov 14, 2023
11 checks passed
@ycscaly
Copy link
Contributor Author

ycscaly commented Nov 14, 2023

Thanks!

@tarcieri tarcieri mentioned this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants