chore(blockifier): split builtin gas costs from base gas costs

[Yonatan-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yonatan-Starkware]

• chore(blockifier): split builtin gas costs from base gas costs #2514 : 2 dependent PRs (#2818 , #2838 ) 👈 (View in Graphite)
• chore(blockifier): add syscalls_gas_costs struct #2359 : 1 other dependent PR (#2501 )
• chore(blockifier): add keccak_builtin_gas_cost #2327
• chore(blockifier): replace entry_point_gas_cost with initial_budget #2247
• feat(blockifier): add get_syscall_gas_cost to versioned constants impl #2188
• feat(blockifier): add impl for GasCosts in versioned_constants #2067
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-08T08:45:21Z
• Started at 2024-12-08T10:25:12Z
• Started at 2024-12-08T10:29:10Z
• Started at 2024-12-08T11:58:36Z
• Started at 2024-12-08T12:11:13Z
• Started at 2024-12-08T12:11:46Z
• Started at 2024-12-08T12:11:46Z
• Started at 2024-12-08T12:12:19Z
• Started at 2024-12-08T12:12:15Z
• Started at 2024-12-08T12:17:57Z
• Started at 2024-12-08T13:32:19Z
• Started at 2024-12-15T07:39:32Z
• Started at 2024-12-15T12:06:46Z
• Started at 2024-12-15T12:09:56Z
• Started at 2024-12-15T13:34:47Z
• Started at 2024-12-15T13:57:23Z
• Started at 2024-12-15T14:59:58Z
• Started at 2024-12-15T15:04:12Z
• Started at 2024-12-19T07:29:11Z
• Started at 2024-12-19T09:26:41Z
• Started at 2024-12-19T09:38:01Z
• Started at 2024-12-19T10:57:26Z
• Started at 2024-12-19T12:18:25Z
• Started at 2024-12-19T12:40:24Z

[codecov]

Codecov Report

Attention: Patch coverage is 52.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 68.14%. Comparing base (e3165c4) to head (9e5b0c7).
Report is 822 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/versioned_constants.rs	45.45%	16 Missing and 8 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2514       +/-   ##
===========================================
+ Coverage   40.10%   68.14%   +28.04%     
===========================================
  Files          26      105       +79     
  Lines        1895    14332    +12437     
  Branches     1895    14332    +12437     
===========================================
+ Hits          760     9767     +9007     
- Misses       1100     4136     +3036     
- Partials       35      429      +394     

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

[meship-starkware]

Reviewed 8 of 10 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 650 at r2 (raw file):

    // An estimation of the initial gas for a transaction to run with. This solution is
    // temporary and this value will be deduced from the transaction's fields.
    pub default_initial_gas_cost: u64,

This is what the transaction costs were till now, right?

Code quote:

 pub default_initial_gas_cost: u64,

[meship-starkware]

Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 662 at r2 (raw file):

    pub range_check_gas_cost: u64,
    // Priced builtins.
    pub keccak_builtin_gas_cost: u64,

Let's remove the builtin from every builtin that has it because it is inconsistent and unnecessary now that they are under the builtin section.

Suggestion:

pub keccak_gas_cost: u64,

---

crates/blockifier/src/versioned_constants.rs line 753 at r2 (raw file):

        "syscall_gas_costs",
        "builtin_gas_costs",
    ];

Are not all the keys in the base now? We should aim to remove this constant, but this is outside of this PR scope.

Code quote:

    const ADDITIONAL_FIELDS: [&'static str; 31] = [
        "block_hash_contract_address",
        "constructor_entry_point_selector",
        "default_entry_point_selector",
        "entry_point_type_constructor",
        "entry_point_type_external",
        "entry_point_type_l1_handler",
        "error_block_number_out_of_range",
        "error_invalid_input_len",
        "error_invalid_argument",
        "error_entry_point_failed",
        "error_entry_point_not_found",
        "error_out_of_gas",
        "execute_entry_point_selector",
        "l1_gas",
        "l1_gas_index",
        "l1_handler_version",
        "l2_gas",
        "l2_gas_index",
        "l1_data_gas",
        "l1_data_gas_index",
        "nop_entry_point_offset",
        "sierra_array_len_bound",
        "stored_block_hash_buffer",
        "transfer_entry_point_selector",
        "validate_declare_entry_point_selector",
        "validate_deploy_entry_point_selector",
        "validate_entry_point_selector",
        "validate_rounding_consts",
        "validated",
        "syscall_gas_costs",
        "builtin_gas_costs",
    ];

---

crates/blockifier/src/versioned_constants.rs line 765 at r2 (raw file):

        let builtins: BuiltinGasCosts = serde_json::from_value(builtins_value)?;
        let syscalls_value: Value =
            serde_json::to_value(&raw_json_data.parse_syscalls(&base, &builtins)?)?;

Here, the way to make sure you have the builtins before you need to calculate the syscall is to call the builtin parse before you call the syscall parse, right?

Code quote:

        let builtins_value: Value = serde_json::to_value(&raw_json_data.parse_builtin()?)?;
        let builtins: BuiltinGasCosts = serde_json::from_value(builtins_value)?;
        let syscalls_value: Value =
            serde_json::to_value(&raw_json_data.parse_syscalls(&base, &builtins)?)?;

---

crates/blockifier/src/versioned_constants.rs line 961 at r2 (raw file):

                        "default_initial_gas_cost" => base.default_initial_gas_cost,
                        "entry_point_initial_budget" => base.entry_point_initial_budget,
                        "syscall_base_gas_cost" => base.syscall_base_gas_cost,

I think it is easier to read this way

Suggestion:

                    let inner_value = match inner_key.as_str() {
                        "step_gas_cost" => base.step_gas_cost,
                        "memory_hole_gas_cost" => base.memory_hole_gas_cost,
                        "default_initial_gas_cost" => base.default_initial_gas_cost,
                        "entry_point_initial_budget" => base.entry_point_initial_budget,
                        "syscall_base_gas_cost" => base.syscall_base_gas_cost,
                        "range_check_gas_cost" => builtins.range_check_gas_cost,
                        "keccak_builtin_gas_cost" => builtins.keccak_builtin_gas_cost,
                        "pedersen_gas_cost" => builtins.pedersen_gas_cost,
                        "bitwise_builtin_gas_cost" => builtins.bitwise_builtin_gas_cost,
                        "ecop_gas_cost" => builtins.ecop_gas_cost,
                        "poseidon_gas_cost" => builtins.poseidon_gas_cost,
                        "add_mod_gas_cost" => builtins.add_mod_gas_cost,
                        "mul_mod_gas_cost" => builtins.mul_mod_gas_cost,
                        "ecdsa_gas_cost" => builtins.ecdsa_gas_cost,

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 662 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Let's remove the builtin from every builtin that has it because it is inconsistent and unnecessary now that they are under the builtin section.

Agree.
Specifically for keccak though, it might be confusing, because we also have the price of the syscall (it's name is just "keccak")

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 753 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Are not all the keys in the base now? We should aim to remove this constant, but this is outside of this PR scope.

no, all the keys that are in BaseGasCosts are not here.
Actually the number of fields that are now in this list is much larger then the number of those that are not in the list. Might be cleaner to replace this list with a list of the ones that we don't want to skip

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 765 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Here, the way to make sure you have the builtins before you need to calculate the syscall is to call the builtin parse before you call the syscall parse, right?

yes

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 961 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I think it is easier to read this way

Done.

[meship-starkware]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

[meship-starkware]

Reviewed 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @noaov1, @Yonatan-Starkware, and @Yoni-Starkware)

[noaov1]

Reviewed 1 of 3 files at r2, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants_0_13_3.json line 80 at r4 (raw file):

            "entry_point_initial_budget": 1,
            "step_gas_cost": 600
        },

Now that you removed transaction_gas_cost, this can be deleted as well (can be done in a separate PR)

Code quote:

        "fee_transfer_gas_cost": {
            "entry_point_initial_budget": 1,
            "step_gas_cost": 600
        },

---

crates/blockifier/src/versioned_constants.rs line 719 at r4 (raw file):

    // List of additinal os constants, beside the gas cost and validate rounding constants, that are
    // not used by the blockifier but included for transparency. These constanst will be ignored
    // during the creation of the struct containing the gas costs.

Suggestion:

    // List of os constants to be ignored
    // during the creation of the struct containing the base gas costs.

---

crates/blockifier/src/versioned_constants.rs line 752 at r4 (raw file):

        "validated",
        "syscall_gas_costs",
        "builtin_gas_costs",

Please keep the list alphabetize.

Code quote:

        "builtin_gas_costs",

[noaov1]

Reviewed 8 of 9 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

[meship-starkware]

Reviewed 5 of 9 files at r5, 1 of 1 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 719 at r4 (raw file):

    // List of additinal os constants, beside the gas cost and validate rounding constants, that are
    // not used by the blockifier but included for transparency. These constanst will be ignored
    // during the creation of the struct containing the gas costs.

Done.

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 752 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please keep the list alphabetize.

Done.

[Yonatan-Starkware]

crates/blockifier/resources/versioned_constants_0_13_3.json line 80 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Now that you removed transaction_gas_cost, this can be deleted as well (can be done in a separate PR)

No problem. opening another PR

[meship-starkware]

Reviewed all commit messages.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)