refactor(blockifier): make VersionedConstantsOverrides fields optional

[ayeletstarkware]

I plan to make PyVersionedConstantsOverrides fields optional as well.

[reviewable-StarkWare]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 56.52174% with 10 lines in your changes missing coverage. Please review.

Project coverage is 46.73%. Comparing base (e3165c4) to head (fac0009).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/versioned_constants.rs	54.54%	2 Missing and 3 partials ⚠️
crates/native_blockifier/src/py_objects.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1496      +/-   ##
==========================================
+ Coverage   40.10%   46.73%   +6.63%     
==========================================
  Files          26      210     +184     
  Lines        1895    24657   +22762     
  Branches     1895    24657   +22762     
==========================================
+ Hits          760    11524   +10764     
- Misses       1100    12343   +11243     
- Partials       35      790     +755     

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

[ArniStarkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

---

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

        constants
    }

Nice

Code quote:

    /// Returns the latest versioned constants after applying the given overrides.
    pub fn get_versioned_constants(
        versioned_constants_overrides: VersionedConstantsOverrides,
    ) -> Self {
        let mut constants = Self::latest_constants().clone();

        if let Some(validate_max_n_steps) = versioned_constants_overrides.validate_max_n_steps {
            constants.validate_max_n_steps = validate_max_n_steps;
        }
        if let Some(max_recursion_depth) = versioned_constants_overrides.max_recursion_depth {
            constants.max_recursion_depth = max_recursion_depth;
        }
        if let Some(invoke_tx_max_n_steps) = versioned_constants_overrides.invoke_tx_max_n_steps {
            constants.invoke_tx_max_n_steps = invoke_tx_max_n_steps;
        }

        constants
    }

---

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

        }
    }
}

This is the whole idea (or is it blocking this PR?)

We can simply derive it. "This is because the default value for Option<T> is None, regardless of the type T."

Suggestion:

impl Default for VersionedConstantsOverrides {
    // TODO: update the default values once the actual values are known.
    fn default() -> Self {
        Self {
            validate_max_n_steps: None,
            max_recursion_depth: None,
            invoke_tx_max_n_steps: None,
        }
    }
}

[ArniStarkware]

crates/blockifier/src/versioned_constants_test.rs line 58 at r1 (raw file):

    assert_eq!(result.validate_max_n_steps, updated_validate_max_n_steps);
    assert_eq!(result.max_recursion_depth, updated_max_recursion_depth);
}

Add one more test for the None case. i.e. test the scenario the removed latest_constants_with_overrides solved.

[ArniStarkware]

Reviewed 3 of 4 files at r1.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 104 at r1 (raw file):

        };
        let versioned_constants =
            VersionedConstants::get_versioned_constants(versioned_constants_overrides);

Nice 2.

Code quote:

        let versioned_constants_overrides = VersionedConstantsOverrides {
            validate_max_n_steps: Some(self.config.validate_max_n_steps),
            max_recursion_depth: Some(self.config.max_recursion_depth),
            invoke_tx_max_n_steps: None,
        };
        let versioned_constants =
            VersionedConstants::get_versioned_constants(versioned_constants_overrides);

[ayeletstarkware]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

This is the whole idea (or is it blocking this PR?)

We can simply derive it. "This is because the default value for Option<T> is None, regardless of the type T."

Done.

---

crates/blockifier/src/versioned_constants_test.rs line 58 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Add one more test for the None case. i.e. test the scenario the removed latest_constants_with_overrides solved.

How's this change? checks both overrides and not overrides in one test.

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, all discussions resolved

---

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

        constants
    }

Non-blocking. We can do the following. write a macro that gets the field name and does the override if need be.

Code quote:

    /// Returns the latest versioned constants after applying the given overrides.
    pub fn get_versioned_constants(
        versioned_constants_overrides: VersionedConstantsOverrides,
    ) -> Self {
        let mut constants = Self::latest_constants().clone();

        if let Some(validate_max_n_steps) = versioned_constants_overrides.validate_max_n_steps {
            constants.validate_max_n_steps = validate_max_n_steps;
        }
        if let Some(max_recursion_depth) = versioned_constants_overrides.max_recursion_depth {
            constants.max_recursion_depth = max_recursion_depth;
        }
        if let Some(invoke_tx_max_n_steps) = versioned_constants_overrides.invoke_tx_max_n_steps {
            constants.invoke_tx_max_n_steps = invoke_tx_max_n_steps;
        }

        constants
    }

---

crates/blockifier/src/versioned_constants_test.rs line 58 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

How's this change? checks both overrides and not overrides in one test.

Love it.

[ArniStarkware]

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

Previously, ArniStarkware (Arnon Hod) wrote…

Non-blocking. We can do the following. write a macro that gets the field name and does the override if need be.

But no need.

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

[ArniStarkware]

crates/blockifier/src/versioned_constants.rs line 837 at r3 (raw file):

                "Maximum number of steps the validation function is allowed to run.",
                ParamPrivacyInput::Public,
            ),

I think this will solve all of our issues.

The corresponding #is_none flag will be turned on because of this is what the default of VersionedConstantsOverrides does!

Suggestion:

            ser_optional_param(
                "validate_max_n_steps",
                &self.validate_max_n_steps,
                "Maximum number of steps the validation function is allowed to run.",
                ParamPrivacyInput::Public,
            ),