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

Update existing components #427

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Feb 2, 2025

This change is Reviewable

Copy link
Contributor Author

andrewmilson commented Feb 2, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

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 7 of 27 files at r1.
Reviewable status: 7 of 27 files reviewed, 4 unresolved discussions (waiting on @andrewmilson)


stwo_cairo_verifier/crates/cairo_air/src/components/range_check_vector/rc_19_constraints.cairo line 87 at r1 (raw file):

        .try_into()
        .unwrap())
        .unbox();

Isn't pop_front expensive?
we should consider to remove the arrays and just store the values directly

Code quote:

        (*interaction_mask_values
        .pop_front()
        .unwrap()
        .span()
        .try_into()
        .unwrap())
        .unbox();

stwo_cairo_verifier/crates/cairo_air/src/components/range_check_vector/rc_4_3_constraints.cairo line 170 at r1 (raw file):

    (RangeCheck_4_3_alpha0) * (trace_1_column_0_offset_0)
        + (RangeCheck_4_3_alpha1) * (trace_1_column_1_offset_0)
        - (RangeCheck_4_3_z)

consider to remove this code and add combine function that just take alpha,z

Code quote:

) -> QM31 {
    (RangeCheck_4_3_alpha0) * (trace_1_column_0_offset_0)
        + (RangeCheck_4_3_alpha1) * (trace_1_column_1_offset_0)
        - (RangeCheck_4_3_z)

stwo_cairo_verifier/crates/cairo_air/src/components/memory_id_to_big/constraints_small.cairo line 385 at r1 (raw file):

    sum = sum * random_coeff + constraint_quotient;

    // Constrait 1

Suggestion:

/ Constraint 1

stwo_cairo_verifier/crates/cairo_air/src/components/memory_id_to_big/constraints_small.cairo line 458 at r1 (raw file):

            ],
        ))
        + (claimed_sum) * (qm31(32768, 0, 0, 0)))

I think that maybe this 32768 is wrong, I thought that it should be different number for each component
Will check

Code quote:

+ (claimed_sum) * (qm31(32768, 0, 0, 0)))

Copy link
Contributor Author

@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.

Reviewable status: 7 of 27 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7)


stwo_cairo_verifier/crates/cairo_air/src/components/memory_id_to_big/constraints_small.cairo line 458 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I think that maybe this 32768 is wrong, I thought that it should be different number for each component
Will check

Yeah you're correct. Those changes are in a later PR.
Currently it needs to modified it manually which I forgot to do here.
It's supposed to be 1/(1 << log_size) but the expressions don't currently express this.


stwo_cairo_verifier/crates/cairo_air/src/components/memory_id_to_big/constraints_small.cairo line 385 at r1 (raw file):

    sum = sum * random_coeff + constraint_quotient;

    // Constrait 1

Thanks. Modified in last PR to avoid crazy conflicts

@andrewmilson andrewmilson force-pushed the 01-29-Remove_jump_t_t_f_opcode branch from 34cf445 to ddd1549 Compare February 6, 2025 03:26
@andrewmilson andrewmilson force-pushed the 01-31-Update_existing_components branch from 4508dd4 to 800a68f Compare February 6, 2025 03:26
@andrewmilson andrewmilson force-pushed the 01-29-Remove_jump_t_t_f_opcode branch from ddd1549 to 8b36664 Compare February 6, 2025 03:34
@andrewmilson andrewmilson force-pushed the 01-31-Update_existing_components branch from 800a68f to 74fb08a Compare February 6, 2025 03:34
@andrewmilson andrewmilson force-pushed the 01-29-Remove_jump_t_t_f_opcode branch from 8b36664 to 50816c6 Compare February 6, 2025 04:59
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