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

constant group element API over curve #296

Closed
alxiong opened this issue Jun 7, 2021 · 5 comments · Fixed by #437
Closed

constant group element API over curve #296

alxiong opened this issue Jun 7, 2021 · 5 comments · Fixed by #437

Comments

@alxiong
Copy link
Contributor

alxiong commented Jun 7, 2021

Motivation

Currently there's a way to define constant field element using field_new!(), however, there's no way of defining a constant group element over a curve.
An symptom of this problem in the library is demonstrated here -- being forced to declare generator point by its (x, y) coordinates separately as opposed to directly as a point.

Problem

Currently GroupAffine/GroupProjective::new() is not a pub const fn, and those structs contains a private/inaccessible field _params: PhantomData to be constructed directly, thus there's no way of constructing one.

Failed Attempt

The natural solution is to update new() to pub const fn new(), however, the compiler would give the following complain:

  --> ec/src/models/twisted_edwards_extended.rs:55:6
   |
55 | impl<P: Parameters> GroupAffine<P> {
   |      ^
   |
   = note: see issue #57563 <https:/rust-lang/rust/issues/57563> for more information
   = help: add `#![feature(const_fn)]` to the crate attributes to enable

The strange thing is why can we have the following in impl_Fp!(), but not ☝️ above:

        impl<P> $Fp<P> {
            #[inline]
            pub const fn new(element: $BigIntegerType) -> Self {
                Self(element, PhantomData)
            }

The reason is mostly due to the wacky status quo of limited subset of const fn support in stable Rust. From a friend of mine who knew better about these progress:

having a totally generic type with no restrictions on it is allowed in const fn on stable currently
...
MIRI (Mid-level InteRmediate Interpereter) is too unstable currently to let them stabilize a lot of the remaining const bits
...
MIRI was designed to detect undefined behavior in unsafe code, but kinda got shoved into the role of being used to implement const
so its not yet 100% semantically compatible with the actual rust compiler, partially because its not done cooking, and partially because rust hasn't actually offically adopted stacked borrows yet (https://plv.mpi-sws.org/rustbelt/stacked-borrows/)

Proposed Action

  • wait until respective const fn support in Rust Stable and come back to this issue.

(cc @weikengchen feel free to add more of your GroupAffine<F, P> idea if you find fit)

@weikengchen
Copy link
Member

Note that although const function pub const fn new is hard, const variables may be possible --- it seems that we only need to make PhantomData public.

Previously, we don't want to make PhantomData public because we want to encourage users to use GroupAffine::new(x, y, infinity) to create an element. Now, since the latter cannot be const in the near future, we may start to consider making PhantomData public.

(regarding the cc note: nope, the GroupAffine<F, P>, even if worked, would be too complicated and not worthwhile.)

@Pratyush
Copy link
Member

Pratyush commented Jun 7, 2021

The reason FpN::new() can be const fn is that FpN<P> does not have a trait bound. GroupAffine<P: Parameters> does have a trait bound, which means that any inherent method is bounded by the same bound, which, due to current restrictions on const fn, means that we cannot make these inherent methods const fn.

Note that it's not clear that we want to make all the fields of GroupAffine public, because of the reasons outlined in #268.

I think this issue will eventually be resolved by improvements in rustc, via rust-lang/rfcs#2632 and rust-lang/rust#67792

@Pratyush
Copy link
Member

Pratyush commented Jun 7, 2021

@alxiong did you have a particular use case for this? AFAIK the only time I have ever needed to hardcode a point is when writing down the generator.

@Pratyush
Copy link
Member

Pratyush commented Jun 7, 2021

As a temporary compromise that should achieve the goals of #268, what we can try to do is the following:

  1. Make the x, y, and _params fields public, but annotated with #[doc(hidden)], so that users don't accidentally rely on these.

  2. Make fn x(&self) -> P::BaseField and fn y(&self) -> P::BaseField methods that allow accessing the coordinates immutably

  3. Add sw_new_unchecked and te_new_unchecked macros that enable constructing new curve points without performing on-curve checks, but it is annotated as such. (Not sure if this latter is worth it, as it enables side-stepping all the guarantees that the fix to Change GroupAffine/GroupProjective to maintain invariants #268 would provide)

@alxiong
Copy link
Contributor Author

alxiong commented Jun 8, 2021

did you have a particular use case for this? AFAIK the only time I have ever needed to hardcode a point is when writing down the generator.

It's some application-level logic where we want to use some fixed group point as a constant. (or rather more accurately, some struct of our own that wraps around the point)

Thanks for both the diagnosis and suggestion on temporary hacks @Pratyush !

This issue is really meant to be just tracking the progress on Rust's stable support on const fn, the two aforementioned issues, and update arkwork easily by then.

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 a pull request may close this issue.

3 participants