feat(blockifier): add get_syscall_gas_cost to versioned constants impl

[Yonatan-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yonatan-Starkware]

• chore(blockifier): add syscalls_gas_costs struct #2359 : 2 dependent PRs (#2501 , #2514 )
• 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 👈 (View in Graphite)
• 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 triggered. View details here

[Yonatan-Starkware]

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

        let mut total_builtin_gas_cost: u64 = 0;
        for (builtin, amount) in &execution_resources.builtin_instance_counter {
            let builtin_cost = constant_gas_costs.get_builtin_gas_cost(builtin).unwrap_or(0);

for now, returning zero by default in the case of unsupported builtin. Would love to consider a better way to handle this error.

[codecov]

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.84%. Comparing base (e3165c4) to head (e5ebc1e).
Report is 613 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/versioned_constants.rs	96.29%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2188       +/-   ##
===========================================
+ Coverage   40.10%   68.84%   +28.74%     
===========================================
  Files          26      108       +82     
  Lines        1895    14025    +12130     
  Branches     1895    14025    +12130     
===========================================
+ Hits          760     9656     +8896     
- Misses       1100     3956     +2856     
- Partials       35      413      +378     

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

[Yonatan-Starkware]

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

    assert_eq!(versioned_constants.get_syscall_gas_cost(&SyscallSelector::CallContract), 87650);
    assert_eq!(versioned_constants.get_syscall_gas_cost(&SyscallSelector::Secp256k1Mul), 8143650);
    assert_eq!(versioned_constants.get_syscall_gas_cost(&SyscallSelector::Sha256ProcessBlock), 841095);

will change the magic numbers to one of two options:
(1) named constants as in other tests
(2)the matching value in the GasCosts struct.

The problem with the first option is that it will require maintenance every time the values in the json will change.
The problem with the second option is that after we will delete the gas costs from the OsConstants as planned it will become a bug.

[Yoni-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yonatan-Starkware)

---

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

Previously, Yonatan-Starkware wrote…

for now, returning zero by default in the case of unsupported builtin. Would love to consider a better way to handle this error.

It's okay to panic. All builtins should be there.

---

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

    /// Calculates a syscall's gas costs from the os resources
    pub fn get_syscall_gas_cost(&self, syscall_selector: &SyscallSelector) -> u64 {
        let constant_gas_costs = &self.os_constants.gas_costs;

Suggestion:

 gas_costs 

---

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

            .get(syscall_selector)
            .expect("Fetching the execution resources of a syscall should not fail.");
        let n_steps = std::cmp::max(u64_from_usize(execution_resources.n_steps), 100);

The max should be with syscall_base_gas_cost.
We should take the max at the end of the calculation. This will require further changes in python.
cc @meship-starkware

Suggestion:

        // The minimum total cost is `syscall_base_gas_cost`, which is pre-charged by the compiler.
        let n_steps = std::cmp::max(u64_from_usize(execution_resources.n_steps), 100);

[Yonatan-Starkware]

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

Previously, Yoni-Starkware (Yoni) wrote…

It's okay to panic. All builtins should be there.

Done.

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-24T11:40:35Z
• Started at 2024-11-24T11:51:00Z
• Started at 2024-11-24T11:55:32Z
• Started at 2024-11-24T12:23:23Z
• Started at 2024-11-24T12:37:51Z
• Started at 2024-11-24T12:49:02Z
• Started at 2024-11-24T13:33:55Z
• Started at 2024-11-24T14:22:14Z
• Started at 2024-11-24T14:56:42Z
• Started at 2024-11-24T14:56:27Z
• Started at 2024-11-24T14:56:42Z
• Started at 2024-11-24T14:57:17Z
• Started at 2024-11-24T14:57:13Z
• Started at 2024-11-24T15:10:31Z
• Started at 2024-11-24T16:11:59Z
• Started at 2024-11-24T16:17:32Z
• Started at 2024-11-24T16:18:45Z
• Started at 2024-11-28T08:03:14Z

[Yonatan-Starkware]

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

Previously, meship-starkware (Meshi Peled) wrote…

Why do you need this test? What does it give you that the syscalls test does not?

for now the function is just there, nothing call it. so by adding the test i verify that it works correctly, and prevent the error of it being unused

[Yoni-Starkware]

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

---

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

Previously, meship-starkware (Meshi Peled) wrote…

this will break Python. Lets put in the Os_constants the number according to the calculation without the max and add the maximum where needed. @Yoni-Starkware WDYT?

@meship-starkware why? he is not using this func yet
@Yonatan-Starkware please add my comment.

---

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

    }

    /// Calculates a syscall's gas costs from the os resources

Suggestion:

/// Calculates the syscall gas cost from the OS resources.

---

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

        let n_memory_holes = u64_from_usize(execution_resources.n_memory_holes);
        let mut total_builtin_gas_cost: u64 = 0;
        for (builtin, amount) in &execution_resources.builtin_instance_counter {

Use rust style instead (iter + map + sum)

Code quote:

        let mut total_builtin_gas_cost: u64 = 0;
        for (builtin, amount) in &execution_resources.builtin_instance_counter {

---

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

            let builtin_cost = gas_costs
                .get_builtin_gas_cost(builtin)
                .unwrap_or_else(|err| panic!("Failed to get gas cost: {}", err));

Suggestion:

(|err| panic!("Failed to get gas cost: {}.",

[Yoni-Starkware]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yonatan-Starkware)

---

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

Previously, Yonatan-Starkware wrote…

for now the function is just there, nothing call it. so by adding the test i verify that it works correctly, and prevent the error of it being unused

Sounds good

---

crates/blockifier/src/versioned_constants_test.rs line 172 at r3 (raw file):

#[test]
fn test_correct_syscall_gas_cost_calculation() {

Suggestion:

fn test_syscall_gas_cost_calculation() {

[Yonatan-Starkware]

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

            let builtin_cost = gas_costs
                .get_builtin_gas_cost(builtin)
                .unwrap_or_else(|err| panic!("Failed to get gas cost: {}", err));

there is already a coma in the error mesegge itself.

[Yonatan-Starkware]

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

Previously, Yonatan-Starkware wrote…

there is already a coma in the error mesegge itself.

the messege that goes into the curly braces

[Yoni-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yonatan-Starkware)

---

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

            .get(syscall_selector)
            .expect("Fetching the execution resources of a syscall should not fail.");
        // The minimum total cost is `syscall_base_gas_cost`, which is pre-charged by the compiler.

Move the message down to the cmp

Code quote:

// The minimum total cost is `syscall_base_gas_cost`, which is pre-charged by the compiler.

[meship-starkware]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

@meship-starkware why? he is not using this func yet
@Yonatan-Starkware please add my comment.

Oh, I see; I'll add a Monday task to change the Python accordingly.

[meship-starkware]

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

[Yoni-Starkware]

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

[Yoni-Starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware)