-
Notifications
You must be signed in to change notification settings - Fork 135
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
Circle starks #924
base: main
Are you sure you want to change the base?
Circle starks #924
Conversation
…rks into optimize_mersenne31
…rks into optimize_mersenne31
…rks into optimize_mersenne31
This reverts commit 21d09c6.
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 PR looks good, left some nits, comments, and suggestions. In general, I'd try to remove every change for the code to panic (even if we know for certain that indexing in some index will always be valid).
math/src/circle/point.rs
Outdated
pub fn zero() -> Self { | ||
Self::new(FieldElement::one(), FieldElement::zero()).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.
This suggestion is not formatted, I recommend not to commit this one.
Note
I'm not sure whether FieldElement::one() or FieldElement::zero() are const
(they should though). If they're not, you'd need to hardcode the values to make this function const
.
pub fn zero() -> Self { | |
Self::new(FieldElement::one(), FieldElement::zero()).unwrap() | |
} | |
pub const fn zero() -> Self { | |
CirclePoint { x: FieldElement::one(), y: FieldElement::zero() } | |
} |
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.
This should be a const
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.
Good catch! Just updated my suggestion
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.
FieldElement::one()
and FieldElement::zero()
aren't const
. I understand they should be, but to fix it we have to change it in every field of Lambdaworks. Maybe we can do it in another PR. If we wanted to hardcode the values, we should add them as parameters for every field we wanted to implement the Circle, I don't know if that is what you are suggesting.
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.
CirclePoint
UX and code's readability could be very much improved if we implement:
For CirclePoint
addition (this would replace add
implementation):
Add<&CirclePoint<F>>
for&CirclePoint<F>
.Add<CirclePoint<F>>
forCirclePoint<F>
.Add<CirclePoint<F>>
for&CirclePoint<F>
.Add<&CirclePoint<F>>
forCirclePoint<F>
.AddAssign<&CirclePoint<F>>
for&CirclePoint<F>
.AddAssign<CirclePoint<F>>
forCirclePoint<F>
.AddAssign<CirclePoint<F>>
for&CirclePoint<F>
.AddAssign<&CirclePoint<F>>
forCirclePoint<F>
.
For CirclePoint
scalar multiplication (this would replace scalar_mul
implementation):
Mul<u128> for CirclePoint<F>
.Mul<u128> for &CirclePoint<F>
.MulAssign<u128> for CirclePoint<F>
.MulAssign<u128> for &CirclePoint<F>
.
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.
Ok! We replaced add
for Add for &CirclePoint<F>
and scalar_mul
for Mul<u128> for CirclePoint<F>
. Should we implement the other ones you mentioned here so we can add non-referenced values, multiply referenced values, etc.?
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.
Yes!
Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #924 +/- ##
==========================================
+ Coverage 71.12% 71.66% +0.53%
==========================================
Files 144 149 +5
Lines 31880 32516 +636
==========================================
+ Hits 22676 23302 +626
- Misses 9204 9214 +10 ☔ View full report in Codecov by Sentry. |
/// As a result we obtain the coefficients of the polynomial in the basis: {1, y, x, xy, 2xˆ2 -1, 2xˆ2y-y, 2xˆ3-x, 2xˆ3y-xy,...} | ||
/// Note that eval has to be a vector of length a power of two 2^n. | ||
#[cfg(feature = "alloc")] | ||
pub fn interpolate_cfft( |
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.
As an optimization, we could just send the twiddles as input to the function. This is useful when we have to interpolate multiple columns
/// returns the evaluation of the polynomial on the points of the standard coset of size 2^n. | ||
/// Note that coeff has to be a vector with length a power of two 2^n. | ||
#[cfg(feature = "alloc")] | ||
pub fn evaluate_cfft( |
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.
Similar comment to the one after this one. Twiddles could be given as inputs
pub fn order_cfft_result_naive( | ||
input: &mut [FieldElement<Mersenne31Field>], | ||
) -> Vec<FieldElement<Mersenne31Field>> { | ||
let mut result = Vec::new(); |
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.
maybe we can use interleave
Circle Starks
This PR introduces the foundational components necessary for the Circle Starks protocol implementation.
Key Features:
Type of change
Checklist