verify and ret #166

ohad-starkware · 2024-11-13T07:53:17Z

This change is

ohad-starkware · 2024-11-13T07:53:33Z

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

Join @ohad-starkware and the rest of your teammates on Graphite

shaharsamocha7

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ohad-starkware)

stwo_cairo_prover/crates/prover/src/components/ret_opcode/prover.rs line 39 at r1 (raw file):

        let cpu_inputs = cpu_inputs
            .into_iter()
            .map(|VmState { pc, ap, fp }| CasmState {

This should be rename to VmState

Code quote:

CasmState

stwo_cairo_prover/crates/prover/src/components/ret_opcode/prover.rs line 278 at r1 (raw file):

                M31_0,
                M31_0,
                M31_0,

I thought we got rid of those

Code quote:

stwo_cairo_prover/crates/prover/src/cairo_air/mod.rs line 346 at r1 (raw file):

        memory_id_to_value_trace_generator
            .write_trace(&mut tree_builder, &mut range_check_9_9_trace_generator);
    let (vi_claim, verify_instruction_interaction_generator) = verify_instruction_trace_generator

maybe we want to shorten all the names, but let's be consistent for now and rethink in the future.

Suggestion:

verify_instruction_claim

stwo_cairo_prover/crates/prover/src/components/mod.rs line 11 at r1 (raw file):

pub mod verifyinstruction;

pub fn pack_values<T: Pack>(values: &[T]) -> Vec<T::SimdType> {

Is this Pack trait new?
Are we going to implement it for every type?
I wonder if this function should be near the trait declaration itself

Code quote:

pub fn pack_values<T: Pack>(values: &[T]) -> Vec<T::SimdType> {

stwo_cairo_prover/crates/prover/src/components/ret_opcode/component.rs line 75 at r1 (raw file):

impl InteractionClaim {
    pub fn mix_into(&self, channel: &mut impl Channel) {
        // TODO(Ohad): mix claimed_sum.

Can't you implement this TODO now?

Code quote:

// TODO(Ohad): mix claimed_sum.

stwo_cairo_prover/crates/prover/src/components/opcodes/mod.rs line 3 at r1 (raw file):

use stwo_prover::constraint_framework::logup::LookupElements;

pub type RelationElements = LookupElements<3>;

Add a comment that this refers to the VmState?
Actually, why the VmState isn't defined here?

Suggestion:

pub type VmRelationElements = LookupElements<3>;

ohad-starkware

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7)

stwo_cairo_prover/crates/prover/src/cairo_air/mod.rs line 346 at r1 (raw file):

Previously, shaharsamocha7 wrote…

maybe we want to shorten all the names, but let's be consistent for now and rethink in the future.

Done.

stwo_cairo_prover/crates/prover/src/components/mod.rs line 11 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Is this Pack trait new?
Are we going to implement it for every type?
I wonder if this function should be near the trait declaration itself

it's a trait in stwo, already implemented recursively for every type that is M31- based (arrays, tupples, array of tupples etc.).
do you think the function also belongs in two? I'm not sure, using this as a utility here.

stwo_cairo_prover/crates/prover/src/components/opcodes/mod.rs line 3 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Add a comment that this refers to the VmState?
Actually, why the VmState isn't defined here?

CasmState - defined by the air team as a prover type,
we should kill papini's vmstate and rename CasmState to VmState, but that would take coordinating with the air team

stwo_cairo_prover/crates/prover/src/components/ret_opcode/component.rs line 75 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Can't you implement this TODO now?

done

stwo_cairo_prover/crates/prover/src/components/ret_opcode/prover.rs line 39 at r1 (raw file):

Previously, shaharsamocha7 wrote…

This should be rename to VmState

same answer as above

stwo_cairo_prover/crates/prover/src/components/ret_opcode/prover.rs line 278 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I thought we got rid of those

in constraints yes
trace_gen is a bit more tricky, because I'm generating the struct appropriately, and it would mean inlining the arrays in lookup_data
here there's only one call to verify so should be okay, changed manually.

ohad-starkware

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

stwo_cairo_prover/crates/prover/src/components/ret_opcode/prover.rs line 278 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

in constraints yes
trace_gen is a bit more tricky, because I'm generating the struct appropriately, and it would mean inlining the arrays in lookup_data
here there's only one call to verify so should be okay, changed manually.

onecall to memory**

shaharsamocha7

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)

stwo_cairo_prover/crates/prover/src/components/mod.rs line 11 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

it's a trait in stwo, already implemented recursively for every type that is M31- based (arrays, tupples, array of tupples etc.).
do you think the function also belongs in two? I'm not sure, using this as a utility here.

I think this function should be near the trait.
Not sure any of those belong to Stwo TBH

stwo_cairo_prover/crates/prover/src/components/opcodes/mod.rs line 3 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

CasmState - defined by the air team as a prover type,
we should kill papini's vmstate and rename CasmState to VmState, but that would take coordinating with the air team

ok, let's send them a message about that

stwo_cairo_prover/crates/prover/src/components/verifyinstruction/component.rs line 93 at r2 (raw file):

pub struct InteractionClaim {
    pub total_sum: SecureField,
    pub claimed_sum: Option<ClaimedPrefixSum>,

AlonT pr has new type names LogupSums
pub type LogupSums = (SecureField, Option<ClaimedPrefixSum>);

We should consider to integrate with that, not for this pr

Code quote:

    pub total_sum: SecureField,
    pub claimed_sum: Option<ClaimedPrefixSum>,

shaharsamocha7

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ohad-starkware)

stwo_cairo_prover/crates/prover/src/components/verifyinstruction/prover.rs line 310 at r1 (raw file):

                M31_0,
                M31_0,
                M31_0,

I don't think we want to do this manual changes.
We will probably generate those files again, so we want to track the changes

Code quote:

                    + ((input_col18) * (M31_256))),
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,
                M31_0,

ohad-starkware

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

stwo_cairo_prover/crates/prover/src/components/mod.rs line 11 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I think this function should be near the trait.
Not sure any of those belong to Stwo TBH

they both belong in "stwo_air_utils", but it doesn't exist

stwo_cairo_prover/crates/prover/src/components/opcodes/mod.rs line 3 at r1 (raw file):

Previously, shaharsamocha7 wrote…

ok, let's send them a message about that

sent

stwo_cairo_prover/crates/prover/src/components/verifyinstruction/component.rs line 93 at r2 (raw file):

Previously, shaharsamocha7 wrote…

AlonT pr has new type names LogupSums
pub type LogupSums = (SecureField, Option<ClaimedPrefixSum>);

We should consider to integrate with that, not for this pr

will consider

stwo_cairo_prover/crates/prover/src/components/verifyinstruction/prover.rs line 310 at r1 (raw file):

Previously, shaharsamocha7 wrote…

I don't think we want to do this manual changes.
We will probably generate those files again, so we want to track the changes

reverted

shaharsamocha7

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

stwo_cairo_prover/crates/prover/src/components/mod.rs line 11 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

they both belong in "stwo_air_utils", but it doesn't exist

so, TODO?

ohad-starkware · 2024-11-14T13:43:16Z

Merge activity

Nov 14, 8:43 AM EST: A user started a stack merge that includes this pull request via Graphite.
Nov 14, 8:43 AM EST: A user merged this pull request with Graphite.

This was referenced Nov 13, 2024

memory small table #165

Merged

pack for casm #160

Merged

addr_to_id fixes #161

Merged

ohad-starkware force-pushed the ohad/verify_and_ret branch from e9ce65a to fde4539 Compare November 13, 2024 07:55

ohad-starkware mentioned this pull request Nov 13, 2024

range check small table #167

Merged

ohad-starkware requested a review from shaharsamocha7 November 13, 2024 08:36

ohad-starkware self-assigned this Nov 13, 2024

ohad-starkware mentioned this pull request Nov 13, 2024

public memory #168

Merged

ohad-starkware force-pushed the ohad/memory_small_table branch from 2214020 to 6c4f481 Compare November 13, 2024 10:50

ohad-starkware force-pushed the ohad/verify_and_ret branch from fde4539 to 2bf099c Compare November 13, 2024 10:50

ohad-starkware force-pushed the ohad/memory_small_table branch from 6c4f481 to 89093cf Compare November 13, 2024 13:59

ohad-starkware force-pushed the ohad/verify_and_ret branch from 2bf099c to 226d903 Compare November 13, 2024 13:59

ohad-starkware force-pushed the ohad/memory_small_table branch 2 times, most recently from 48fa695 to b13cf94 Compare November 13, 2024 14:01

ohad-starkware force-pushed the ohad/verify_and_ret branch from 226d903 to 7957833 Compare November 13, 2024 14:01

ohad-starkware force-pushed the ohad/memory_small_table branch 3 times, most recently from e1fc64b to f4c76f5 Compare November 14, 2024 07:13

shaharsamocha7 requested changes Nov 14, 2024

View reviewed changes

ohad-starkware force-pushed the ohad/memory_small_table branch from f4c76f5 to b2c3ecc Compare November 14, 2024 09:22

ohad-starkware commented Nov 14, 2024

View reviewed changes

ohad-starkware force-pushed the ohad/verify_and_ret branch from 7957833 to 3890805 Compare November 14, 2024 09:45

ohad-starkware commented Nov 14, 2024

View reviewed changes

shaharsamocha7 approved these changes Nov 14, 2024

View reviewed changes

shaharsamocha7 requested changes Nov 14, 2024

View reviewed changes

ohad-starkware force-pushed the ohad/memory_small_table branch from b2c3ecc to 6c78000 Compare November 14, 2024 11:01

ohad-starkware changed the base branch from ohad/memory_small_table to graphite-base/166 November 14, 2024 11:35

verify and ret

c851891

ohad-starkware force-pushed the ohad/verify_and_ret branch from 3890805 to c851891 Compare November 14, 2024 12:17

ohad-starkware changed the base branch from graphite-base/166 to main November 14, 2024 12:17

ohad-starkware commented Nov 14, 2024

View reviewed changes

shaharsamocha7 approved these changes Nov 14, 2024

View reviewed changes

ohad-starkware merged commit 9189828 into main Nov 14, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verify and ret #166

verify and ret #166

ohad-starkware commented Nov 13, 2024 •

edited by shaharsamocha7

Loading

ohad-starkware commented Nov 13, 2024 •

edited

Loading

shaharsamocha7 left a comment

ohad-starkware left a comment

ohad-starkware left a comment

shaharsamocha7 left a comment

shaharsamocha7 left a comment

ohad-starkware left a comment

shaharsamocha7 left a comment

ohad-starkware commented Nov 14, 2024 •

edited

Loading

verify and ret #166

verify and ret #166

Conversation

ohad-starkware commented Nov 13, 2024 • edited by shaharsamocha7 Loading

ohad-starkware commented Nov 13, 2024 • edited Loading

shaharsamocha7 left a comment

Choose a reason for hiding this comment

ohad-starkware left a comment

Choose a reason for hiding this comment

ohad-starkware left a comment

Choose a reason for hiding this comment

shaharsamocha7 left a comment

Choose a reason for hiding this comment

shaharsamocha7 left a comment

Choose a reason for hiding this comment

ohad-starkware left a comment

Choose a reason for hiding this comment

shaharsamocha7 left a comment

Choose a reason for hiding this comment

ohad-starkware commented Nov 14, 2024 • edited Loading

Merge activity

ohad-starkware commented Nov 13, 2024 •

edited by shaharsamocha7

Loading

ohad-starkware commented Nov 13, 2024 •

edited

Loading

ohad-starkware commented Nov 14, 2024 •

edited

Loading