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

Revive base field type on Curve trait #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vmx
Copy link

@vmx vmx commented Jul 1, 2022

Curves operate on certain base fields. Sometimes in can be useful to have
a direct reference to which field this is. With the Base associate type
you can not get that information.

More concretely this will be used by some GPU code, which needs access to
the information which base field a curve operates on.

BREAKING CHANGE: crates implementing Curve need to define the Base.

To be even more concrete. I'm working on the multiexp implementation for GPUs.
I'd like to be able to bundle implementations for different fields within one kernel,
hence I name the kernel <base-field>_<scalar-field>_multiexp. For that I need
to know which base field the curve has.

There are potential workarounds, but this seems to be the cleanest solution to me.
Updating crates implementing Curve is straightforward, I'd be happy to provide PRs
for those if you let me know which ones they are (I know of e.g. blstrs).

Curves operate on certain base fields. Sometimes in can be useful to have
a direct reference to which field this is. With the `Base` associate type
you can not get that information.

More concretely this will be used by some GPU code, which needs access to
the information which base field a curve operates on.

BREAKING CHANGE: crates implementing `Curve` need to define the `Base`.
@vmx
Copy link
Author

vmx commented Jul 4, 2022

Here's the code what it then would look like if Base would be available: https://github.com/filecoin-project/ec-gpu/blob/fft-no-gpuengine/ec-gpu-gen/src/multiexp.rs#L170-L174

@str4d
Copy link
Member

str4d commented Jul 5, 2022

This change is related to a change I already planned to make, to expose coordinate APIs (much as I dislike them being present because they are bad for protocols that should be relying just on the group APIs, they are necessary for implementing generic EC circuits). I've written up some thoughts in #30.

Your motivation here is different: you want to name the base field, in order to use (base field, scalar field) as an identifier for a multi-scalar multiplication / multiexp implementation (via I presume an extension trait on ff::Field that provides the name method). I am not sure this is an ideal naming scheme, because an MSM algorithm does not take pairs (base field, scalar field) as arguments, or even (curve point, scalar field) (though for a specific group that happens to be an elliptic curve this will be the case), but pairs (group element, scalar field). There could be multiple different kinds of groups defined over the same base and scalar field (e.g. curves in Edwards vs Montgomery form), and conversely there are abstract groups like ristretto255 that have a scalar field but no base field.

Given that a group with multiplication support will have a well-defined scalar field, would it not be better to name the MSM implementation according to the group? That feels to me like it captures all the necessary information, and it could be achieved in the same way you are naming fields, without changes to the group crate.

@vmx
Copy link
Author

vmx commented Jul 7, 2022

Thanks again @str4d for explaining why I wanted it for the wrong reasons (and it was indeed for the wrong reasons). I've updated my code to depend only on the curve point (as that GPU kernel code is elliptic curve specific). Due to those changes, I think I now have a good reason.

As I want to rely on the curve point only, but I need the base field for the GPU kernel source code generation, I currently need to pass it in explicitly. Here's some example code from https://github.com/filecoin-project/ec-gpu/blob/9f7e8d9372b75e1c5f11fe1c933527edfb335e9a/ec-gpu-gen/build.rs#L39-L42:

        Config::new()
            .add_fft::<Scalar>()
            .add_multiexp::<G1Affine, Fp>()
            .add_multiexp::<G2Affine, Fp2>()

If I would've access to the base field, I would only need to rely on G1Affine/G2Affine and wouldn't need to pass in Fp/Fp2 explicitly. Which is what I already do for the Scalar field.

@daira
Copy link
Contributor

daira commented Sep 8, 2022

I have no objection.

(I think that there are good reasons to make the coordinates accessible, although I would prefer that the curve model also be made accessible in that case, since the coordinates only make sense in the context of a curve model. It seems odd to me to declare the base field without making the coordinates accessible, though, since then there's no way to use it without relying on knowledge of the particular curve.)

@str4d
Copy link
Member

str4d commented Jul 29, 2023

If we're going to re-expose the base field type, it will be in the context of #30 (which I'm now back thinking about).

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.

4 participants