feat(blockifier): add impl for GasCosts in versioned_constants

[Yonatan-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 43 lines in your changes missing coverage. Please review.

Project coverage is 68.71%. Comparing base (e3165c4) to head (640e1dc).
Report is 560 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/versioned_constants.rs	0.00%	43 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2067       +/-   ##
===========================================
+ Coverage   40.10%   68.71%   +28.60%     
===========================================
  Files          26      109       +83     
  Lines        1895    13964    +12069     
  Branches     1895    13964    +12069     
===========================================
+ Hits          760     9595     +8835     
- Misses       1100     3959     +2859     
- Partials       35      410      +375     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[Yoni-Starkware]

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

---

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

    pub fn get_builtin_gas_cost(&self, builtin: &BuiltinName) -> u64 {
        match *builtin {
            BuiltinName::output => 0,

Please locate all zeros at the end and document them
segment_arena - it's a virtual builtin.
output - not relevant for smart contract.
The rest are not supported for cairo 1

---

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

            SyscallSelector::StorageRead => self.storage_read_gas_cost,
            SyscallSelector::StorageWrite => self.storage_write_gas_cost,
            _ => 0,

Please list all cases explicitly (we don't like default, it could hide bugs)

[avi-starkware]

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

[Yoni-Starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Yonatan-Starkware)

---

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

    }

    pub fn get_syscall_gas_cost_in_os_constants(&self, selector: &SyscallSelector) -> u64 {

Suggestion:

get_syscall_gas_cost

[Yoni-Starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Yonatan-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Please list all cases explicitly (we don't like default, it could hide bugs)

Actually, better to return an error.

[Yoni-Starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Yonatan-Starkware)

---

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

            BuiltinName::pedersen => self.pedersen_gas_cost,
            BuiltinName::ecdsa => 0,
            BuiltinName::keccak => 0,

Hmmm, might be a problem since we need a price for all builtins for the bouncer.
cc @meship-starkware

Code quote:

       BuiltinName::keccak => 0,

[github-actions]

Artifacts upload triggered. View details here

[avi-starkware]

Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @Yonatan-Starkware)

---

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

            BuiltinName::segment_arena => Err(GasCostsError::VirtualBuiltin),
            // The unsupported builtins in Cairo 1: output, ecdsa
            _ => Err(GasCostsError::UnsupportedBuiltin),

The comment would inevitably become stale. If there are only two cases, better to list them explicitly than mention them in a comment

Suggestion:

            BuiltinName::output => Err(GasCostsError::UnsupportedBuiltin),
            BuiltinName::ecdsa => Err(GasCostsError::UnsupportedBuiltin),
            _ => Err(GasCostsError::UnsupportedBuiltin),

---

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

    UnsupportedBuiltin,
    #[error("A virtual builin- not relevant for smart contracts")]
    VirtualBuiltin,

Suggestion:

    #[error("used syscall: {0} is not supported in a Cairo 0 contract")]
    UnsupportedCairo0Syscall,
    #[error("used builtin: {0} is not supported in a Cairo 1 contract")]
    UnsupportedCairo1Builtin,
    #[error("a virtual builtin cannot be used in a smart contract")]
    VirtualBuiltin,

[meship-starkware]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @Yonatan-Starkware)

[meship-starkware]

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

---

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

    pub fn get_builtin_gas_cost(&self, builtin: &BuiltinName) -> Result<u64, GasCostsError> {
        match *builtin {
            BuiltinName::range_check => Ok(self.range_check_gas_cost),

.

Suggestion:

 self.range_check_gas_cost,

---

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

            BuiltinName::range_check => Ok(self.range_check_gas_cost),
            BuiltinName::pedersen => Ok(self.pedersen_gas_cost),
            BuiltinName::keccak => Ok(self.keccak_gas_cost),

This is not the right price. This price is for the keccak syscall, which contains the builtin price and other resources.

Code quote:

self.keccak_gas_cost

---

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

            BuiltinName::segment_arena => Err(GasCostsError::VirtualBuiltin),
            // The unsupported builtins in Cairo 1: output, ecdsa
            _ => Err(GasCostsError::UnsupportedBuiltin),

Please do this explicitly (return error for all not supported builtins explicitly).

Code quote:

            // The unsupported builtins in Cairo 1: output, ecdsa
            _ => Err(GasCostsError::UnsupportedBuiltin),

---

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

            SyscallSelector::StorageRead => Ok(self.storage_read_gas_cost),
            SyscallSelector::StorageWrite => Ok(self.storage_write_gas_cost),
            _ => Err(GasCostsError::UnvalidSyscall),

Please do this explicitly (return error for all cairo 0 syscalls explicitly).

Code quote:

 _ => Err(GasCostsError::UnvalidSyscall),

[meship-starkware]

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

---

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

    UnsupportedBuiltin,
    #[error("A virtual builin- not relevant for smart contracts")]
    VirtualBuiltin,

#[error("used syscall: {0} is not supported in a Cairo 1 contract")]
UnsupportedCairo1Syscall,

[meship-starkware]

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

---

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

Previously, meship-starkware (Meshi Peled) wrote…

This is not the right price. This price is for the keccak syscall, which contains the builtin price and other resources.

The right price is 136189. Lets talk about it f2f as well.

[meship-starkware]

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

[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
• feat(blockifier): add impl for GasCosts in versioned_constants #2067 👈 (View in Graphite)
• main

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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

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

---

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

            // The following are unsupported builtins in Cairo 1
            BuiltinName::output => Err(GasCostsError::UnsupportedCairo1Builtin {builtin: *builtin,}),
            BuiltinName::ecdsa => Err(GasCostsError::UnsupportedCairo1Builtin {builtin: *builtin,}),

Gather these variants in the same arm

Code quote:

            // The following are unsupported builtins in Cairo 1
            BuiltinName::output => Err(GasCostsError::UnsupportedCairo1Builtin {builtin: *builtin,}),
            BuiltinName::ecdsa => Err(GasCostsError::UnsupportedCairo1Builtin {builtin: *builtin,}),

[github-actions]

Artifacts upload triggered. View details here

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 577 at r5 (raw file):

            BuiltinName::ec_op => Ok(self.ecop_gas_cost),
            //TODO (Yonatan): once keccak_builtin_gas_cost is being inserted to the versioned constants, replace the constant with field's value 
            BuiltinName::keccak => Ok(KECCAK_BUILTIN_GAS_COST), 

For now I put the correct price manually. I suggest that for now we will merge it like this. This will help us achieve a separation between this task and the task of inserting keccak_builtin_gas_cost to the versioned constants. I think this is important because the latter will involve changes in the python repo as well, and thus take a little longer.

[Yoni-Starkware]

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

---

crates/blockifier/src/versioned_constants.rs line 827 at r5 (raw file):

pub enum GasCostsError {
    #[error("used syscall: {:?} is not supported in a Cairo 0 contract", selector)]
    UnsupportedCairo1Syscall {selector: SyscallSelector},

Suggestion:

DeprecatedSyscall

---

crates/blockifier/src/versioned_constants.rs line 829 at r5 (raw file):

    UnsupportedCairo1Syscall {selector: SyscallSelector},
    #[error("used builtin: {:?} is not supported in a Cairo 1 contract", builtin)]
    UnsupportedCairo1Builtin {builtin: BuiltinName},

Suggestion:

UnsupportedBuiltinInCairo1

[Yoni-Starkware]

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

---

crates/blockifier/src/versioned_constants.rs line 615 at r5 (raw file):

            SyscallSelector::StorageWrite => Ok(self.storage_write_gas_cost),
            // The following are unsupported syscalls in Cairo 1
            SyscallSelector::DelegateCall => Err(GasCostsError::UnsupportedCairo1Syscall{selector: *selector,}),

Gather them in the same arm

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

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

---

crates/blockifier/src/versioned_constants.rs line 584 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

The error is self-explenatory

(Remove the comment)

---

crates/blockifier/src/versioned_constants.rs line 614 at r7 (raw file):

            SyscallSelector::StorageRead => Ok(self.storage_read_gas_cost),
            SyscallSelector::StorageWrite => Ok(self.storage_write_gas_cost),
            // The following are unsupported syscalls in Cairo 1

(Remove the comment)

[Yoni-Starkware]

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

---

crates/blockifier/src/versioned_constants.rs line 597 at r10 (raw file):

    pub fn get_syscall_gas_cost(&self, selector: &SyscallSelector) -> Result<u64, GasCostsError> {
        match selector {
            SyscallSelector::CallContract => Ok(self.call_contract_gas_cost),

Remove the Ok here as well

Code quote:

Ok(

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-24T11:40:27Z
• Started at 2024-11-24T11:50:54Z
• Started at 2024-11-24T12:37:42Z
• Started at 2024-11-24T12:47:02Z
• Started at 2024-11-24T13:49:26Z
• Started at 2024-11-24T13:54:58Z
• Started at 2024-11-24T14:07:11Z

[Yoni-Starkware]

Reviewed 1 of 1 files at r11.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yonatan-Starkware)

[Yonatan-Starkware]

crates/blockifier/src/versioned_constants.rs line 597 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Remove the Ok here as well

done

[Yoni-Starkware]

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

[meship-starkware]

Reviewed 1 of 3 files at r3, 1 of 2 files at r10, 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware)

[meship-starkware]

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

[Yoni-Starkware]

Reviewed 1 of 2 files at r13.
Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @avi-starkware and @meship-starkware)

[Yoni-Starkware]

Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware)

OOO