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

atomic mults in verify #328

Merged
merged 1 commit into from
Jan 9, 2025
Merged

atomic mults in verify #328

merged 1 commit into from
Jan 9, 2025

Conversation

ohad-starkware
Copy link
Contributor

@ohad-starkware ohad-starkware commented Jan 8, 2025

This change is Reviewable

Copy link
Contributor Author

ohad-starkware commented Jan 8, 2025

@ohad-starkware ohad-starkware changed the base branch from main to graphite-base/328 January 8, 2025 09:12
@ohad-starkware ohad-starkware changed the base branch from graphite-base/328 to ohad/refactor_instructions_by_pc January 8, 2025 09:12
@ohad-starkware ohad-starkware self-assigned this Jan 8, 2025
@ohad-starkware ohad-starkware changed the base branch from ohad/refactor_instructions_by_pc to graphite-base/328 January 8, 2025 14:21
@ohad-starkware ohad-starkware changed the base branch from graphite-base/328 to main January 8, 2025 14:22
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 5 files at r1, 2 of 2 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @ohad-starkware)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 178 at r3 (raw file):

    pub final_state: CasmState,
    pub casm_states_by_opcode: CasmStatesByOpcode,
    pub instruction_by_pc: HashMap<M31, u64>,

We forgot that in the previous pr?

Code quote:

  pub instruction_by_pc: HashMap<M31, u64>,

stwo_cairo_prover/crates/prover/src/components/verify_instruction/prover.rs line 49 at r3 (raw file):

#[derive(Default)]
pub struct ClaimGenerator {
    /// pc -> instruction.

?

Suggestion:

    /// pc -> encoded_instruction.

stwo_cairo_prover/crates/prover/src/components/verify_instruction/prover.rs line 50 at r3 (raw file):

pub struct ClaimGenerator {
    /// pc -> instruction.
    instructions_by_pc: HashMap<M31, u64>,

Nit

Suggestion:

instructions: HashMap<M31, u64>,

stwo_cairo_prover/crates/prover/src/components/verify_instruction/prover.rs line 146 at r3 (raw file):

    }

    // We only care about PC, the the instruction is propagated by the adapter.

Nit
Currently you wrote "the the instruction"

Suggestion:

    // Instruction is determined by PC only.

stwo_cairo_prover/crates/prover/src/components/verify_instruction/prover.rs line 147 at r3 (raw file):

    // We only care about PC, the the instruction is propagated by the adapter.
    fn add_input(&self, (pc, ..): &InputType) {

Doesn't it mean that you send the entire thing for add_inputs?

Code quote:

    fn add_input(&self, (pc, ..): &InputType) {

Copy link
Contributor Author

@ohad-starkware ohad-starkware 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: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/components/verify_instruction/prover.rs line 49 at r3 (raw file):

Previously, shaharsamocha7 wrote…

?

yes


stwo_cairo_prover/crates/prover/src/components/verify_instruction/prover.rs line 147 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Doesn't it mean that you send the entire thing for add_inputs?

yes that is what will happen because of autogen, I assume it will get optimized out, and Im not doing anything with it anyway


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 178 at r3 (raw file):

Previously, shaharsamocha7 wrote…

We forgot that in the previous pr?

yes

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 5 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ohad-starkware)


stwo_cairo_prover/crates/prover/src/input/decode.rs line 73 at r4 (raw file):

///
/// The Deconstructed instruction in the form of (offsets, flags): ([M31;3], [M31;15]).
pub fn deconstruct_instruction_into_felts(mut encoded_instr: u64) -> ([M31; 3], [M31; 15]) {

NIT rename

Suggestion:

pub fn deconstruct_instruction(mut encoded_instr: u64) -> ([M31; 3], [M31; 15]) {

stwo_cairo_prover/crates/prover/src/input/decode.rs line 73 at r4 (raw file):

///
/// The Deconstructed instruction in the form of (offsets, flags): ([M31;3], [M31;15]).
pub fn deconstruct_instruction_into_felts(mut encoded_instr: u64) -> ([M31; 3], [M31; 15]) {

Add a unit test to this function, or a TODO

Copy link
Contributor Author

@ohad-starkware ohad-starkware 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: all files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)


stwo_cairo_prover/crates/prover/src/input/decode.rs line 73 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Add a unit test to this function, or a TODO

done

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 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ohad-starkware)

Copy link
Contributor Author

ohad-starkware commented Jan 9, 2025

Merge activity

  • Jan 9, 3:53 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 9, 3:53 AM EST: A user merged this pull request with Graphite.

@ohad-starkware ohad-starkware merged commit 2c3edec into main Jan 9, 2025
7 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