Add line folding

[atgrosso]

This change is 

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @atgrosso)

---

stwo_cairo_verifier/src/fri.cairo line 1 at r1 (raw file):

pub mod folding;

Please put contents of fri/line.cairo and fri/folding.cairo in this file.

---

stwo_cairo_verifier/src/utils.cairo line 101 at r1 (raw file):

pub fn bit_reverse_index(mut index: usize, mut bits: u32) -> usize {
    assert!(bits < 32);

Isn't 32 a valid value here?

I guess it should really be

use core::num::traits::BitSize;

..

assert!(bits <= usize::bits());

---

stwo_cairo_verifier/src/utils.cairo line 107 at r1 (raw file):

        result *= 2;
        result = result | ((index / pow_of_two) & 1);
        pow_of_two *= 2;

Saves an op

Suggestion:

        result = (result * 2) | (index & 1);
        index /= 2;

---

stwo_cairo_verifier/src/fri/folding.cairo line 12 at r1 (raw file):

/// Folds a degree `d` polynomial into a degree `d/2` polynomial.
pub fn fold_line(eval: @LineEvaluation, alpha: QM31) -> LineEvaluation {

Suggestion:

/// Folds a degree `d` polynomial into a degree `d/2` polynomial.
///
/// Let `eval` be a polynomial evaluated on line domain `E`, `alpha` be a random field
/// element and `pi(x) = 2x^2 - 1` be the circle's x-coordinate doubling map. This function
/// returns `f' = f0 + alpha * f1` evaluated on `pi(E)` such that `2f(x) = f0(pi(x)) + x *
/// f1(pi(x))`.
pub fn fold_line(eval: @LineEvaluation, alpha: QM31) -> LineEvaluation {

---

stwo_cairo_verifier/src/fri/folding.cairo line 14 at r1 (raw file):

pub fn fold_line(eval: @LineEvaluation, alpha: QM31) -> LineEvaluation {
    let domain = eval.domain;
    let mut values: Array<QM31> = array![];

Suggestion:

    let mut values = array![];

---

stwo_cairo_verifier/src/fri/folding.cairo line 26 at r1 (raw file):

    LineEvaluationImpl::new(domain.double(), values)
}
pub fn ibutterfly(v0: QM31, v1: QM31, itwid: M31) -> (QM31, QM31) {

Suggestion:

}

pub fn ibutterfly(v0: QM31, v1: QM31, itwid: M31) -> (QM31, QM31) {

---

stwo_cairo_verifier/src/fri/folding.cairo line 27 at r1 (raw file):

}
pub fn ibutterfly(v0: QM31, v1: QM31, itwid: M31) -> (QM31, QM31) {
    (v0 + v1, (v0 - v1) * itwid.into())

Suggestion:

).mul_m31(itwid)

---

stwo_cairo_verifier/src/fri/folding.cairo line 29 at r1 (raw file):

    (v0 + v1, (v0 - v1) * itwid.into())
}
mod test {

Suggestion:

}

#[cfg(test)]
mod test {

---

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

    use stwo_cairo_verifier::utils::{bit_reverse_index, pow};
    use stwo_cairo_verifier::fri::folding::{FOLD_STEP, CIRCLE_TO_LINE_FOLD_STEP};
    #[test]

Suggestion:

    use stwo_cairo_verifier::fri::folding::{FOLD_STEP, CIRCLE_TO_LINE_FOLD_STEP};

    #[test]

---

stwo_cairo_verifier/src/fri/folding.cairo line 53 at r1 (raw file):

        let result = sparse_line_evaluation.fold(alpha);
        let expected_result = array![qm31(515899232, 1030391528, 1006544539, 11142505)];
        assert_eq!(expected_result, result);

Suggestion:

        let alpha = qm31(1047716961, 506143067, 1065078409, 990356067);

        let result = sparse_line_evaluation.fold(alpha);

        assert_eq!(result, array![qm31(515899232, 1030391528, 1006544539, 11142505)]);

---

stwo_cairo_verifier/src/fri/folding.cairo line 55 at r1 (raw file):

        assert_eq!(expected_result, result);
    }
    #[test]

Suggestion:

    }

    #[test]

---

stwo_cairo_verifier/src/fri/folding.cairo line 68 at r1 (raw file):

        let result = sparse_line_evaluation.fold(alpha);
        let expected_result = array![qm31(1379727866, 1083096056, 1409020369, 1977903500)];
        assert_eq!(expected_result, result);

Suggestion:

        let alpha = qm31(2084521793, 1326105747, 548635876, 1532708504);
    
        let result = sparse_line_evaluation.fold(alpha);
    
        assert_eq!(result, array![qm31(1379727866, 1083096056, 1409020369, 1977903500)]);

---

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

    /// Coefficients of the polynomial in [line_ifft] algorithm's basis.
    ///
    /// The coefficients are stored in bit-reversed order.

Given line_ifft not here

Suggestion:

/// The coefficients of the polynomial stored in bit-reversed order.
/// 
/// The coefficients are in a basis relating to the circle's x-coordinate doubling 
/// map `pi(x) = 2x^2 - 1` i.e.
///
/// ```text
/// B = { 1 } * { x } * { pi(x) } * { pi(pi(x)) } * ...
///   = { 1, x, pi(x), pi(x) * x, pi(pi(x)), pi(pi(x)) * x, pi(pi(x)) * pi(x), ... }
/// ```

---

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

    /// Returns a domain comprising of the x-coordinates of points in a coset.
    fn new(coset: Coset) -> LineDomain {
        LineDomain { coset: coset }

Please include the asserts.

---

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

/// Holds a small foldable subset of univariate SecureField polynomial evaluations.
/// Evaluation is held at the CPU backend.

Verifier doesn't require a concept of backend

---

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

        };
        return res;
    }

Suggestion:

    fn fold(self: SparseLineEvaluation, alpha: QM31) -> Array<QM31> {
        let mut res = array![];
        for eval in self.subline_evals {
            res.append(fold_line(eval, alpha)[0])
        }
        res
    }

[andrewmilson]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @atgrosso)

---

stwo_cairo_verifier/src/utils.cairo line 107 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Saves an op

Actually this might be better

use core::traits::DivRem;
..
const NZ2: NonZero<usize> = 2;
..
let (next_index, bit) = DivRem::div_rem(index, NZ2);
result = (result * 2) | bit;
index = next_index;

[atgrosso]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @atgrosso)

stwo_cairo_verifier/src/fri.cairo line 1 at r1 (raw file):

pub mod folding;

Please put contents of fri/line.cairo and fri/folding.cairo in this file.

stwo_cairo_verifier/src/utils.cairo line 101 at r1 (raw file):

pub fn bit_reverse_index(mut index: usize, mut bits: u32) -> usize {
    assert!(bits < 32);

Isn't 32 a valid value here?

I guess it should really be

use core::num::traits::BitSize;

..

assert!(bits <= usize::bits());

You are right, using BitSize::bits() bound exactly to usize. Done!

stwo_cairo_verifier/src/utils.cairo line 107 at r1 (raw file):

        result *= 2;
        result = result | ((index / pow_of_two) & 1);
        pow_of_two *= 2;

Saves an op

Suggestion:

        result = (result * 2) | (index & 1);
        index /= 2;

Good catch. Done!

stwo_cairo_verifier/src/fri/folding.cairo line 12 at r1 (raw file):

/// Folds a degree `d` polynomial into a degree `d/2` polynomial.
pub fn fold_line(eval: @LineEvaluation, alpha: QM31) -> LineEvaluation {

Suggestion:

/// Folds a degree `d` polynomial into a degree `d/2` polynomial.
///
/// Let `eval` be a polynomial evaluated on line domain `E`, `alpha` be a random field
/// element and `pi(x) = 2x^2 - 1` be the circle's x-coordinate doubling map. This function
/// returns `f' = f0 + alpha * f1` evaluated on `pi(E)` such that `2f(x) = f0(pi(x)) + x *
/// f1(pi(x))`.
pub fn fold_line(eval: @LineEvaluation, alpha: QM31) -> LineEvaluation {

Done

stwo_cairo_verifier/src/fri/folding.cairo line 14 at r1 (raw file):

pub fn fold_line(eval: @LineEvaluation, alpha: QM31) -> LineEvaluation {
    let domain = eval.domain;
    let mut values: Array<QM31> = array![];

Suggestion:

    let mut values = array![];

Done

stwo_cairo_verifier/src/fri/folding.cairo line 26 at r1 (raw file):

    LineEvaluationImpl::new(domain.double(), values)
}
pub fn ibutterfly(v0: QM31, v1: QM31, itwid: M31) -> (QM31, QM31) {

Suggestion:

}

pub fn ibutterfly(v0: QM31, v1: QM31, itwid: M31) -> (QM31, QM31) {

Done

stwo_cairo_verifier/src/fri/folding.cairo line 27 at r1 (raw file):

}
pub fn ibutterfly(v0: QM31, v1: QM31, itwid: M31) -> (QM31, QM31) {
    (v0 + v1, (v0 - v1) * itwid.into())

Suggestion:

).mul_m31(itwid)

Added

stwo_cairo_verifier/src/fri/folding.cairo line 29 at r1 (raw file):

    (v0 + v1, (v0 - v1) * itwid.into())
}
mod test {

Suggestion:

}

#[cfg(test)]
mod test {

Done

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

    use stwo_cairo_verifier::utils::{bit_reverse_index, pow};
    use stwo_cairo_verifier::fri::folding::{FOLD_STEP, CIRCLE_TO_LINE_FOLD_STEP};
    #[test]

Suggestion:

    use stwo_cairo_verifier::fri::folding::{FOLD_STEP, CIRCLE_TO_LINE_FOLD_STEP};

    #[test]

Done

stwo_cairo_verifier/src/fri/folding.cairo line 53 at r1 (raw file):

        let result = sparse_line_evaluation.fold(alpha);
        let expected_result = array![qm31(515899232, 1030391528, 1006544539, 11142505)];
        assert_eq!(expected_result, result);

Suggestion:

        let alpha = qm31(1047716961, 506143067, 1065078409, 990356067);

        let result = sparse_line_evaluation.fold(alpha);

        assert_eq!(result, array![qm31(515899232, 1030391528, 1006544539, 11142505)]);

Done

stwo_cairo_verifier/src/fri/folding.cairo line 55 at r1 (raw file):

        assert_eq!(expected_result, result);
    }
    #[test]

Suggestion:

    }

    #[test]

Done

stwo_cairo_verifier/src/fri/folding.cairo line 68 at r1 (raw file):

        let result = sparse_line_evaluation.fold(alpha);
        let expected_result = array![qm31(1379727866, 1083096056, 1409020369, 1977903500)];
        assert_eq!(expected_result, result);

Suggestion:

        let alpha = qm31(2084521793, 1326105747, 548635876, 1532708504);
    
        let result = sparse_line_evaluation.fold(alpha);
    
        assert_eq!(result, array![qm31(1379727866, 1083096056, 1409020369, 1977903500)]);

Done

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

    /// Coefficients of the polynomial in [line_ifft] algorithm's basis.
    ///
    /// The coefficients are stored in bit-reversed order.

Given line_ifft not here

Suggestion:

/// The coefficients of the polynomial stored in bit-reversed order.
/// 
/// The coefficients are in a basis relating to the circle's x-coordinate doubling 
/// map `pi(x) = 2x^2 - 1` i.e.
///
/// ```text
/// B = { 1 } * { x } * { pi(x) } * { pi(pi(x)) } * ...
///   = { 1, x, pi(x), pi(x) * x, pi(pi(x)), pi(pi(x)) * x, pi(pi(x)) * pi(x), ... }
/// ```

Yes! good catch. sgtm

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

    /// Returns a domain comprising of the x-coordinates of points in a coset.
    fn new(coset: Coset) -> LineDomain {
        LineDomain { coset: coset }

Please include the asserts.

Done. The tests that use odds are refactored in the next pr (odds is added there)

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

/// Holds a small foldable subset of univariate SecureField polynomial evaluations.
/// Evaluation is held at the CPU backend.

Verifier doesn't require a concept of backend

Oops, removed it

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

        };
        return res;
    }

Suggestion:

    fn fold(self: SparseLineEvaluation, alpha: QM31) -> Array<QM31> {
        let mut res = array![];
        for eval in self.subline_evals {
            res.append(fold_line(eval, alpha)[0])
        }
        res
    }

Done

[atgrosso]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @atgrosso)

stwo_cairo_verifier/src/utils.cairo line 107 at r1 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…

Actually this might be better

use core::traits::DivRem;
..
const NZ2: NonZero<usize> = 2;
..
let (next_index, bit) = DivRem::div_rem(index, NZ2);
result = (result * 2) | bit;
index = next_index;

It looks better to me too. Done

[atgrosso]

stwo_cairo_verifier/src/fri.cairo line 1 at r1 (raw file):

pub mod folding;

Please put contents of fri/line.cairo and fri/folding.cairo in this file.

Done. Currently we only moved folding.cairo on this PR. We'll continue to move the missing files and functions related to fri

[andrewmilson]

Thanks. with couple small comments

Reviewed 2 of 6 files at r1, 3 of 6 files at r2, all commit messages.
Reviewable status: 5 of 8 files reviewed, 18 unresolved discussions (waiting on @atgrosso)

---

stwo_cairo_verifier/src/circle.cairo line 44 at r2 (raw file):

        let sx = x.clone() * x.clone();
        sx.clone() + sx - M31One::one()
    }

The example acts as a Rust docstring test so best to remove.
Also clone can be omitted.

Suggestion:

    /// Applies the circle's x-coordinate doubling map.
    fn double_x(x: M31) -> M31 {
        let sqx = x * x;
        sqx + sqx - M31One::one()
    }

---

stwo_cairo_verifier/src/circle.cairo line 67 at r2 (raw file):

        };
        res
    }

Same here.

Suggestion:

    /// Returns the log order of a point.
    ///
    /// All points have an order of the form `2^k`.
    fn log_order(self: @CirclePointM31) -> u32 {
        // we only need the x-coordinate to check order since the only point
        // with x=1 is the circle's identity
        let mut res = 0;
        let mut cur = self.x.clone();
        while cur != M31One::one() {
            cur = Self::double_x(cur);
            res += 1;
        };
        res
    }

---

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

Previously, atgrosso (Tomás Grosso) wrote…

Done

Don't see any change

[atgrosso]

Thanks. with couple small comments

Reviewed 2 of 6 files at r1, 3 of 6 files at r2, all commit messages.
Reviewable status: 5 of 8 files reviewed, 18 unresolved discussions (waiting on @atgrosso)

stwo_cairo_verifier/src/circle.cairo line 44 at r2 (raw file):

        let sx = x.clone() * x.clone();
        sx.clone() + sx - M31One::one()
    }

The example acts as a Rust docstring test so best to remove. Also clone can be omitted.

Suggestion:

    /// Applies the circle's x-coordinate doubling map.
    fn double_x(x: M31) -> M31 {
        let sqx = x * x;
        sqx + sqx - M31One::one()
    }

Done

stwo_cairo_verifier/src/circle.cairo line 67 at r2 (raw file):

        };
        res
    }

Same here.

Suggestion:

    /// Returns the log order of a point.
    ///
    /// All points have an order of the form `2^k`.
    fn log_order(self: @CirclePointM31) -> u32 {
        // we only need the x-coordinate to check order since the only point
        // with x=1 is the circle's identity
        let mut res = 0;
        let mut cur = self.x.clone();
        while cur != M31One::one() {
            cur = Self::double_x(cur);
            res += 1;
        };
        res
    }

Done

stwo_cairo_verifier/src/poly/line.cairo line 115 at r1 (raw file):
Previously, atgrosso (Tomás Grosso) wrote…

Done

Don't see any change

Sry, i only removed the "return". I didn't see the for cycle.
I tried to use a for but scarb failed with: "not yet implemented: Handle usages of for loop expression"
Maybe the scarb version (nightly-2024-06-15) used don't support the for structure?

Thankss