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

Implement Component for RangeCheckUnitComponent. #12

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

alonh5
Copy link
Contributor

@alonh5 alonh5 commented Jul 7, 2024

This change is Reviewable

@alonh5 alonh5 requested a review from shaharsamocha7 July 7, 2024 13:41
@alonh5 alonh5 force-pushed the 07-03-update_stwo_and_fix_changes branch from 10914d6 to 21185a0 Compare July 8, 2024 11:55
@alonh5 alonh5 force-pushed the 07-07-implement_component_for_rangecheckunitcomponent branch 2 times, most recently from b4631c6 to 5fe74b3 Compare July 9, 2024 10:12
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 all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alonh5)


stwo_cairo_prover/src/components/range_check_unit.rs line 58 at r1 (raw file):

    fn max_constraint_log_degree_bound(&self) -> u32 {
        self.log_n_instances

The step constraint is of degree 2, right?

Suggestion:

self.log_n_instances + 1?

stwo_cairo_prover/src/components/range_check_unit.rs line 94 at r1 (raw file):

            interaction_elements,
            constraint_zero_domain,
        );

Why not adding all the constraints?

Code quote:

        self.evaluate_lookup_boundary_constraints_at_point(
            point,
            mask,
            evaluation_accumulator,
            interaction_elements,
            constraint_zero_domain,
        );

Copy link
Contributor Author

@alonh5 alonh5 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 1 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


stwo_cairo_prover/src/components/range_check_unit.rs line 58 at r1 (raw file):

Previously, shaharsamocha7 wrote…

The step constraint is of degree 2, right?

Done.


stwo_cairo_prover/src/components/range_check_unit.rs line 94 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Why not adding all the constraints?

All in one PR? It would be harder to review

@alonh5 alonh5 force-pushed the 07-03-update_stwo_and_fix_changes branch from 21185a0 to 73fe30d Compare July 10, 2024 10:46
@alonh5 alonh5 force-pushed the 07-07-implement_component_for_rangecheckunitcomponent branch from 5fe74b3 to 7a7ccc6 Compare July 10, 2024 10:46
@alonh5 alonh5 force-pushed the 07-03-update_stwo_and_fix_changes branch from 73fe30d to 0c40555 Compare July 10, 2024 10:58
@alonh5 alonh5 force-pushed the 07-07-implement_component_for_rangecheckunitcomponent branch from 7a7ccc6 to 5a3f6c8 Compare July 10, 2024 10:58
@alonh5 alonh5 mentioned this pull request Jul 10, 2024
@alonh5 alonh5 force-pushed the 07-03-update_stwo_and_fix_changes branch from 0c40555 to 9a64b0a Compare July 10, 2024 11:04
@alonh5 alonh5 force-pushed the 07-07-implement_component_for_rangecheckunitcomponent branch from 5a3f6c8 to e0f48d2 Compare July 10, 2024 11:04
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.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @alonh5)


stwo_cairo_prover/src/components/range_check_unit.rs line 33 at r2 (raw file):

impl RangeCheckUnitComponent {
    fn evaluate_lookup_boundary_constraints_at_point(

Don't you want to update the eval_at_domain function for the prover in the same pr?

Code quote:

fn evaluate_lookup_boundary_constraints_at_point(

stwo_cairo_prover/src/components/range_check_unit.rs line 73 at r2 (raw file):

        point: CirclePoint<SecureField>,
    ) -> TreeVec<ColumnVec<Vec<CirclePoint<SecureField>>>> {
        TreeVec::new(vec![fixed_mask_points(&vec![vec![0_usize]], point)])

didn't you have that with ;2?

Code quote:

fixed_mask_points(&vec![vec![0_usize]

Copy link
Contributor Author

@alonh5 alonh5 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 1 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


stwo_cairo_prover/src/components/range_check_unit.rs line 33 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Don't you want to update the eval_at_domain function for the prover in the same pr?

It's in the next PR. I can combine them if you prefer.


stwo_cairo_prover/src/components/range_check_unit.rs line 73 at r2 (raw file):

Previously, shaharsamocha7 wrote…

didn't you have that with ;2?

Yes, I fixed it in the next PR.

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.

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alonh5)


stwo_cairo_prover/src/components/range_check_unit.rs line 33 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

It's in the next PR. I can combine them if you prefer.

your call

Copy link
Contributor Author

alonh5 commented Jul 11, 2024

Merge activity

  • Jul 11, 3:20 AM EDT: @alonh5 started a stack merge that includes this pull request via Graphite.
  • Jul 11, 3:25 AM EDT: Graphite rebased this pull request as part of a merge.
  • Jul 11, 3:26 AM EDT: @alonh5 merged this pull request with Graphite.

@alonh5 alonh5 force-pushed the 07-03-update_stwo_and_fix_changes branch from 9a64b0a to 0ac1cf2 Compare July 11, 2024 07:21
@alonh5 alonh5 changed the base branch from 07-03-update_stwo_and_fix_changes to main July 11, 2024 07:23
@alonh5 alonh5 force-pushed the 07-07-implement_component_for_rangecheckunitcomponent branch from e0f48d2 to 9e6c006 Compare July 11, 2024 07:24
@alonh5 alonh5 merged commit d3cfe07 into main Jul 11, 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.

2 participants