support builtins in the adapter

[Stavbe]

This change is 

[ohad-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r1 (raw file):

use cairo_vm::air_public_input::MemorySegmentAddresses;

// TODO(STAV): Understand if the adapter should pass builtins that won't be supported by stwo.

Suggestion:

TODO(Stav)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

#[derive(Debug)]
pub struct BuiltinsSegments {
    pub range_check_bits_128_builtin: MemorySegmentAddresses,

struct represents only buildtins, no need to mention that

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 22 at r1 (raw file):

}

impl Default for BuiltinsSegments {

please remove that

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 136 at r1 (raw file):

                    }
                }
                // Output, executaion and program segments are not builtins.

typo

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 144 at r1 (raw file):

        }
        res
    }

better?

Suggestion:

pub struct BuiltinsSegments {
    pub range_check_bits_128_builtin: Option<MemorySegmentAddresses>,
    pub range_check_bits_96_builtin: Option<MemorySegmentAddresses>,
    pub bitwise_builtin: Option<MemorySegmentAddresses>,
    pub add_mod_builtin: Option<MemorySegmentAddresses>,
    pub ec_op_builtin: Option<MemorySegmentAddresses>,
    pub ecdsa_builtin: Option<MemorySegmentAddresses>,
    pub keccak_builtin: Option<MemorySegmentAddresses>,
    pub mul_mod_builtin: Option<MemorySegmentAddresses>,
    pub pedersen_builtin: Option<MemorySegmentAddresses>,
    pub poseidon_builtin: Option<MemorySegmentAddresses>,
}
impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,
    ) -> Self {
        let mut res = BuiltinsSegments::default();
        for (name, value) in memory_segments.iter() {
            match *name {
                "range_check" => {
                    res.range_check_bits_128_builtin =
                        Some((value.begin_addr, value.stop_ptr).into())
                }

                "range_check96" => {
                    res.range_check_bits_96_builtin =
                        Some((value.begin_addr, value.stop_ptr).into())
                }
                "bitwise" => res.bitwise_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "add_mod" => res.add_mod_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "ec_op" => res.ec_op_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "ecdsa" => res.ecdsa_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "keccak" => res.keccak_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "mul_mod" => res.mul_mod_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "pedersen" => {
                    res.pedersen_builtin = Some((value.begin_addr, value.stop_ptr).into())
                }
                "poseidon" => {
                    res.poseidon_builtin = Some((value.begin_addr, value.stop_ptr).into())
                }

                // Output, executaion and program segments are not builtins.
                "output" => {}
                "execution" => {}
                "program" => {}
                _ => panic!("Unknown memory segment: {name}"),
            }
        }
        res
    }
}

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 217 at r1 (raw file):

        assert_eq!(components.mul_opcode_is_small_t_is_imm_t.len(), 0);
        assert_eq!(components.mul_opcode_is_small_t_is_imm_f.len(), 0);
        assert_eq!(components.mul_opcode_is_small_f_is_imm_f.len(), 11146);

why are these changed? are you running with dev mode on?
turn it off

Code quote:

l

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 300 at r1 (raw file):

        let input = small_cairo_input();

        // Check opcode components.

Suggestion:

Test

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 359 at r1 (raw file):

        assert_eq!(components.ret_opcode.len(), 462);

        // Check builtins

same and point at eol.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 360 at r1 (raw file):

        // Check builtins
        let builtins = input.builtins_segments;

Suggestion:

builtins_segments

---

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

            })
        });
    let trace = runner.relocated_trace.clone().unwrap();

it's a large object
its possible to reorder the code and avoid this

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 219 at r1 (raw file):

    }

    // When not ignored, the test passes only with dev_mod = false.

is this related?

[ohad-starkware]

Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 145 at r1 (raw file):

        res
    }
}

better!

Suggestion:

// TODO(STAV): Understand if the adapter should pass builtins that won't be supported by stwo.
/// This struct holds the builtins used in a Cairo program.
/// Most of them are not implemented yet by Stwo.
#[derive(Debug, Default)]
pub struct BuiltinsSegments {
    pub range_check_bits_128_builtin: Option<MemorySegmentAddresses>,
    pub range_check_bits_96_builtin: Option<MemorySegmentAddresses>,
    pub bitwise_builtin: Option<MemorySegmentAddresses>,
    pub add_mod_builtin: Option<MemorySegmentAddresses>,
    pub ec_op_builtin: Option<MemorySegmentAddresses>,
    pub ecdsa_builtin: Option<MemorySegmentAddresses>,
    pub keccak_builtin: Option<MemorySegmentAddresses>,
    pub mul_mod_builtin: Option<MemorySegmentAddresses>,
    pub pedersen_builtin: Option<MemorySegmentAddresses>,
    pub poseidon_builtin: Option<MemorySegmentAddresses>,
}
impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,
    ) -> Self {
        let mut res = BuiltinsSegments::default();
        for (name, value) in memory_segments.iter() {
            let value = (value.begin_addr, value.stop_ptr).into();
            match *name {
                "range_check" => res.range_check_bits_128_builtin = Some(value),
                "range_check96" => res.range_check_bits_96_builtin = Some(value),
                "bitwise" => res.bitwise_builtin = Some(value),
                "add_mod" => res.add_mod_builtin = Some(value),
                "ec_op" => res.ec_op_builtin = Some(value),
                "ecdsa" => res.ecdsa_builtin = Some(value),
                "keccak" => res.keccak_builtin = Some(value),
                "mul_mod" => res.mul_mod_builtin = Some(value),
                "pedersen" => res.pedersen_builtin = Some(value),
                "poseidon" => res.poseidon_builtin = Some(value),
                // Output, executaion and program segments are not builtins.
                "output" => {}
                "execution" => {}
                "program" => {}
                _ => panic!("Unknown memory segment: {name}"),
            }
        }
        res
    }
}

[Stavbe]

Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @DavidLevitGurevich and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

struct represents only buildtins, no need to mention that

It is because the name of the component in the air-infra ends with"builtin" and in the future we would like to make sure it sync.(Same for the _opcdoe in CasmStatesByOpcodes)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 22 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

please remove that

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 145 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

better!

Much better! Thanks

---

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

Previously, ohad-starkware (Ohad) wrote…

it's a large object
its possible to reorder the code and avoid this

Done.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 217 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

why are these changed? are you running with dev mode on?
turn it off

I did s(ame as 245)

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 219 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

is this related?

Not relevant anymore

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 359 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

same and point at eol.

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r1 (raw file):

use cairo_vm::air_public_input::MemorySegmentAddresses;

// TODO(STAV): Understand if the adapter should pass builtins that won't be supported by stwo.

Done.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 300 at r1 (raw file):

        let input = small_cairo_input();

        // Check opcode components.

Done.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 360 at r1 (raw file):

        // Check builtins
        let builtins = input.builtins_segments;

Done.

[ohad-starkware]

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 44 at r2 (raw file):

                "output" => {}
                "execution" => {}
                "program" => {}

Suggestion:

                "output" | "execution" | "program" => {}

[ohad-starkware]

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 41 at r2 (raw file):

                "pedersen" => res.pedersen_builtin = Some(value),
                "poseidon" => res.poseidon_builtin = Some(value),
                // Output, execution and program segments are not builtins.

Suggestion:

// Not builtins.

[ohad-starkware]

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 29 at r2 (raw file):

        let mut res = BuiltinsSegments::default();
        for (name, value) in memory_segments.iter() {
            let value = (value.begin_addr, value.stop_ptr).into();

then remove the some from below

Suggestion:

 let value = Some((value.begin_addr, value.stop_ptr).into());

[ohad-starkware]

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 243 at r2 (raw file):

                stop_ptr: 1757348
            })
        );

non-blocking

Suggestion:

        assert_eq!(
            builtins_segments.range_check_bits_128_builtin,
            Some((1715768, 1757348).into()),
        );

[ohad-starkware]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

Previously, Stavbe wrote…

It is because the name of the component in the air-infra ends with"builtin" and in the future we would like to make sure it sync.(Same for the _opcdoe in CasmStatesByOpcodes)

im not sure it should appear there, @DavidLevitGurevich wdyt?

[Stavbe]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 29 at r2 (raw file):

Previously, ohad-starkware (Ohad) wrote…

then remove the some from below

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 41 at r2 (raw file):

                "pedersen" => res.pedersen_builtin = Some(value),
                "poseidon" => res.poseidon_builtin = Some(value),
                // Output, execution and program segments are not builtins.

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 44 at r2 (raw file):

                "output" => {}
                "execution" => {}
                "program" => {}

Done.

[ohad-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

---

stwo_cairo_prover/crates/adapted_prover/src/main.rs line 68 at r3 (raw file):

    let args = Args::try_parse_from(args)?;

    // dev_mode = true

remove

[Stavbe]

• unify adapter functionality #274 : 2 dependent PRs (#280 , #290 )
• support builtins in the adapter #269 👈 (View in Graphite)
• main

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

[Stavbe]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @ohad-starkware)

---

stwo_cairo_prover/crates/adapted_prover/src/main.rs line 68 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

remove

Done.

[shaharsamocha7]

Reviewed 1 of 4 files at r1, 1 of 4 files at r2, 1 of 2 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, and @Stavbe)

---

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

use cairo_vm::air_public_input::MemorySegmentAddresses;

// TODO(Stav): Understand if the adapter should pass builtins that won't be supported by stwo.

I think we can decide on that now and remove the TODO.
Any reason not to support all the builtins in this struct?

Code quote:

// TODO(Stav): Understand if the adapter should pass builtins that won't be supported by stwo.

---

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

// TODO(Stav): Understand if the adapter should pass builtins that won't be supported by stwo.
/// This struct holds the builtins used in a Cairo program.
/// Most of them are not implemented yet by Stwo.

Remove

Code quote:

/// Most of them are not implemented yet by Stwo.

---

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

/// Most of them are not implemented yet by Stwo.
#[derive(Debug, Default)]
pub struct BuiltinsSegments {

Does Default here set all the values as None?
I think the code that construct this struct should have a unit test

Code quote:

#[derive(Debug, Default)]
pub struct BuiltinsSegments {

---

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

impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(

Consider to implement the From trait, and then you'll be able to just use into()
non-blocking

Code quote:

    pub fn from_memory_segments(

---

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

    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,

Suggestion:

    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, MemorySegmentAddresses>,

---

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

            let value = Some((value.begin_addr, value.stop_ptr).into());
            match *name {
                "range_check" => res.range_check_bits_128_builtin = value,

Consider to use BuitinName enum from cairo_vm instead of hardcoded strings

Code quote:

            match *name {
                "range_check" => res.range_check_bits_128_builtin = value,

[Stavbe]

Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, and @shaharsamocha7)

---

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

Previously, shaharsamocha7 wrote…

I think we can decide on that now and remove the TODO.
Any reason not to support all the builtins in this struct?

If Stwo won't do anything with then why?

---

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

Previously, shaharsamocha7 wrote…

Remove

Done.

---

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

Previously, shaharsamocha7 wrote…

Does Default here set all the values as None?
I think the code that construct this struct should have a unit test

Yes, added.

---

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

    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,

Done.

[shaharsamocha7]

Reviewed 3 of 4 files at r5, all commit messages.
Reviewable status: 5 of 6 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, and @Stavbe)

---

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

Previously, Stavbe wrote…

If Stwo won't do anything with then why?

I don't understand, stwo has to handle all the builtins that are used by the program.
We can't just ignore them.

Another alternative is to assert that the "unsupported" builtins are not in use
but I don't think this is a good place for this.

---

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

Previously, Stavbe wrote…

Done.

I'm sorry but the previous impl was better because I don't like the
let builtins_segments = memory_segments.into();

Can you revert?

---

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

        assert_eq!(builtins_segments.add_mod_builtin, None);
        assert_eq!(builtins_segments.ec_op_builtin, None);
        assert_eq!(builtins_segments.ecdsa_builtin, Some((353, 353).into()));

Is this a correct behaviour?
I think that if the range is 0 we want to set it as None.
@yuvalsw WDYT?

Code quote:

assert_eq!(builtins_segments.ecdsa_builtin, Some((353, 353).into()));

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/air_pub.json line 425 at r5 (raw file):

  ],
  "dynamic_params": null
}

Add new line

[Stavbe]

Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, @shaharsamocha7, and @yuvalsw)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

im not sure it should appear there, @DavidLevitGurevich wdyt?

Since there are builtins here that won’t be implemented by the air-infra, it’s not relevant. Deleted.

---

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

Previously, shaharsamocha7 wrote…

I don't understand, stwo has to handle all the builtins that are used by the program.
We can't just ignore them.

Another alternative is to assert that the "unsupported" builtins are not in use
but I don't think this is a good place for this.

Done.

---

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

Previously, shaharsamocha7 wrote…

I'm sorry but the previous impl was better because I don't like the
let builtins_segments = memory_segments.into();

Can you revert?

Done.

---

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

Previously, shaharsamocha7 wrote…

Is this a correct behaviour?
I think that if the range is 0 we want to set it as None.
@yuvalsw WDYT?

Any idea what the difference is between builtins that don’t get a memory segment and those whose range is 0?

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/air_pub.json line 425 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Add new line

Done.

[ohad-starkware]

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

Previously, Stavbe wrote…

Any idea what the difference is between builtins that don’t get a memory segment and those whose range is 0?

+1 on the question

[yuvalsw]

Reviewed 1 of 4 files at r1, 1 of 2 files at r4, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Alon-Ti, @DavidLevitGurevich, @shaharsamocha7, and @Stavbe)

---

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

Previously, ohad-starkware (Ohad) wrote…

+1 on the question

@shaharsamocha7 - I think you are right. Both would probably work, but we do want to be as consistent as possible. Similar inconsistencies made us some trouble before.
@Stavbe - I think it may be a question of whether the builtin appears in the program but is unused (empty range) or simply does not appear in the program (no memory segment). @Alon-Ti or Yair Vaknin may know to confirm/correct.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 9 at r6 (raw file):

#[derive(Debug, Default)]
pub struct BuiltinsSegments {
    pub range_check_bits_128: Option<MemorySegmentAddresses>,

is this list ordered in some way? I'd expect it to either be alphabetical or some logical sense (I think there is a constant order of the builtins), in which case I'd expect e.g. add_mod and mul_mod to be next to each other.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 22 at r6 (raw file):

impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.

builtin_name -> MemorySegmentAddressess ?

Code quote:

map of memory MemorySegmentAddressess.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 26 at r6 (raw file):

        let mut res = BuiltinsSegments::default();
        for (name, value) in memory_segments.iter() {
            let value = Some((value.begin_addr, value.stop_ptr).into());

It looks like you are converting from a MemorySegmentAddresses to a tuple and back, am I missing something?

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 59 at r6 (raw file):

    fn test_builtins_segments() {
        let mut d = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
        d.push("src/input/test_builtins_segments/air_pub.json");

I see this is used in other places too, but:

1. I don't like the name d. dir?
2. This seems to need to be a util function. Can you please move it (in a separate PR - either before merging this one, or after, but with adding a TODO here).

Code quote:

        let mut d = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
        d.push("src/input/test_builtins_segments/air_pub.json");

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 59 at r6 (raw file):

    fn test_builtins_segments() {
        let mut d = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
        d.push("src/input/test_builtins_segments/air_pub.json");

Seems like this input doesn't use any builtin (all builtin segments are empty) - do we have a better input for this test which belongs to a program that did use builtins?

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 61 at r6 (raw file):

        d.push("src/input/test_builtins_segments/air_pub.json");
        let pub_data_string = std::fs::read_to_string(&d)
            .unwrap_or_else(|_| panic!("Unable to read file: {}", d.display()));

is the .display() needed?

Code quote:

d.display())

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 64 at r6 (raw file):

        let pub_data: PublicInput<'_> =
            sonic_rs::from_str(&pub_data_string).expect("Unable to parse JSON");
        let builtins_segments = BuiltinsSegments::from_memory_segments(&pub_data.memory_segments);

nit

Suggestion:

let builtin_segments = BuiltinSegments::from_mem

---

stwo_cairo_prover/crates/prover/src/input/mod.rs line 22 at r6 (raw file):

    pub public_mem_addresses: Vec<u32>,

    // Builtins.

I think this comment can be removed.

Code quote:

    // Builtins

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/air_pub.json line 0 at r6 (raw file):
Can you somehow doc how this was generated, what program it relates to, etc.?

[ohad-starkware]

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

Previously, yuvalsw wrote…

@shaharsamocha7 - I think you are right. Both would probably work, but we do want to be as consistent as possible. Similar inconsistencies made us some trouble before.
@Stavbe - I think it may be a question of whether the builtin appears in the program but is unused (empty range) or simply does not appear in the program (no memory segment). @Alon-Ti or Yair Vaknin may know to confirm/correct.

if a builtin is declared but never used (control flow) it gets 0 range
we need to convert that to None in the code above

[Stavbe]

Reviewable status: 1 of 8 files reviewed, 8 unresolved discussions (waiting on @Alon-Ti, @DavidLevitGurevich, @shaharsamocha7, and @yuvalsw)

---

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

Previously, ohad-starkware (Ohad) wrote…

if a builtin is declared but never used (control flow) it gets 0 range
we need to convert that to None in the code above

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 9 at r6 (raw file):

Previously, yuvalsw wrote…

is this list ordered in some way? I'd expect it to either be alphabetical or some logical sense (I think there is a constant order of the builtins), in which case I'd expect e.g. add_mod and mul_mod to be next to each other.

reordered according to the BuiltiName enum

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 22 at r6 (raw file):

Previously, yuvalsw wrote…

builtin_name -> MemorySegmentAddressess ?

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 26 at r6 (raw file):

Previously, yuvalsw wrote…

It looks like you are converting from a MemorySegmentAddresses to a tuple and back, am I missing something?

It’s either that or sending the hashmap by value. Since there’s a problem with borrowing and no Clone implementation exists for MemorySegmentAddresses.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 59 at r6 (raw file):

Previously, yuvalsw wrote…

I see this is used in other places too, but:

1. I don't like the name d. dir?
2. This seems to need to be a util function. Can you please move it (in a separate PR - either before merging this one, or after, but with adding a TODO here).

Sure

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 59 at r6 (raw file):

Previously, yuvalsw wrote…

Seems like this input doesn't use any builtin (all builtin segments are empty) - do we have a better input for this test which belongs to a program that did use builtins?

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 61 at r6 (raw file):

Previously, yuvalsw wrote…

is the .display() needed?

"std::path::PathBufcannot be formatted with the default formatter; call.display()` on it"

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/air_pub.json line at r6 (raw file):

Previously, yuvalsw wrote…

Can you somehow doc how this was generated, what program it relates to, etc.?

Added in the json

[ohad-starkware]

Reviewable status: 1 of 8 files reviewed, 8 unresolved discussions (waiting on @Alon-Ti, @DavidLevitGurevich, @shaharsamocha7, and @yuvalsw)

[shaharsamocha7]

Reviewed 7 of 7 files at r7, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @DavidLevitGurevich, @Stavbe, and @yuvalsw)

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/blake2s_felts_public_input.json line 1 at r7 (raw file):

{

Is there anything special about this program, or is it just an arbitrary test input?
Consider to rename the file to test_public_input.json

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/blake2s_felts_public_input.json line 2 at r7 (raw file):

{
  "_comment": "This file contains the AIR public input of the `blake2s_felts` Cairo program from `cairo-vm/cairo_programs`. It was generated by compiling this program in proof mode and running the VM with a specified path to output the AIR public inputs.",

Note that this comment was added manually, meaning if one will regenerate this input file, the comment will be gone.

Code quote:

{
  "_comment": "This file contains the AIR public input of the `blake2s_felts` Cairo program from `cairo-vm/cairo_programs`. It was generated by compiling this program in proof mode and running the VM with a specified path to output the AIR public inputs.",

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/blake2s_felts_public_input.json line 3036 at r7 (raw file):

  ],
  "dynamic_params": null
}

add new line

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 18 at r7 (raw file):

    pub range_check_bits_96: Option<MemorySegmentAddresses>,
    pub add_mod: Option<MemorySegmentAddresses>,
    pub mul_mod: Option<MemorySegmentAddresses>,

Reorder alphabetically

Code quote:

    pub range_check_bits_128: Option<MemorySegmentAddresses>,
    pub pedersen: Option<MemorySegmentAddresses>,
    pub ecdsa: Option<MemorySegmentAddresses>,
    pub keccak: Option<MemorySegmentAddresses>,
    pub bitwise: Option<MemorySegmentAddresses>,
    pub ec_op: Option<MemorySegmentAddresses>,
    pub poseidon: Option<MemorySegmentAddresses>,
    pub range_check_bits_96: Option<MemorySegmentAddresses>,
    pub add_mod: Option<MemorySegmentAddresses>,
    pub mul_mod: Option<MemorySegmentAddresses>,

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 31 at r7 (raw file):

                } else {
                    Some((value.begin_addr, value.stop_ptr).into())
                };

comment that it filters empty ranged builtins
Also, consider to assert that value.begin_addr <= value.stop_ptr and move to a get_segment function

Code quote:

                let segments = if value.begin_addr == value.stop_ptr {
                    None
                } else {
                    Some((value.begin_addr, value.stop_ptr).into())
                };

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 68 at r7 (raw file):

            .unwrap_or_else(|_| panic!("Unable to read file: {}", path.display()));
        let pub_data: PublicInput<'_> =
            sonic_rs::from_str(&pub_data_string).expect("Unable to parse JSON");

this should be moved to a test function. Is it your TODO?

Code quote:

        let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
        path.push("src/input/test_builtins_segments/blake2s_felts_public_input.json");
        let pub_data_string = std::fs::read_to_string(&path)
            .unwrap_or_else(|_| panic!("Unable to read file: {}", path.display()));
        let pub_data: PublicInput<'_> =
            sonic_rs::from_str(&pub_data_string).expect("Unable to parse JSON");

[Stavbe]

Reviewable status: 3 of 8 files reviewed, 12 unresolved discussions (waiting on @DavidLevitGurevich, @shaharsamocha7, and @yuvalsw)

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 18 at r7 (raw file):

Previously, shaharsamocha7 wrote…

Reorder alphabetically

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 31 at r7 (raw file):

Previously, shaharsamocha7 wrote…

comment that it filters empty ranged builtins
Also, consider to assert that value.begin_addr <= value.stop_ptr and move to a get_segment function

Done.

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 68 at r7 (raw file):

Previously, shaharsamocha7 wrote…

this should be moved to a test function. Is it your TODO?

yes

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/blake2s_felts_public_input.json line 1 at r7 (raw file):

Previously, shaharsamocha7 wrote…

Is there anything special about this program, or is it just an arbitrary test input?
Consider to rename the file to test_public_input.json

Arbitrary

---

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/blake2s_felts_public_input.json line 3036 at r7 (raw file):

Previously, shaharsamocha7 wrote…

add new line

Done.

[Stavbe]

Reviewable status: 3 of 8 files reviewed, 12 unresolved discussions (waiting on @DavidLevitGurevich, @shaharsamocha7, and @yuvalsw)

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 68 at r7 (raw file):

Previously, Stavbe wrote…

yes

I can’t move it out of this function due to a borrowing issue with pub_data_string:
"returns a value referencing data owned by the current function."

[yuvalsw]

Reviewed 2 of 7 files at r7, 1 of 3 files at r8, 3 of 3 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @DavidLevitGurevich, @shaharsamocha7, and @Stavbe)

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 18 at r7 (raw file):

Previously, Stavbe wrote…

Done.

I think I'd keep an order similar to the order in BuiltinName, WDYT?

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 68 at r7 (raw file):

Previously, Stavbe wrote…

I can’t move it out of this function due to a borrowing issue with pub_data_string:
"returns a value referencing data owned by the current function."

Not sure what the exact issue is (need to see), but you can clone if doesn't work otherwise.
(Again - TODO here is completely fine, then resolve this issue separately)

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 26 at r6 (raw file):

Previously, Stavbe wrote…

It’s either that or sending the hashmap by value. Since there’s a problem with borrowing and no Clone implementation exists for MemorySegmentAddresses.

Oh... Another PR to LC :) It should definitely have clone.
Resolving for now.

---

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 59 at r6 (raw file):

Previously, Stavbe wrote…

Sure

2. Continuing on Shahar's comment. Resolving this one.

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 54 at r10 (raw file):

                    assert!(
                        value.begin_addr < value.stop_ptr,
                        "Invalid segment addresses for builtin: {}",

1. Add the faulty addresses.
2. Is panic what we want in this case?

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 56 at r10 (raw file):

                        "Invalid segment addresses for builtin: {}",
                        name
                    );

suggestion

Suggestion:

                    assert!(
                        value.begin_addr < value.stop_ptr,
                        "Invalid segment addresses for builtin '{name}'"
                    );

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 83 at r10 (raw file):

            sonic_rs::from_str(&pub_data_string).expect("Unable to parse JSON");

        let builtins_segments = BuiltinSegments::from_memory_segments(&pub_data.memory_segments);

Suggestion:

builtin_segments = BuiltinSegments::from_mem

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

[Stavbe]

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @yuvalsw)

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 18 at r7 (raw file):

Previously, yuvalsw wrote…

I think I'd keep an order similar to the order in BuiltinName, WDYT?

It looks very random, and it’s different from the order in AirPrivateInputSerializable, so maybe alphabetical order is the right compromise.

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 68 at r7 (raw file):

Previously, yuvalsw wrote…

Not sure what the exact issue is (need to see), but you can clone if doesn't work otherwise.
(Again - TODO here is completely fine, then resolve this issue separately)

Clone doesn't solve the issue.
Can add a todo anyway

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 54 at r10 (raw file):

Previously, yuvalsw wrote…

1. Add the faulty addresses.
2. Is panic what we want in this case?

1. Done
2. Other suggestions? It shouldn't be a valid output of the vm.

[yuvalsw]

Reviewed 1 of 1 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 54 at r10 (raw file):

Previously, Stavbe wrote…

1. Done
2. Other suggestions? It shouldn't be a valid output of the vm.

2. It's probably fine

---

stwo_cairo_prover/crates/prover/src/input/builtin_segments.rs line 67 at r12 (raw file):

}

// TODO(Stav): move read json to a test function.

better move right above the test function itself.