Add poseidon round keys const table

[anatgstarkware]

This change is 

[DavidLevitGurevich]

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, and @shaharsamocha7)

---

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

pub fn table_id_to_col_id(table_id: &str, col_index: usize) -> String {
    format!("{}_{}", table_id, col_index)

if a table id ends with a list of _ separated numbers there might be a colision (for example range check vector). maybe better:

format!("{}_col{}", table_id, col_index)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/consts.rs line 704 at r1 (raw file):

    [[0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0]],
];

can everything below this line be in a separate file?

[anatgstarkware]

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

Previously, DavidLevitGurevich wrote…

if a table id ends with a list of _ separated numbers there might be a colision (for example range check vector). maybe better:

format!("{}_col{}", table_id, col_index)

I agree, @shaharsamocha7 WDYT?

[anatgstarkware]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/consts.rs line 704 at r1 (raw file):

Previously, DavidLevitGurevich wrote…

can everything below this line be in a separate file?

Done.

[shaharsamocha7]

Reviewed 4 of 5 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, and @DavidLevitGurevich)

---

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

Previously, anatgstarkware (anatg) wrote…

I agree, @shaharsamocha7 WDYT?

I don't understand why there is a collision
It is important to have unique names, maybe we should have a test for that.

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 31 at r2 (raw file):

#[derive(Debug)]
pub struct PoseidonRoundKeysPackedM31(pub [[PackedM31; FELT252PACKED27_N_WORDS * 3]; 4]);

I think this struct should store the values in the most direct way (non packed) maybe as it is in the consts file

It should also have a to_columns function that does the transpose of the values and generate the PoseidonRoundKeysColumn

Code quote:

pub struct PoseidonRoundKeysPackedM31(pub [[PackedM31; FELT252PACKED27_N_WORDS * 3]; 4]);

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 88 at r2 (raw file):

    pub keys: Rc<PoseidonRoundKeysPackedM31>,
    pub index: usize,
}

In my mind this struct should only store the values that are relevant for this column
If it make it easier for you, you can also store it as a non-packed form in the struct.

Suggestion:

#[derive(Debug)]
pub struct PoseidonRoundKeysColumn {
    pub values: Vec<PackedM31>,
    pub index: usize,
}

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 25 at r2 (raw file):

pub fn table_id_to_col_id(table_id: &str, col_index: usize) -> String {
    format!("{}_{}", table_id, col_index)
}

I don't think this function belong to this file

Code quote:

pub fn table_id_to_col_id(table_id: &str, col_index: usize) -> String {
    format!("{}_{}", table_id, col_index)
}

[anatgstarkware]

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

---

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

Previously, shaharsamocha7 wrote…

I don't understand why there is a collision
It is important to have unique names, maybe we should have a test for that.

There are no collisions between different column ids because the last number is always the column index in a table with more than one column. It does collide with the table ids in the infra, but it's not a problem, just confusing.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed.rs line 25 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I don't think this function belong to this file

Moved. do you like its new location?

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 31 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I think this struct should store the values in the most direct way (non packed) maybe as it is in the consts file

It should also have a to_columns function that does the transpose of the values and generate the PoseidonRoundKeysColumn

If we don't want to hold the entire packed table in advance, as we said before, then we don't need this struct at all.

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 88 at r2 (raw file):

Previously, shaharsamocha7 wrote…

In my mind this struct should only store the values that are relevant for this column
If it make it easier for you, you can also store it as a non-packed form in the struct.

Removed

[shaharsamocha7]

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 5 of 7 files reviewed, 10 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 31 at r2 (raw file):

Previously, anatgstarkware (anatg) wrote…

If we don't want to hold the entire packed table in advance, as we said before, then we don't need this struct at all.

Packed elements is only needed at a column level.
I thought you want the entire table in a struct for other purposes, so in my suggestion you can keep both.
Don't you need the table struct?

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 9 at r3 (raw file):

pub fn table_id_to_col_id(table_id: &str, col_index: usize) -> String {
    format!("{}_{}", table_id, col_index)
}

In my mind this function is not yet needed as a tool, and just could be inlined to the id() function directly

Code quote:

pub fn table_id_to_col_id(table_id: &str, col_index: usize) -> String {
    format!("{}_{}", table_id, col_index)
}

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 13 at r3 (raw file):

// Packs the values of column <col> and rows <vec_row * N_LANES..vec_row * N_LANES + N_LANES> into a
// PackedM31. Uses the <get_m31> function to get the value in a given row and column.
pub fn pack<const R: usize, F>(get_m31: F, vec_row: usize, col: usize) -> PackedM31

IIUC this function is not a trivial packing but also uses for padding.
Please make it explicit in the function name and docs

Code quote:

pub fn pack<const R: usize, F>(get_m31: F, vec_row: usize, col: usize) -> PackedM31

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 21 at r3 (raw file):

pub fn round_keys_m31(round: usize, col: usize) -> M31 {
    assert!(col < FELT252PACKED27_N_WORDS * 3);

Consider make a constant
is it N_STATE?

Code quote:

3

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 22 at r3 (raw file):

pub fn round_keys_m31(round: usize, col: usize) -> M31 {
    assert!(col < FELT252PACKED27_N_WORDS * 3);
    assert!(round < 35);

make a constant

Suggestion:

N_ROUNDS = 35

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 37 at r3 (raw file):

    pub fn packed_at(&self, vec_row: usize) -> PackedM31 {
        pack::<35, _>(round_keys_m31, vec_row, self.col)
    }

Current implementation evaluates this in each call of packed_at
Consider moving this logic to new and add Vec field

Code quote:

    pub fn packed_at(&self, vec_row: usize) -> PackedM31 {
        pack::<35, _>(round_keys_m31, vec_row, self.col)
    }

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 42 at r3 (raw file):

impl PreProcessedColumn for PoseidonRoundKeysColumn {
    fn log_size(&self) -> u32 {
        4

IIUC this should be 8, this counts total number of non-packed rows
Maybe we should test it? (gen_column_simd column size equiv log_size)

Code quote:

4

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 72 at r3 (raw file):

    #[test]
    fn test_packed_at_round_keys() {

Nit
Consider to change the test to check arbitrary index in a poseidon column (will make the test simpler)

Code quote:

    fn test_packed_at_round_keys() {

[ohad-starkware]

Reviewed 1 of 5 files at r3.
Reviewable status: 6 of 7 files reviewed, 11 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 9 at r3 (raw file):

Previously, shaharsamocha7 wrote…

In my mind this function is not yet needed as a tool, and just could be inlined to the id() function directly

+1

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 13 at r3 (raw file):

// Packs the values of column <col> and rows <vec_row * N_LANES..vec_row * N_LANES + N_LANES> into a
// PackedM31. Uses the <get_m31> function to get the value in a given row and column.
pub fn pack<const R: usize, F>(get_m31: F, vec_row: usize, col: usize) -> PackedM31

we have better tools for packing M31 structs.
also functional programming here is overkill IMO, this can be rewritten as a 3-liner as I suggested in #444 and inlined in the caller

[anatgstarkware]

Reviewable status: 3 of 7 files reviewed, 9 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, @ohad-starkware, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 9 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

+1

But I do use it in the air infra (soon in a pr)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 13 at r3 (raw file):

Previously, shaharsamocha7 wrote…

IIUC this function is not a trivial packing but also uses for padding.
Please make it explicit in the function name and docs

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 13 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

we have better tools for packing M31 structs.
also functional programming here is overkill IMO, this can be rewritten as a 3-liner as I suggested in #444 and inlined in the caller

We need it also for Blake and Pedersen
It's a 3-liner already :)
Can you please write here the exact code that you suggest I change to? I do want it to use a given Fn.

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 31 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Packed elements is only needed at a column level.
I thought you want the entire table in a struct for other purposes, so in my suggestion you can keep both.
Don't you need the table struct?

no :)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 21 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Consider make a constant
is it N_STATE?

Done

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 22 at r3 (raw file):

Previously, shaharsamocha7 wrote…

make a constant

DONE

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 37 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Current implementation evaluates this in each call of packed_at
Consider moving this logic to new and add Vec field

Done.

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 42 at r3 (raw file):

Previously, shaharsamocha7 wrote…

IIUC this should be 8, this counts total number of non-packed rows
Maybe we should test it? (gen_column_simd column size equiv log_size)

There are 35 rounds so the next power of two is 64, so 6, right?
My implementation of gen_column_simd uses log_size, so how can you test that?

[ohad-starkware]

Reviewable status: 3 of 7 files reviewed, 9 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 42 at r3 (raw file):

Previously, anatgstarkware (anatg) wrote…

There are 35 rounds so the next power of two is 64, so 6, right?
My implementation of gen_column_simd uses log_size, so how can you test that?

you can make it not use log_size,
write the CPU column, and convert to a Simd column (BaseColum::from_iter)
you also decouple it from packed it which is a positive change

[ohad-starkware]

Reviewable status: 3 of 7 files reviewed, 9 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 45 at r4 (raw file):

        let packed_keys = from_fn(|i| pack_and_pad::<N_ROUNDS, _>(round_keys_m31, i, col));
        Self { packed_keys, col }
    }

Suggestion:

    pub fn new(col: usize) -> Self {
        let packed_keys = from_fn(|vec_row| {
            PackedM31::from_array(from_fn(|i| {
                let row = vec_row * N_LANES + i;
                match row {
                    ..N_ROUNDS => round_keys_m31((vec_row * N_LANES) + i, col),
                    _ => round_keys_m31(0, col),
                }
            }))
        });
        Self { packed_keys, col }
    }

[ohad-starkware]

Reviewable status: 3 of 7 files reviewed, 9 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 45 at r4 (raw file):

        let packed_keys = from_fn(|i| pack_and_pad::<N_ROUNDS, _>(round_keys_m31, i, col));
        Self { packed_keys, col }
    }

this came out less good than than in 444, nevertheless I don't see the point in pack_and_pad, it was very hard to read

[anatgstarkware]

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

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 13 at r3 (raw file):

Previously, anatgstarkware (anatg) wrote…

We need it also for Blake and Pedersen
It's a 3-liner already :)
Can you please write here the exact code that you suggest I change to? I do want it to use a given Fn.

Changed it as you suggested (still not inline though)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 42 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

you can make it not use log_size,
write the CPU column, and convert to a Simd column (BaseColum::from_iter)
you also decouple it from packed it which is a positive change

@ohad-starkware, IIUC, I need the from_simd because I have the packed array, but I can take it as is, right? Is it ok now?

@shaharsamocha7, regarding the LOG_N_ROWS, I changed its definition so it's calculated from N_ROUNDS. Is that better?

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 45 at r4 (raw file):

Previously, ohad-starkware (Ohad) wrote…

this came out less good than than in 444, nevertheless I don't see the point in pack_and_pad, it was very hard to read

I need pack_and_pad for Pedersen and Blake as well. I don't want to copy-paste this code
Changed its implementation to yours. I hope it's clearer now.

[shaharsamocha7]

Reviewed 1 of 3 files at r5.
Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 9 at r3 (raw file):

Previously, anatgstarkware (anatg) wrote…

But I do use it in the air infra (soon in a pr)

to generate the same id for the constraints?
Idk, it is just seems unrelated here..

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 42 at r3 (raw file):

Previously, anatgstarkware (anatg) wrote…

@ohad-starkware, IIUC, I need the from_simd because I have the packed array, but I can take it as is, right? Is it ok now?

@shaharsamocha7, regarding the LOG_N_ROWS, I changed its definition so it's calculated from N_ROUNDS. Is that better?

you are right it is 6, I'm not good at math.
Re the question - I think it is good enough, alternatively you could also take it from the packed_keys length

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 21 at r5 (raw file):

        let row = (vec_row * N_LANES) + i;
        match row {
            1..35 => get_m31(row, col),

this 35 is too specific in this function, is it not N_ROUNDS?

Code quote:

1..35 => get_m31(row, col)

[anatgstarkware]

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 21 at r5 (raw file):

Previously, shaharsamocha7 wrote…

this 35 is too specific in this function, is it not N_ROUNDS?

yes, my mistake, fixed (cannot use patterns with const, so had to change a bit)

[shaharsamocha7]

Reviewed 1 of 2 files at r6.
Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 21 at r5 (raw file):

Previously, anatgstarkware (anatg) wrote…

yes, my mistake, fixed (cannot use patterns with const, so had to change a bit)

it is better but still looks specific for columns that has round notation.
Are you gonna have more columns that use this code?

[anatgstarkware]

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 21 at r5 (raw file):

Previously, shaharsamocha7 wrote…

it is better but still looks specific for columns that has round notation.
Are you gonna have more columns that use this code?

I don't think so, but now there's no round notation

[anatgstarkware]

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

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 9 at r3 (raw file):

Previously, shaharsamocha7 wrote…

to generate the same id for the constraints?
Idk, it is just seems unrelated here..

yes. do you have a better idea?

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 42 at r3 (raw file):

Previously, shaharsamocha7 wrote…

you are right it is 6, I'm not good at math.
Re the question - I think it is good enough, alternatively you could also take it from the packed_keys length

I compute N_PACKED_ROWS from LOG_N_ROWS

[shaharsamocha7]

Reviewed 1 of 4 files at r4, 1 of 3 files at r9, all commit messages.
Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @ohad-starkware)

[anatgstarkware]

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, @ohad-starkware, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 23 at r9 (raw file):

Previously, ohad-starkware (Ohad) wrote…

inline please

Done.

[ohad-starkware]

Reviewed 1 of 3 files at r9.
Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

[anatgstarkware]

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

Previously, anatgstarkware (anatg) wrote…

There are no collisions between different column ids because the last number is always the column index in a table with more than one column. It does collide with the table ids in the infra, but it's not a problem, just confusing.

@Gali-StarkWare, did you already add range check vector IDs of the form "range_check_4_4_4_4_column_2"?

[shaharsamocha7]

Reviewed 1 of 3 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, and @Gali-StarkWare)

---

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

Previously, anatgstarkware (anatg) wrote…

@Gali-StarkWare, did you already add range check vector IDs of the form "range_check_4_4_4_4_column_2"?

She has a pr
https://reviewable.io/reviews/starkware-libs/stwo-cairo/451#-

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 12 at r10 (raw file):

    (0..n)
        .map(|i| if i < padding_offset { i } else { 0 })
        .map(|i| get_m31(i, col))

Why are there two maps? can you merge them to one?

Code quote:

        .map(|i| if i < padding_offset { i } else { 0 })
        .map(|i| get_m31(i, col))

[Gali-StarkWare]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, and @DavidLevitGurevich)

---

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

Previously, shaharsamocha7 wrote…

She has a pr
https://reviewable.io/reviews/starkware-libs/stwo-cairo/451#-

It's not a preprocessed column yet, but I agree the we will need some sort of separation for that (e.g. "column" like you suggested)

[anatgstarkware]

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

Previously, Gali-StarkWare wrote…

It's not a preprocessed column yet, but I agree the we will need some sort of separation for that (e.g. "column" like you suggested)

The new id format is already in the code, not in a pr (see below in this file)

[anatgstarkware]

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 12 at r10 (raw file):

Previously, shaharsamocha7 wrote…

Why are there two maps? can you merge them to one?

I could, that's how it was before, but I understood that @ohad-starkware thinks this is more readable, @ohad-starkware?

[ohad-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 12 at r10 (raw file):

Previously, anatgstarkware (anatg) wrote…

I could, that's how it was before, but I understood that @ohad-starkware thinks this is more readable, @ohad-starkware?

style, u choose
I like the 2 maps better, one is a round-number projection, the other is the value

[ohad-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

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

Previously, anatgstarkware (anatg) wrote…

The new id format is already in the code, not in a pr (see below in this file)

it is not etched in stone

[shaharsamocha7]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anatgstarkware, @dancarmoz, @DavidLevitGurevich, and @ohad-starkware)

---

stwo_cairo_prover/crates/prover/src/cairo_air/preprocessed_utils.rs line 12 at r10 (raw file):

Previously, ohad-starkware (Ohad) wrote…

style, u choose
I like the 2 maps better, one is a round-number projection, the other is the value

My comment is non-blocking, you choose

[DavidLevitGurevich]

Reviewed 1 of 5 files at r2, 1 of 4 files at r4, 1 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dancarmoz)

---

a discussion (no related file):
can this PR be splitted?

[DavidLevitGurevich]

Reviewed 1 of 5 files at r2, 1 of 5 files at r3, 3 of 4 files at r10.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dancarmoz)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 29 at r10 (raw file):

    let felt252_index = col / FELT252PACKED27_N_WORDS;
    let felt_index = col % FELT252PACKED27_N_WORDS;

felt_index -> m31_index

[anatgstarkware]

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @dancarmoz, @DavidLevitGurevich, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 29 at r10 (raw file):

Previously, DavidLevitGurevich wrote…

felt_index -> m31_index

Done.

[DavidLevitGurevich]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dancarmoz)

[DavidLevitGurevich]

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dancarmoz)

[Gali-StarkWare]

Reviewed 2 of 8 files at r13, all commit messages.
Reviewable status: 4 of 10 files reviewed, 1 unresolved discussion (waiting on @dancarmoz, @DavidLevitGurevich, @ohad-starkware, and @shaharsamocha7)

---

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 29 at r14 (raw file):

#[derive(Debug)]
pub struct PoseidonRoundKeysColumn {

Why not PoseidonRoundKeys? This is how we get it from you (and also the other column's structs' names are without it)

[anatgstarkware]

stwo_cairo_prover/crates/prover/src/cairo_air/poseidon/const_columns.rs line 29 at r14 (raw file):

Previously, Gali-StarkWare wrote…

Why not PoseidonRoundKeys? This is how we get it from you (and also the other column's structs' names are without it)

I think this is a more accurate name, as this is a single column.
Currently, the infra doesn't pass the name of the PreProcessedColumn class, see RangeCheck for example.
Also, note that the class name is not enough.
It was requested that we pass the column_id from now on (in a different pr in stwo-air-infra)