feat: add Cairo constants to VersionedConstants - unused

[giladchase]

Currently not using them, just adding the deserialization logic and tests.
In upcoming commits this will replace constants.rs.

The idea is that values inside cairo_constants that are json objects,
that is, have the form:

{
  A: x,
  B: y,
  C: z,
  ...
}

should be parsed as xA + yB + z*C + ..., where {x,y,z} must be
integers (otherwise an error is thrown), and where {A,B,C} must
correspond to keys that have already been parsed in the json.

Note: JSON standard doesn't enforce order on the keys, but our
implementation does; we should consider using a format that ensures
order, rather than relying on serde's implementation and IndexMap.

For reference, here's the way it looks in the cairo constants flie: https://github.com/starkware-industries/starkware/blob/daaa8ef1e466a1905f88749bb0dde2142e1bc9d7/src/starkware/starknet/core/os/constants.cairo#L1.

FYI @Yael-Starkware

Python: https://reviewable.io/reviews/starkware-industries/starkware/33612

---

This change is 

[codecov-commenter]

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (38fd92e) 70.37% compared to head (755a80c) 70.35%.
Report is 7 commits behind head on main-v0.13.1.

Files	Patch %	Lines
crates/blockifier/src/versioned_constants.rs	72.52%	22 Missing and 3 partials ⚠️
crates/native_blockifier/src/py_utils.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1379      +/-   ##
================================================
- Coverage         70.37%   70.35%   -0.02%     
================================================
  Files                60       60              
  Lines              7773     7849      +76     
  Branches           7773     7849      +76     
================================================
+ Hits               5470     5522      +52     
- Misses             1874     1895      +21     
- Partials            429      432       +3     

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

[elintul]

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)

---

crates/blockifier/resources/versioned_constants.json line 1 at r1 (raw file):

{

Consider alphabetizing (in each section - constants, nested JSONs).
Also, please point me to the origin so I can compare.

---

crates/blockifier/resources/versioned_constants.json line 2 at r1 (raw file):

{
  "cairo_os_constants": {

[starknet_]os_resources

Code quote:

cairo_os_constants

---

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

    // Cairo OS constants.
    // Note: if loaded from a json file, there are some assumptions made on its structure.
    // See the struct's docstring for more details.

How is it different from the current OsResources? Maybe we should use this name

Suggestion:

    // Starknet OS constants.
    // Note: if loaded from a json file, there are some assumptions made on its structure.
    // See the struct's docstring for more details.

[elintul]

Reviewed 1 of 3 files at r4, all commit messages.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)

[giladchase]

Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @elintul and @noaov1)

---

crates/blockifier/resources/versioned_constants.json line 1 at r1 (raw file):

Previously, elintul (Elin) wrote…

Consider alphabetizing (in each section - constants, nested JSONs).
Also, please point me to the origin so I can compare.

Did it for the other sections (as per your request on a previous PR 😬 ) but for cairo constants we can't do it, it is ordered in a different way: the constants appearing as keys in object types must appear before the object type used them.
For example:
FEE_TRANSFER_GAS_COST must appear after ENTRY_POINT_GAS_COST, because the former's value references ENTRY_POINT_GAS_COST (ya here alphabetized also works here but you know what i mean :) )

---

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

Previously, elintul (Elin) wrote…

How is it different from the current OsResources? Maybe we should use this name

Looks like versioned constants only includes gas costs, whereas os resources includes different info, like n_memory holes and n_steps.
For example, for GetExecutionInfo in os resources we have:

            "GetExecutionInfo": {
                "builtin_instance_counter": {
                    "range_check_builtin": 1
                },
                "n_memory_holes": 0,
                "n_steps": 62
            },

but in versioned constants it's

    "GET_EXECUTION_INFO_GAS_COST": {
      "SYSCALL_BASE_GAS_COST": 1,
      "STEP_GAS_COST": 10
    },

[elintul]

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

[elintul]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)

a discussion (no related file):
Let's go over this PR together?

---

[elintul]

Reviewed 2 of 9 files at r6, all commit messages.
Reviewable status: 2 of 9 files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)

[giladchase]

Reviewable status: 2 of 9 files reviewed, 3 unresolved discussions (waiting on @elintul and @noaov1)

a discussion (no related file):

Previously, elintul (Elin) wrote…

Let's go over this PR together?

Done.

---

[elintul]

Reviewed 1 of 9 files at r6, all commit messages.
Reviewable status: 3 of 9 files reviewed, 2 unresolved discussions (waiting on @noaov1)

[elintul]

Reviewed 1 of 9 files at r6, 3 of 4 files at r8, all commit messages.
Reviewable status: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @noaov1)

[elintul]

Reviewed all commit messages.
Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @noaov1)

[giladchase]

Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @elintul and @noaov1)

---

crates/blockifier/resources/versioned_constants.json line 1 at r1 (raw file):

Also, please point me to the origin so I can compare.

missed that, here it is.

[noaov1]

Reviewed all commit messages.
Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @elintul, @giladchase, and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 3 at r9 (raw file):

{
  "starknet_os_constants": {
    "nop_entry_point_offset": -1,

Is it possible to add documentation?

Code quote:

    "nop_entry_point_offset": -1,

---

crates/blockifier/resources/versioned_constants.json line 144 at r9 (raw file):

    "l2_gas": "'L2_GAS'",
    "l1_gas_index": 0,
    "l2_gas_index": 1

Those are not used is the blockifier. Do we want to add them anyway?

Code quote:

    "l1_gas": "'L1_GAS'",
    "l2_gas": "'L2_GAS'",
    "l1_gas_index": 0,
    "l2_gas_index": 1

---

crates/blockifier/src/versioned_constants.rs line 28 at r9 (raw file):

pub struct VersionedConstants {
    // Limits.
    pub invoke_tx_max_n_steps: u32,

Do we want it to be pub?

Code quote:

    pub invoke_tx_max_n_steps: u32,

[giladchase]

Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @elintul, @noaov1, and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 3 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Is it possible to add documentation?

Not really :( no comments in json.
If it helps, this variable isn't used, it's just here cause this part contains all the consts in constants.cairo (see link in pr description).

All that's used in the blockifier are the constants that end with _cost.

---

crates/blockifier/resources/versioned_constants.json line 144 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Those are not used is the blockifier. Do we want to add them anyway?

True, also everything else that doesn't end with _gas_cost isn't used (except for one that ends with _intial_budget).

Yep we want to load up all constants that have any relation to the blockifier into here, even if they aren't used, for the sake of transparency.

---

crates/blockifier/src/versioned_constants.rs line 28 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Do we want it to be pub?

Ya, we want this to be easily overridable by our users, to make it easier to run execute/validate with modified stats.

I blocked for the hashmaps just because they have invariants on them (must contain fixed keys).
We can create a specialized setter for them, but there are already plans for making them structs instead of hashmaps (@dorimedini-starkware already started with vm_resource_fee_costs), which solves this issue completely without any setters.

[elintul]

Reviewed 1 of 1 files at r9.
Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 244 at r9 (raw file):

#[derive(Debug, Error)]
pub enum StarknetConstantsSerdeError {

Are those only OS constants in the end?
Also, please alphabetize the variants.

Suggestion:

OsConstantsSerdeError

[giladchase]

Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @elintul, @noaov1, and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 2 at r9 (raw file):

{
  "starknet_os_constants": {

FYI @noaov1

After offline talk with @dorimedini-starkware we decided that it'd be better to dispense with the order of the keys, this would change the parsing logic, from a 1-pass parse, into a fixed-point iteration parse.

However, since this requires non-trivial changes, we'll add this in 13.2 (i'll get on implementing it right away though, but open the PR directly on 13.2).

The motivation for removing the order is due to: relying on sorting in an unsorted format like json is hackish, also the rest of the fields are sorted, and this one isn't, which is awkward.

---

crates/blockifier/src/versioned_constants.rs line 244 at r9 (raw file):

Previously, elintul (Elin) wrote…

Are those only OS constants in the end?
Also, please alphabetize the variants.

Done.

[dorimedini-starkware]

Reviewed 1 of 9 files at r6, 1 of 4 files at r8, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @elintul, @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 28 at r9 (raw file):

Previously, giladchase wrote…

Ya, we want this to be easily overridable by our users, to make it easier to run execute/validate with modified stats.

I blocked for the hashmaps just because they have invariants on them (must contain fixed keys).
We can create a specialized setter for them, but there are already plans for making them structs instead of hashmaps (@dorimedini-starkware already started with vm_resource_fee_costs), which solves this issue completely without any setters.

By design, if we need to store values per builtin, we want to keep hashmaps with builtins as keys (for flexibility).
Not sure how it's relevant to this comment but that's my two cents

---

crates/blockifier/src/versioned_constants.rs line 68 at r9 (raw file):

                    "Only gas costs listed in `StarknetOsConstants::GAS_COSTS` should be \
                     requested, got: {}",
                    name

can you output StarknetOSConstants::ALLOWED_GAS_COST_NAMES in this panic?

Code quote:

                    "Only gas costs listed in `StarknetOsConstants::GAS_COSTS` should be \
                     requested, got: {}",
                    name

---

crates/blockifier/src/versioned_constants.rs line 108 at r9 (raw file):

// FIXME FOLLOWUP: if we switch from JSON, we can switch to strongly typed fields, instead of an
// internal indexmap: using strongly typed fields breaks the order under serialization, making
// testing very difficult.

how painful is it to remove this order assumption, in this PR?
i.e. a recursive implementation / while loop implementation that does not rely on intermediate values appearing before their first usage?

Code quote:

// FIXME: JSON doesn't guarantee order, serde seems to work for this use-case, buit there is no
// guarantee that it will stay that way. Seriously consider switching to serde_yaml/other format.
// FIXME FOLLOWUP: if we switch from JSON, we can switch to strongly typed fields, instead of an
// internal indexmap: using strongly typed fields breaks the order under serialization, making
// testing very difficult.

---

crates/blockifier/src/versioned_constants.rs line 151 at r9 (raw file):

        "secp256r1_new_gas_cost",
        "keccak_gas_cost",
        "keccak_round_cost_gas_cost",

alphabetize?

Code quote:

        "step_gas_cost",
        "range_check_gas_cost",
        "memory_hole_gas_cost",
        "initial_gas_cost",
        "entry_point_initial_budget",
        "syscall_base_gas_cost",
        "entry_point_gas_cost",
        "fee_transfer_gas_cost",
        "transaction_gas_cost",
        "call_contract_gas_cost",
        "deploy_gas_cost",
        "get_block_hash_gas_cost",
        "get_execution_info_gas_cost",
        "library_call_gas_cost",
        "replace_class_gas_cost",
        "storage_read_gas_cost",
        "storage_write_gas_cost",
        "emit_event_gas_cost",
        "send_message_to_l1_gas_cost",
        "secp256k1_add_gas_cost",
        "secp256k1_get_point_from_x_gas_cost",
        "secp256k1_get_xy_gas_cost",
        "secp256k1_mul_gas_cost",
        "secp256k1_new_gas_cost",
        "secp256r1_add_gas_cost",
        "secp256r1_get_point_from_x_gas_cost",
        "secp256r1_get_xy_gas_cost",
        "secp256r1_mul_gas_cost",
        "secp256r1_new_gas_cost",
        "keccak_gas_cost",
        "keccak_round_cost_gas_cost",

---

crates/blockifier/src/versioned_constants.rs line 178 at r9 (raw file):

        let gas_cost_whitelist: IndexSet<_> =
            Self::ALLOWED_GAS_COST_NAMES.iter().copied().collect();
        for (key, value) in raw_json_data.raw_json_file_as_dict {

I think this inner loop can easily be made recursive (input: key, output: value + updates the gas costs in-place).
base case: check if the key is already in gas_costs, if so return the value.
then you don't need it sorted specifically.
WDYT?

Code quote:

(key, value) in raw_json_data.raw_json_file_as_dict {

---

crates/blockifier/src/versioned_constants.rs line 247 at r9 (raw file):

    #[error("Validation failed: {0}")]
    ValidationError(String),
    #[error("Value {1} for key '{0}' is out of range and cannot be cast into u64")]

aren't these backwards?

Code quote:

{1} for key '{0}'

---

crates/blockifier/src/versioned_constants.rs line 250 at r9 (raw file):

    OutOfRange(String, Number),
    #[error(
        "Value {1} used to create value for key '{0}' is out of range and cannot be cast into u64"

backwards? 0 and 1?

Code quote:

{1} used to create value for key '{0}'

---

crates/blockifier/src/versioned_constants_test.rs line 29 at r9 (raw file):

    // entry_point_intial_budget * 4 + step_gas_cost * 5.
    assert_eq!(versioned_constants.gas_cost("entry_point_gas_cost"), 34);

document by using expressions

Suggestion:

    assert_eq!(versioned_constants.gas_cost("step_gas_cost"), 2);
    assert_eq!(versioned_constants.gas_cost("entry_point_initial_budget"), 2 * 3); // step_gas_cost * 3.

    // entry_point_intial_budget * 4 + step_gas_cost * 5.
    assert_eq!(versioned_constants.gas_cost("entry_point_gas_cost"), 6 * 4 + 2 * 5);

[dorimedini-starkware]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @elintul, @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 2 at r9 (raw file):

Previously, giladchase wrote…

FYI @noaov1

After offline talk with @dorimedini-starkware we decided that it'd be better to dispense with the order of the keys, this would change the parsing logic, from a 1-pass parse, into a fixed-point iteration parse.

However, since this requires non-trivial changes, we'll add this in 13.2 (i'll get on implementing it right away though, but open the PR directly on 13.2).

The motivation for removing the order is due to: relying on sorting in an unsorted format like json is hackish, also the rest of the fields are sorted, and this one isn't, which is awkward.

wonder f we should revisit this... see below (non blocking)

[elintul]

Reviewed all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

[giladchase]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 28 at r9 (raw file):

Previously, dorimedini-starkware wrote…

By design, if we need to store values per builtin, we want to keep hashmaps with builtins as keys (for flexibility).
Not sure how it's relevant to this comment but that's my two cents

Let's discuss this once it becomes relevant, post 13.1 probably 😅

---

crates/blockifier/src/versioned_constants.rs line 68 at r9 (raw file):

Previously, dorimedini-starkware wrote…

can you output StarknetOSConstants::ALLOWED_GAS_COST_NAMES in this panic?

Done.

---

crates/blockifier/src/versioned_constants.rs line 108 at r9 (raw file):

Previously, dorimedini-starkware wrote…

how painful is it to remove this order assumption, in this PR?
i.e. a recursive implementation / while loop implementation that does not rely on intermediate values appearing before their first usage?

Let's handle this in a refactor, to be added very soon after we're done with 13.1.
Srry for the mess.

---

crates/blockifier/src/versioned_constants.rs line 151 at r9 (raw file):

Previously, dorimedini-starkware wrote…

alphabetize?

Can't 😬 because of order sensitivity.

---

crates/blockifier/src/versioned_constants.rs line 178 at r9 (raw file):

Previously, dorimedini-starkware wrote…

I think this inner loop can easily be made recursive (input: key, output: value + updates the gas costs in-place).
base case: check if the key is already in gas_costs, if so return the value.
then you don't need it sorted specifically.
WDYT?

Talked offline, will be handled post 13.1.

---

crates/blockifier/src/versioned_constants.rs line 247 at r9 (raw file):

Previously, dorimedini-starkware wrote…

aren't these backwards?

Nope it's Value {value} for key {key}, made it into a struct now to clarify.

[giladchase]

Reviewable status: 7 of 9 files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 250 at r9 (raw file):

Previously, dorimedini-starkware wrote…

backwards? 0 and 1?

Nope, like the rev comment, made it into a struct now to clarify.

---

crates/blockifier/src/versioned_constants_test.rs line 29 at r9 (raw file):

Previously, dorimedini-starkware wrote…

document by using expressions

Done. i kept the comments though cause they help with eplxaining it.

[giladchase]

Reviewable status: 7 of 9 files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware, @noaov1, and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 2 at r9 (raw file):

Previously, dorimedini-starkware wrote…

wonder f we should revisit this... see below (non blocking)

Post 13.1 😅

[dorimedini-starkware]

Reviewed 2 of 2 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yael-Starkware)

[noaov1]

Reviewed 1 of 9 files at r6, 1 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 144 at r9 (raw file):

Previously, giladchase wrote…

True, also everything else that doesn't end with _gas_cost isn't used (except for one that ends with _intial_budget).

Yep we want to load up all constants that have any relation to the blockifier into here, even if they aren't used, for the sake of transparency.

A suggestion (if you like it you can add a TODO): use the constants in this file instead of the file crates/blockifier/src/abi/constants.rs.

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yael-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 144 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

A suggestion (if you like it you can add a TODO): use the constants in this file instead of the file crates/blockifier/src/abi/constants.rs.

Started with that, but it's a can of worms.

If i use some of the constants from here rather than in constants.rs, i'll have to use all of them, for consistency, otherwise it'd be confusing why some constants are used and some aren't.

However, if we use them all, then the parsing logic would have to support also strings, and negative numbers, which will noticeably complicate the parsing logic, and the getters will be more complicated, and so on.

Better tackle that later.

[noaov1]

Reviewed 2 of 9 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

:)

[dorimedini-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion

---

crates/blockifier/src/versioned_constants_test.rs line 3 at r12 (raw file):

use pretty_assertions::assert_eq;

use super::*;

?

Code quote:

use super::*;