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 LinePoly #26

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Add LinePoly #26

merged 1 commit into from
Sep 19, 2024

Conversation

ajgara
Copy link
Contributor

@ajgara ajgara commented Jul 16, 2024

Add the implementation of LinePoly and eval_at_point, as needed for the Fri Verifier.


This change is Reviewable

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @ajgara)


stwo_cairo_verifier/src/poly/line.cairo line 6 at r1 (raw file):

#[derive(Drop, Clone)]
pub struct LinePoly {
    pub coeffs: Array<SecureField>,

Those are in bit-reverse order, right?
Please add documentation

Code quote:

pub coeffs: Array<SecureField>,

stwo_cairo_verifier/src/poly/line.cairo line 16 at r1 (raw file):

    }

    fn eval_at_point(self: @LinePoly, mut x: SecureField) -> SecureField {

Add a TODO to consider replacing the eval_at_point with fft

Code quote:

 fn eval_at_point(self: @LinePoly, mut x: SecureField) -> SecureField {

stwo_cairo_verifier/src/poly/line.cairo line 21 at r1 (raw file):

        while i < *self.log_size {
            mappings.append(x);
            x = m31(2).into() * x * x - m31(1).into();

more efficient

Suggestion:

let x_square = x * x; 
x = x_square + x_square - m31(1).into();

stwo_cairo_verifier/src/poly/line.cairo line 40 at r1 (raw file):

        };

        *level[0]

Add an assert that level is of length 1.

Code quote:

        *level[0]

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ajgara)

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ajgara)


stwo_cairo_verifier/src/poly/line.cairo line 13 at r3 (raw file):

/// reversed order is {b₀, b₄, b₂, b₆}.
/// Then, the polynomial p represented by coeffs = [a₀, a₁, a₂, a₃] and log_size = 2 is
/// p = a₀ * b₀ + a₁ * b₄ + a₂ * b₂ + a₃ * b₆.

Please avoid of using unicode chars.
I think that the first line is enough for this doc.

Suggestion:

/// A univariate polynomial represented by its coefficients in the line part of the FFT-basis
/// in bit reversed order.

@schouhy
Copy link
Contributor

schouhy commented Jul 29, 2024

Sure! done.

Copy link
Contributor

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @ajgara and @shaharsamocha7)


stwo_cairo_verifier/src/poly/line.cairo line 42 at r4 (raw file):

            };
            level = new_level;
        };

Can this be done recursively to reflect the code in the Rust implementation https://github.com/starkware-libs/stwo/blob/e4014820ee79ab32025d9bcb80d1bfe8d277d61b/src/core/poly/line.rs#L106

Code quote:

        let mappings = @mappings;
        let mut i = mappings.len();
        while i > 0 {
            i -= 1;
            let mut new_level = array![];
            let mut j = 0;
            while j < level.len() {
                new_level.append(*level[j] + *mappings[i] * *level[j + 1]);
                j += 2;
            };
            level = new_level;
        };

stwo_cairo_verifier/src/poly/line.cairo line 65 at r4 (raw file):

        };
        let x = m31(590768354);
        let result = line_poly.eval_at_point(x.into());

Arrange Act Assert

Suggestion:

   
   let result = line_poly.eval_at_point(x.into());
   

stwo_cairo_verifier/src/poly/line.cairo line 76 at r4 (raw file):

        };
        let x = m31(10);
        let result = line_poly.eval_at_point(x.into());

Arrange Act Assert

Suggestion:

        
        let result = line_poly.eval_at_point(x.into());
        

Copy link
Contributor Author

@ajgara ajgara left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


stwo_cairo_verifier/src/poly/line.cairo line 13 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Please avoid of using unicode chars.
I think that the first line is enough for this doc.

Done.


stwo_cairo_verifier/src/poly/line.cairo line 42 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Can this be done recursively to reflect the code in the Rust implementation https://github.com/starkware-libs/stwo/blob/e4014820ee79ab32025d9bcb80d1bfe8d277d61b/src/core/poly/line.rs#L106

Done.


stwo_cairo_verifier/src/poly/line.cairo line 65 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Arrange Act Assert

Done.


stwo_cairo_verifier/src/poly/line.cairo line 76 at r4 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Arrange Act Assert

Done.

Copy link
Contributor Author

@ajgara ajgara left a comment

Choose a reason for hiding this comment

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

Let us know if there's anything else to improve! I'm not used to this tool haha.

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

Copy link
Contributor

@andrewmilson andrewmilson left a comment

Choose a reason for hiding this comment

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

:lgtm: with comments

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @ajgara and @shaharsamocha7)


stwo_cairo_verifier/src/poly/line.cairo line 1 at r5 (raw file):

use core::array::ArrayTrait;

Is this needed?


stwo_cairo_verifier/src/poly/line.cairo line 20 at r5 (raw file):

    }

    // TODO: Consider replacing with FFT.

I don't think "replacing with FFT" is necessary. The protocol only uses LinePoly to evaluate at a single point.


stwo_cairo_verifier/src/poly/utils.cairo line 4 at r5 (raw file):

pub fn fold(

Please copy over docs from rust.

Co-authored-by: ajgara <[email protected]>
Copy link
Contributor

@carlogf carlogf left a comment

Choose a reason for hiding this comment

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

Hi! I addressed the comments and rebased with the last commit in main

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @ajgara, @andrewmilson, and @shaharsamocha7)


stwo_cairo_verifier/src/poly/line.cairo line 1 at r5 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Is this needed?

Not needed, removed it


stwo_cairo_verifier/src/poly/line.cairo line 20 at r5 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

I don't think "replacing with FFT" is necessary. The protocol only uses LinePoly to evaluate at a single point.

makes sense :)

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r5, 1 of 2 files at r6, all commit messages.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @ajgara, @andrewmilson, and @carlogf)

Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ajgara and @andrewmilson)

@andrewmilson andrewmilson merged commit a02de6f into starkware-libs:main Sep 19, 2024
4 checks passed
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.

5 participants