chore(blockifier): add gas cost to bultin price array

[meship-starkware]

This change is 

[noaov1]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 204 at r1 (raw file):

        MaybeRelocatable::from(500),  // Posidon
        MaybeRelocatable::from(234),  // AddMod
        MaybeRelocatable::from(616),  // MulMod

Please define those constants in the versioned constant file and use it here

Code quote:

        MaybeRelocatable::from(4130), // Pesetsen
        MaybeRelocatable::from(594),  // Bitwise
        MaybeRelocatable::from(4166), // EcOp
        MaybeRelocatable::from(500),  // Posidon
        MaybeRelocatable::from(234),  // AddMod
        MaybeRelocatable::from(616),  // MulMod

---

crates/blockifier/src/execution/entry_point_execution.rs line 210 at r1 (raw file):

    // for _i in 0..20 {
    //     data.push(MaybeRelocatable::from(0));
    // }

Please delete :)

Code quote:

    // // TODO(spapini): Put real costs here.
    // for _i in 0..20 {
    //     data.push(MaybeRelocatable::from(0));
    // }

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, BALANCE};

#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 17051678; "VM")]

Why is that the only test with a gas consumption change? 🤔

Code quote:

#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 17051678; "VM")]

[meship-starkware]

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

---

crates/blockifier/src/execution/entry_point_execution.rs line 210 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please delete :)

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is that the only test with a gas consumption change? 🤔

I think it is because this is the only syscalls aside from deploy that uses a builtin other than range check, and because I rebased it over main, we have not tested deploy yet. I'll try to rebase over the test deploy gas cost and see what will happen.

[meship-starkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I think it is because this is the only syscalls aside from deploy that uses a builtin other than range check, and because I rebased it over main, we have not tested deploy yet. I'll try to rebase over the test deploy gas cost and see what will happen.

it looks like that is not the solution, as deploy gas cost has not changed

[meship-starkware]

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

---

crates/blockifier/src/execution/entry_point_execution.rs line 204 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please define those constants in the versioned constant file and use it here

Done.

[codecov]

Codecov Report

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

Project coverage is 76.62%. Comparing base (676c4a4) to head (73c2fc3).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
.../blockifier/src/execution/entry_point_execution.rs	95.45%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
- Coverage   76.63%   76.62%   -0.01%     
==========================================
  Files         351      351              
  Lines       37092    37107      +15     
  Branches    37092    37107      +15     
==========================================
+ Hits        28424    28433       +9     
- Misses       6369     6373       +4     
- Partials     2299     2301       +2     

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

[meship-starkware]

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

it looks like that is not the solution, as deploy gas cost has not changed

@ilyalesokhin-starkware Do you have any idea why the cost of the secp might change as a result of updating the builtins price array?
Also, the price from r1 is the price after updating the builtins array and before updating the syscall gas_cost
and the price in r2 is after updating both the gas cost and builtin array prices.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

@ilyalesokhin-starkware Do you have any idea why the cost of the secp might change as a result of updating the builtins price array?
Also, the price from r1 is the price after updating the builtins array and before updating the syscall gas_cost
and the price in r2 is after updating both the gas cost and builtin array prices.

Yes, contracts use that array when updating the gas counter.

[meship-starkware]

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Yes, contracts use that array when updating the gas counter.

Is there a resone Only sekpk1 was affected? It sounds like the call contract syscalls should have been affected as well.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Is there a resone Only sekpk1 was affected? It sounds like the call contract syscalls should have been affected as well.

Do the other tests use builtins?

[meship-starkware]

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Do the other tests use builtins?

I am not sure what you mean by asking if the test uses builtins, but from the os resources, the only syscalls that use builtins that are not rangecheck are deployed sha256 and keccak; as for the code in the test contract, I don't see much difference between secpk1 and secpr1.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure what you mean by asking if the test uses builtins, but from the os resources, the only syscalls that use builtins that are not rangecheck are deployed sha256 and keccak; as for the code in the test contract, I don't see much difference between secpk1 and secpr1.

The secp256k1 test calls verify_eth_signature which does keccak.
the secpr1 test does not do that.

[meship-starkware]

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 12 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

The secp256k1 test calls verify_eth_signature which does keccak.
the secpr1 test does not do that.

Cool that would explain it thanks.

[noaov1]

Reviewed 1 of 2 files at r2, 7 of 9 files at r3, all commit messages.
Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @meship-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 201 at r3 (raw file):

    contract_class: &ContractClassV1,
    read_only_segments: &mut ReadOnlySegments,
    versioned_constants: &VersionedConstants,

Suggestion:

    gas_costs: &GasCosts,

---

crates/blockifier/src/execution/entry_point_execution.rs line 203 at r3 (raw file):

    versioned_constants: &VersionedConstants,
) -> Result<usize, PreExecutionError> {
    // Create the builtin cost segment, with dummy values.

Can you please refer to a source that determines the correct order of the builtins?

Code quote:

    // Create the builtin cost segment, with dummy values.

[meship-starkware]

Reviewable status: 7 of 10 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @noaov1)

---

crates/blockifier/src/execution/entry_point_execution.rs line 203 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please refer to a source that determines the correct order of the builtins?

I am unsure about adding the function name to the comment, but I think it would be the easiest way to find it.

[noaov1]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @meship-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 203 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am unsure about adding the function name to the comment, but I think it would be the easiest way to find it.

I wonder if this method is the what counts (and in that case, consider using it), or the OS itself.

---

crates/blockifier/src/execution/entry_point_execution.rs line 211 at r4 (raw file):

        gas_costs.poseidon_gas_cost,
        gas_costs.add_mod_gas_cost,
        gas_costs.mul_mod_gas_cost,

Those prices are defined in the compiler as well (here).
Why is it needed in the complier?
Do we need to use the same costs? I suspect that no
@ilyalesokhin-starkware

Code quote:

        gas_costs.pedersen_gas_cost,
        gas_costs.bitwise_builtin_gas_cost,
        gas_costs.ecop_gas_cost,
        gas_costs.poseidon_gas_cost,
        gas_costs.add_mod_gas_cost,
        gas_costs.mul_mod_gas_cost,

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 211 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Those prices are defined in the compiler as well (here).
Why is it needed in the complier?
Do we need to use the same costs? I suspect that no
@ilyalesokhin-starkware

The compiler has a framework to run Cairo code without relying on the sequencer, so it needs those prices.
The prices should be the same, but nothing will break if they are different.

[ilyalesokhin-starkware]

Reviewed 1 of 2 files at r2, 7 of 9 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

[meship-starkware]

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

---

crates/blockifier/src/execution/entry_point_execution.rs line 203 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I wonder if this method is the what counts (and in that case, consider using it), or the OS itself.

Done.

[noaov1]

Reviewed 9 of 10 files at r6, 1 of 10 files at r7, all commit messages.
Reviewable status: 10 of 19 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 211 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

The compiler has a framework to run Cairo code without relying on the sequencer, so it needs those prices.
The prices should be the same, but nothing will break if they are different.

Why should they be the same?
Is the compiler uses it for testing?

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 211 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why should they be the same?
Is the compiler uses it for testing?

If the prices are different, then the testing framework would give different gas costs than Starknet.

Yes, The compiler uses it for testing.

[ilyalesokhin-starkware]

Reviewed 9 of 10 files at r6, 8 of 10 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)