fill memory segments of builtins to the next power of 2 instances

[ohad-nir-starkware]

This change is 

[yuvalsw]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-nir-starkware, and @shaharsamocha7)

---

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

    }

    pub fn get(&self, addr: u32) -> MemoryValue {

in set, addr is u64m what should it be?

---

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

        StateTransitions::from_iter(trace_iter, &mut memory, dev_mode);
    let mut builtins_segments = BuiltinSegments::from_memory_segments(memory_segments);
    memory = builtins_segments.fill_builtin_segment(memory, BuiltinName::range_check);

add a TODO to fill other builtins too?

---

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

            assert!(original_segment_length % cells_per_instance == 0);
            let original_num_instances = original_segment_length / cells_per_instance;
            assert!(original_num_instances * 2 > num_instances); // the next power of 2.

also assert original_num_instances <= num_instances ?

Code quote:

assert!(original_num_instances * 2 > num_instances); // the next power of 2.

---

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

            assert!(original_num_instances * 2 > num_instances); // the next power of 2.

            assert!(segment.stop_ptr - 1 <= memory.address_to_id.len());

why -1 and <= instead of < ?

Code quote:

ment.stop_ptr - 1 <= memory.a

---

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

                    );
                }
            }

this logic is very similar to the actual logic, so I am not sure this test achieves its goal. Consider testing the actual golden data

Code quote:

            for instance in original_num_instances..num_instances {
                for j in 0..cells_per_instance {
                    let address = segment.begin_addr + instance * cells_per_instance + j;
                    assert!(
                        memory.address_to_id[address]
                            == memory.address_to_id[segment.begin_addr + j]
                    );
                }
            }

---

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

    }

    /// Pads a builtin segment with copies of it's first instance.

Suggestion:

pies of its first i

---

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

    }

    /// Pads a builtin segment with copies of it's first instance.

please doc why it's safe (that at least one instance exists).

Code quote:

es of it's first instance.

---

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

                address_to_fill += 1;
            }
        }

consider exporting to a util function that copies a block of size N from memory to somewhere else in the memory.

Code quote:

        for _ in num_instances..nearest_power_of_two {
            for j in 0..cells_per_instance {
                memory.copy_value((begin_addr + j) as u32, address_to_fill as u32);
                address_to_fill += 1;
            }
        }

---

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

impl BuiltinSegments {
    pub fn add_segment(

not your code, but consider renaming to set_segment. IIUC, this overwrites the segment if already set, thus "add" makes less sense. You also use it here to overwrite, so it's not just a theoretical thing.
(of course, if it appears a lot, this, if done, should be done in a separate PR)

---

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

    }

    fn get_segment(&mut self, builtin_name: BuiltinName) -> &Option<MemorySegmentAddresses> {

does it need to return ref?

Code quote:

> &Option<M

---

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

            BuiltinName::range_check96 => 1,
            BuiltinName::add_mod => 7,
            BuiltinName::mul_mod => 7,

So these are indeed constant?

Code quote:

            BuiltinName::add_mod => 7,
            BuiltinName::mul_mod => 7,

[ohad-nir-starkware]

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

---

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

Previously, yuvalsw wrote…

not your code, but consider renaming to set_segment. IIUC, this overwrites the segment if already set, thus "add" makes less sense. You also use it here to overwrite, so it's not just a theoretical thing.
(of course, if it appears a lot, this, if done, should be done in a separate PR)

Done.

---

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

Previously, yuvalsw wrote…

So these are indeed constant?

add_mod and mul_mod require special attention and likely a separate PR.
returning 7 (4 for p, 1 for n, 1 for values_ptr, 1 for offsets_ptr) seemed like a better option than returning 0.
any other intermediate solutions?

---

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

Previously, yuvalsw wrote…

please doc why it's safe (that at least one instance exists).

Done.

---

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

Previously, yuvalsw wrote…

consider exporting to a util function that copies a block of size N from memory to somewhere else in the memory.

Done.

---

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

Previously, yuvalsw wrote…

in set, addr is u64m what should it be?

get is a method of Memory and set is a method of MemoryBuilder, they take u32 and u64 respectively in the origin/main code, so untill one of those function changes, copy_value will "disagree" with one of them.

---

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

Previously, yuvalsw wrote…

add a TODO to fill other builtins too?

Done.

---

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

Previously, yuvalsw wrote…

also assert original_num_instances <= num_instances ?

Done.

---

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

Previously, yuvalsw wrote…

why -1 and <= instead of < ?

Done.

---

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

    }

    /// Pads a builtin segment with copies of it's first instance.

Done.

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 16 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-nir-starkware, and @yuvalsw)

---

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

    /// Pads a builtin segment with copies of its first instance if that segment isn't None, in
    /// which case at least one instance is guaranteed to exist.
    /// The segment is padded to the nearest power of two number of instances.

I would write what this function should do rather than how it do that
IMO "copies if its first instance" is an implementation detail rather than purpose.

Code quote:

    /// Pads a builtin segment with copies of its first instance if that segment isn't None, in
    /// which case at least one instance is guaranteed to exist.
    /// The segment is padded to the nearest power of two number of instances.

---

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

    /// Pads a builtin segment with copies of its first instance if that segment isn't None, in
    /// which case at least one instance is guaranteed to exist.
    /// The segment is padded to the nearest power of two number of instances.

please rename this everywhere

Suggestion:

next power of two

---

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

        else {
            return memory;
        };

Can you explain?
Why do you return the memory for everything else?

Code quote:

        else {
            return memory;
        };

---

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

        assert!(initial_length % cells_per_instance == 0);
        let num_instances = initial_length / cells_per_instance;
        let nearest_power_of_two = num_instances.next_power_of_two();

rename

Suggestion:

let next_power_of_two = num_instances.next_power_of_two();

---

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

    /// Verifies that the builtin segment is padded with copies of the first instance to the next
    /// power of 2 instances.
    pub fn verify_segment_is_padded(

should it be pub?

Code quote:

   pub fn verify_segment_is_padded(

[ohad-nir-starkware]

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

---

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

Previously, yuvalsw wrote…

does it need to return ref?

MemorySegmentAddresses currently does not implement clone, thus in order to avoid ownership issues I use references.
That struct is defined in the cairo-vm repo, which I currently cannot edit.
Added a TODO to use clone instead of references once clone is implemented.

---

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

Previously, shaharsamocha7 wrote…

please rename this everywhere

Done.

---

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

Previously, shaharsamocha7 wrote…

I would write what this function should do rather than how it do that
IMO "copies if its first instance" is an implementation detail rather than purpose.

Done.

---

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

Previously, shaharsamocha7 wrote…

Can you explain?
Why do you return the memory for everything else?

if self.get_segment(builtin_name) is not &Some(MemorySegmentAddresses {begin_addr,stop_ptr})
then either builtin_name is not a builtin or it is a builtin with an empty segment.
either way the function should just return memory as is.
If a builtin isn't used then self.get_segment(builtin_name) returns &None, in which case I don't want to panic, I want to leave the memory as is and move on to the next builtin.

---

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

Previously, shaharsamocha7 wrote…

rename

Done.

---

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

Previously, yuvalsw wrote…

this logic is very similar to the actual logic, so I am not sure this test achieves its goal. Consider testing the actual golden data

Done.

---

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

Previously, shaharsamocha7 wrote…

should it be pub?

Done.

[shaharsamocha7]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: 2 of 3 files reviewed, 16 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-nir-starkware, and @yuvalsw)

---

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

    let mut builtins_segments = BuiltinSegments::from_memory_segments(memory_segments);
    // TODO (ohadn): fill in the memory segments of the rest of the builtins.
    // Different logic might be required for add_mod and mul_mod.

This sentence is another TODO that should be in the fill_builtin_segment function

Code quote:

// Different logic might be required for add_mod and mul_mod.

---

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

    /// Pads a builtin segment with additional valid instances.
    /// The segment is padded to the next power of 2 number of instances.
    pub fn fill_builtin_segment(

IMHO this function shouldn't be placed here.
As I understand it, we pad each builtin segment at a time (which has a side effect on the memory)
Consider to have a function that updates a single builtin, which get a mutable reference to the memory.

non-blocking

Code quote:

    pub fn fill_builtin_segment(

---

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

    #[test]
    fn test_fill_builtin_segment() {

IMO this test is better then the previous test function
It is still a bit messy so I would consider to "clean" it.
e.g. define an initiliaze_memory test function and reduce the code at the beggining of the test.

Code quote:

fn test_fill_builtin_segment() {

---

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

        else {
            panic!("Expected nonempty segment for builtin '{}'.", builtin_name);
        };

This shouldn't be here.
Why do you need to check the test setup?

Code quote:

        else {
            panic!("Expected nonempty segment for builtin '{}'.", builtin_name);
        };

---

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

        // Assert that new_num_instances is the next power of 2 after num_instances.
        assert!(num_instances * 2 > new_num_instances);
        assert!(num_instances <= new_num_instances);

assert_eq!(new_num_instances = num_instances.next_power_of_2());

If num_instances is hard_coded (71) you know that the next power is 128, do we need all this code to check that?

Code quote:

        // Assert that new_num_instances is a power of 2.
        assert!((new_num_instances & (new_num_instances - 1)) == 0);
        // Assert that new_num_instances is the next power of 2 after num_instances.
        assert!(num_instances * 2 > new_num_instances);
        assert!(num_instances <= new_num_instances);

[DavidLevitGurevich]

Reviewed all commit messages.
Reviewable status: 2 of 3 files reviewed, 16 unresolved discussions (waiting on @ArielElp, @ohad-nir-starkware, and @yuvalsw)

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

MemorySegmentAddresses currently does not implement clone, thus in order to avoid ownership issues I use references.
That struct is defined in the cairo-vm repo, which I currently cannot edit.
Added a TODO to use clone instead of references once clone is implemented.

you can send them a PR, talk to @ArielElp if you have a specific code suggestion

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

add_mod and mul_mod require special attention and likely a separate PR.
returning 7 (4 for p, 1 for n, 1 for values_ptr, 1 for offsets_ptr) seemed like a better option than returning 0.
any other intermediate solutions?

I think 7 is the correct number in this case. correct me if I'm wrong, you are counting the size of the padding, which is indeed 7 per instance

[DavidLevitGurevich]

Reviewable status: 2 of 3 files reviewed, 17 unresolved discussions (waiting on @ArielElp, @ohad-nir-starkware, and @yuvalsw)

---

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

impl BuiltinSegments {
    /// Adds a segment to the builtin segments.

fix the documentation accordingly

[yuvalsw]

Reviewed 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-nir-starkware, and @shaharsamocha7)

---

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

Previously, DavidLevitGurevich wrote…

I think 7 is the correct number in this case. correct me if I'm wrong, you are counting the size of the padding, which is indeed 7 per instance

If the size of the padding is constant (and 7), then great. If it's not and requires special attention, 7 here is ok if it's the closest answer, but a TODO is a must (and fixing it soon in a following PR).

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

Done.

I meant, please mention that there is at least one existing instance, and thus it's safe to access it and copy it.

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

Done.

I don't see, did you push?

---

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

Previously, shaharsamocha7 wrote…

IMO this test is better then the previous test function
It is still a bit messy so I would consider to "clean" it.
e.g. define an initiliaze_memory test function and reduce the code at the beggining of the test.

I am ok with this, as long as the numbers appear in the test (that is, passed as params (with proper naming) to this setup functions). You don't want to need to jump to the impl of the setup function to understand where these numbers came from.

---

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

Previously, shaharsamocha7 wrote…

assert_eq!(new_num_instances = num_instances.next_power_of_2());

If num_instances is hard_coded (71) you know that the next power is 128, do we need all this code to check that?

Was about to comment exactly the same. I think it's more than "do we really need all this code". It's safer to test for 128 explicitly. Less logic in the test (that, as said before, could have bugs of its own).

I'd just change to
assert_eq(new_num_instances, 128);

---

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

            memory: &Memory,
            original_stop_ptr: usize,
            golden_instance: Vec<u32>,

consider
(then you don't need the into in the call)

Suggestion:

golden_instance: &[u32],

---

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

    fn test_fill_builtin_segment() {
        let mut dummy_builtin_segments = BuiltinSegments::default();
        let mut dummy_memory_builder = MemoryBuilder::new(MemoryConfig::default());

should MemoryBuilder derive/impl Default? (nonblocking, also can be done in a separate PR)

Code quote:

MemoryBuilder::new(MemoryConfig::default());

---

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

        let begin_addr = 23581;
        let stop_ptr = begin_addr + cells_per_instance * num_instances;
        dummy_builtin_segments.set_segment(

IMO "dummy" is not necessary. It's a test, it's clear enough it's dummy, and it's easier to read without this word everywhere. Nonblocking ofcourse and up to you.

---

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

        );
        for (i, &value) in instance_example.iter().enumerate() {
            dummy_memory_builder.set((begin_addr + i) as u64, MemoryValue::Small(value));

I suggest to also test padding with instances that contain large values. WDYT?

Code quote:

MemoryValue::Small(value)

---

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

            panic!("Expected nonempty segment for builtin '{}'.", builtin_name);
        };
        assert!(new_begin_addr == begin_addr);

general comment: you can use assert_eq, assert_lt etc.

---

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

                        == dummy_memory.address_to_id[begin_addr + j]
                );
            }

similar to another comment I previously wrote - this is better be exported to a util function asserting blocks of memory.

Code quote:

            for j in 0..cells_per_instance {
                let address = begin_addr + instance * cells_per_instance + j;
                assert!(
                    dummy_memory.address_to_id[address]
                        == dummy_memory.address_to_id[begin_addr + j]
                );
            }

[ohad-nir-starkware]

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

---

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

Previously, yuvalsw wrote…

If the size of the padding is constant (and 7), then great. If it's not and requires special attention, 7 here is ok if it's the closest answer, but a TODO is a must (and fixing it soon in a following PR).

@DavidLevitGurevich yes, I wasn't certain at first, but I quite confident of that now.

---

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

Previously, yuvalsw wrote…

I don't see, did you push?

I implemented the function copy_value inside MemoryBuilder, it copies one value at a time.
seems thats not what you had in mind.

---

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

Previously, DavidLevitGurevich wrote…

fix the documentation accordingly

Done.

---

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

Previously, yuvalsw wrote…

I am ok with this, as long as the numbers appear in the test (that is, passed as params (with proper naming) to this setup functions). You don't want to need to jump to the impl of the setup function to understand where these numbers came from.

Done.

---

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

Previously, yuvalsw wrote…

I suggest to also test padding with instances that contain large values. WDYT?

Done.

---

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

Previously, shaharsamocha7 wrote…

This shouldn't be here.
Why do you need to check the test setup?

Done.

---

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

Previously, yuvalsw wrote…

Was about to comment exactly the same. I think it's more than "do we really need all this code". It's safer to test for 128 explicitly. Less logic in the test (that, as said before, could have bugs of its own).

I'd just change to
assert_eq(new_num_instances, 128);

Done.

---

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

Previously, shaharsamocha7 wrote…

This sentence is another TODO that should be in the fill_builtin_segment function

Done.

---

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

Previously, yuvalsw wrote…

consider
(then you don't need the into in the call)

Done.

[shaharsamocha7]

Reviewable status: 0 of 3 files reviewed, 9 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-nir-starkware, and @yuvalsw)

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

Done.

I think the correct place for this "address the cases of add_mod and mul_mod" TODO is inside the function
non-blocking

[yuvalsw]

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

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

I implemented the function copy_value inside MemoryBuilder, it copies one value at a time.
seems thats not what you had in mind.

copy_value is good, In addition I meant we may have a function for copying a block (that is this inner for loop can be translate to something like copy_memory_block(address_to_fill, begin_addr, cells_per_instance) (as in target, source, n)

[ohad-nir-starkware]

@shaharsamocha7 should I delete verify_segment_is_padded?

On another note, I have changed the way fill_builtin_segment pads from using the first of the original instances to using the last.
The reason is because it generalises better to add_mod and mul_mod, as the last original instance of a mod_builtin should have n=1 (can't guarantee yet that it is always the case for cairo0, but it is something we desire from the cairoVM output).

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

---

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

Previously, yuvalsw wrote…

I meant, please mention that there is at least one existing instance, and thus it's safe to access it and copy it.

Done.

---

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

Previously, yuvalsw wrote…

copy_value is good, In addition I meant we may have a function for copying a block (that is this inner for loop can be translate to something like copy_memory_block(address_to_fill, begin_addr, cells_per_instance) (as in target, source, n)

Done.

---

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

Previously, shaharsamocha7 wrote…

IMHO this function shouldn't be placed here.
As I understand it, we pad each builtin segment at a time (which has a side effect on the memory)
Consider to have a function that updates a single builtin, which get a mutable reference to the memory.

non-blocking

added a TODO because currently we don't have a more preferable location.

---

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

Previously, yuvalsw wrote…

similar to another comment I previously wrote - this is better be exported to a util function asserting blocks of memory.

Done.

[DavidLevitGurevich]

Reviewed 1 of 3 files at r3, 1 of 1 files at r9, all commit messages.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @ohad-nir-starkware, @shaharsamocha7, and @yuvalsw)

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

added a TODO because currently we don't have a more preferable location.

maybe it should be a function of the memory itself?

[DavidLevitGurevich]

Reviewed 3 of 3 files at r11.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ohad-nir-starkware, @shaharsamocha7, and @yuvalsw)

---

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

Previously, DavidLevitGurevich wrote…

maybe it should be a function of the memory itself?

I'm unblocking because it is unclear that the memory is the right place.

---

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

        use crate::input::memory::{EncodedMemoryValueId, Memory};

        /// Verifies that the builtin segment is padded with copies of the first instance to the

first -> golden instance

---

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

        let mut instance_to_fill_start = stop_ptr as u32;
        for _ in num_instances..next_power_of_two {
            memory.copy_segment(

is there a way in this context to assert that this memory cell is empty? @shaharsamocha7

---

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

            }),
        );
        memory

if the memory is mutable, why do you return it?

[yuvalsw]

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

---

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

        for _ in num_instances..next_power_of_two {
            memory.copy_segment(
                (stop_ptr - cells_per_instance) as u32,

please export this to a var (consider golden_instance_start or similar)

Code quote:

(stop_ptr - cells_per_instance) as u32

---

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

                (stop_ptr - cells_per_instance) as u32,
                instance_to_fill_start,
                cells_per_instance as u32,

if this is converted to u32 multiple times, consider doing the conversion in the definition of the var.

Code quote:

cells_per_instance as u32

[ohad-nir-starkware]

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

---

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

Previously, yuvalsw wrote…

please export this to a var (consider golden_instance_start or similar)

Done.

---

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

Previously, yuvalsw wrote…

if this is converted to u32 multiple times, consider doing the conversion in the definition of the var.

it interacts with some usize variables so some conversions will be needed either way.
it seems that the root cause of there being so many usize variables is MemorySegmentAddresses (preexisting struct),
so maybe I should change it's fields to u32. WDYT?

---

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

Previously, DavidLevitGurevich wrote…

if the memory is mutable, why do you return it?

Done.

---

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

Previously, DavidLevitGurevich wrote…

first -> golden instance

Done.

[yuvalsw]

Reviewed 2 of 3 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-nir-starkware, and @shaharsamocha7)

---

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

Previously, ohad-nir-starkware (Ohad Nir) wrote…

it interacts with some usize variables so some conversions will be needed either way.
it seems that the root cause of there being so many usize variables is MemorySegmentAddresses (preexisting struct),
so maybe I should change it's fields to u32. WDYT?

We probably need a refactor for this typing mess, which includes structs in the LC repo. Let it be for this PR and let's separately discuss how to make it better in separate PRs

[ohad-nir-starkware]

update: I have verified that both the python VM and the rust VM have security checks within their modulo_builtin runners which include an assert that the last instance of the builtin has n=batchsize=1.
This means that padding the builtin segment with copies of its last (VM provided) instance should work for all builtins, including add_mod and mul_mod without additional processing.

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