feat: added private versioned constants

[amosStarkware]

This change is 

[amosStarkware]

+reviewer:@dorimedini-starkware

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[dorimedini-starkware]

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

---

crates/blockifier/src/versioned_constants.rs line 207 at r1 (raw file):

    /// Private versioned constants are used if they are provided, otherwise the latest versioned
    /// constants are used. `validate_max_n_steps` & `max_recursion_depth` override both.

Suggestion:

    /// Private versioned constants are used if they are provided, otherwise the latest versioned
    /// constants are used. `validate_max_n_steps` & `max_recursion_depth` override both.
    // TODO(Amos, 1/8/2024): remove the explicit max_n_steps, they should be part of the general override.

---

crates/blockifier/src/versioned_constants.rs line 209 at r1 (raw file):

    /// constants are used. `validate_max_n_steps` & `max_recursion_depth` override both.
    pub fn get_versioned_constants(
        private_versioned_constants: Option<String>,

please move all deserialization logic to the native blockifier side (you can add a utility function for this)

Suggestion:

versioned_constants_override: Option<VersionedConstants>,

---

crates/blockifier/src/versioned_constants.rs line 231 at r1 (raw file):

                }
            }
        }

reduce some duplication

Suggestion:

        let base_versioned_constants = match private_versioned_constants {
            Some(private_versioned_constants) => {
                log::debug!(
                    "Using provided private versioned constants (with additional overrides)."
                );
                serde_json::from_str(&private_versioned_constants)
                    .expect("Versioned constants JSON file is malformed.")
            }
            None => {
                log::debug!("Using latest versioned constants (with additional overrides).");
                Self::latest_constants().clone()
            };
            Self {
                validate_max_n_steps,
                max_recursion_depth,
                ..base_versioned_constants
            }
        }

---

crates/native_blockifier/src/py_block_executor.rs line 115 at r1 (raw file):

            validate_max_n_steps,
            max_recursion_depth,
        );

1. Define a VersionedConstantsOverrides (without the Py) struct
2. implement From<PyVersionedConstantsOverrides> for VersionedConstantsOverrides
3. use it instead of the three params in get_versioned_constants

Suggestion:

        let versioned_constants = VersionedConstants::get_versioned_constants(
            versioned_constants_overrides.into
        );

---

crates/native_blockifier/src/py_objects.rs line 65 at r1 (raw file):

            Err(PyValueError::new_err("Failed to parse private_versioned_constants."))
        }
    }

is this ever used in business logic...? I don't see any usage here

Code quote:

    #[staticmethod]
    pub fn validate_private_versioned_constants_str(
        private_versioned_constants: &str,
    ) -> PyResult<()> {
        if serde_json::from_str::<VersionedConstants>(private_versioned_constants).is_ok() {
            Ok(())
        } else {
            Err(PyValueError::new_err("Failed to parse private_versioned_constants."))
        }
    }

---

crates/native_blockifier/src/py_validator.rs line 48 at r1 (raw file):

            validate_max_n_steps,
            max_recursion_depth,
        );

see above

Code quote:

        let PyVersionedConstantsOverrides {
            validate_max_n_steps,
            max_recursion_depth,
            private_versioned_constants,
        } = versioned_constants_overrides;
        let versioned_constants = VersionedConstants::get_versioned_constants(
            private_versioned_constants,
            validate_max_n_steps,
            max_recursion_depth,
        );

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 24.13793% with 44 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@35f5081). Learn more about missing BASE report.

Files	Patch %	Lines
crates/native_blockifier/src/py_objects.rs	0.00%	33 Missing ⚠️
crates/blockifier/src/versioned_constants.rs	77.77%	4 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs	0.00%	3 Missing ⚠️
crates/native_blockifier/src/py_validator.rs	0.00%	3 Missing ⚠️
crates/native_blockifier/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1992   +/-   ##
=======================================
  Coverage        ?   78.47%           
=======================================
  Files           ?       62           
  Lines           ?     8967           
  Branches        ?     8967           
=======================================
  Hits            ?     7037           
  Misses          ?     1483           
  Partials        ?      447           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[amosStarkware]

Reviewable status: 1 of 7 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/versioned_constants.rs line 207 at r1 (raw file):

    /// Private versioned constants are used if they are provided, otherwise the latest versioned
    /// constants are used. `validate_max_n_steps` & `max_recursion_depth` override both.

how would you implement this?
i.e., how would an override of only validate_max_n_steps look like?

---

crates/blockifier/src/versioned_constants.rs line 209 at r1 (raw file):

Previously, dorimedini-starkware wrote…

please move all deserialization logic to the native blockifier side (you can add a utility function for this)

Done.

---

crates/blockifier/src/versioned_constants.rs line 231 at r1 (raw file):

Previously, dorimedini-starkware wrote…

reduce some duplication

Done.

---

crates/native_blockifier/src/py_block_executor.rs line 115 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. Define a VersionedConstantsOverrides (without the Py) struct
2. implement From<PyVersionedConstantsOverrides> for VersionedConstantsOverrides
3. use it instead of the three params in get_versioned_constants

Done.

---

crates/native_blockifier/src/py_objects.rs line 65 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is this ever used in business logic...? I don't see any usage here

This is used in the Python test private_versioned_constants_test.py

---

crates/native_blockifier/src/py_validator.rs line 48 at r1 (raw file):

Previously, dorimedini-starkware wrote…

see above

Done.

[dorimedini-starkware]

Reviewed 5 of 6 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware)

---

crates/blockifier/src/versioned_constants.rs line 207 at r1 (raw file):

Previously, amosStarkware wrote…

how would you implement this?
i.e., how would an override of only validate_max_n_steps look like?

good question.
I can think of a solution using a macro; macro should accept the fields defining the versioned constants, and automatically defines VersionedConstants and VersionedConstantsOverrides: the latter equal to the former except every field is wrapped with Option<>.
the macro can also generate an apply_overrides(&mut self, overrides: &VersionedConstantsOverrides) method on VersionedConstants to update every non-None field.
needs thought, out of scope of this PR though; a TODO is enough

---

crates/native_blockifier/src/py_block_executor.rs line 109 at r3 (raw file):

            VersionedConstantsOverrides::from(py_versioned_constants_overrides);
        let versioned_constants =
            VersionedConstants::get_versioned_constants(versioned_constants_overrides);

more concice

Suggestion:

        let versioned_constants =
            VersionedConstants::get_versioned_constants(
                py_versioned_constants_overrides.into()
            );

---

crates/native_blockifier/src/py_validator.rs line 42 at r3 (raw file):

            VersionedConstantsOverrides::from(py_versioned_constants_overrides);
        let versioned_constants =
            VersionedConstants::get_versioned_constants(versioned_constants_overrides);

see above

Suggestion:

        let versioned_constants =
            VersionedConstants::get_versioned_constants(
                py_versioned_constants_overrides.into()
            );

[amosStarkware]

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/versioned_constants.rs line 207 at r1 (raw file):

Previously, dorimedini-starkware wrote…

good question.
I can think of a solution using a macro; macro should accept the fields defining the versioned constants, and automatically defines VersionedConstants and VersionedConstantsOverrides: the latter equal to the former except every field is wrapped with Option<>.
the macro can also generate an apply_overrides(&mut self, overrides: &VersionedConstantsOverrides) method on VersionedConstants to update every non-None field.
needs thought, out of scope of this PR though; a TODO is enough

Done.

---

crates/native_blockifier/src/py_block_executor.rs line 109 at r3 (raw file):

Previously, dorimedini-starkware wrote…

more concice

Done.

---

crates/native_blockifier/src/py_validator.rs line 42 at r3 (raw file):

Previously, dorimedini-starkware wrote…

see above

Done.

[amosStarkware]

+reviewer:@Yoni-Starkware

Reviewable status: 3 of 7 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

[dorimedini-starkware]

Reviewed 1 of 1 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)