refactor(blockifier): fix gas cost in versioned constants

[meship-starkware]

This change is 

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 80 at r4 (raw file):

}

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

why is this cheaper now?

Code quote:

5210

[ilyalesokhin-starkware]

Reviewed 2 of 11 files at r4, all commit messages.
Reviewable status: 2 of 12 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)

[meship-starkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 80 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

why is this cheaper now?

Because the storage_read and storage_write are cheaper. It seems like deploy gas cost does not affect this test. I will investigate further to see why.

[meship-starkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 80 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Because the storage_read and storage_write are cheaper. It seems like deploy gas cost does not affect this test. I will investigate further to see why.

Ok, after some investigation, I saw that the call data that we are checking is the first inner call, which is the constructor
that executes one storage write, so these changes make sense. It looks like this is the intention of this test, but the result is that we do not have a test that checks the consumption of gas for the deploy syscall.

---

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 120 at r4 (raw file):

            gas_consumed: expected_gas,
            ..CallExecution::default()
        }

These lines show that we only checking the execution result of the contractor and the expected return data shows that this is the intension of this test.

Code quote:

    let deploy_call = &entry_point_call.execute_directly(&mut state).unwrap().inner_calls[0];

    assert_eq!(deploy_call.call.storage_address, contract_address);
    assert_eq!(
        deploy_call.execution,
        CallExecution {
            retdata: retdata![constructor_calldata[0]],
            gas_consumed: expected_gas,
            ..CallExecution::default()
        }

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 80 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Ok, after some investigation, I saw that the call data that we are checking is the first inner call, which is the constructor
that executes one storage write, so these changes make sense. It looks like this is the intention of this test, but the result is that we do not have a test that checks the consumption of gas for the deploy syscall.

Can you document that?

[ilyalesokhin-starkware]

Reviewed 1 of 3 files at r1, 8 of 11 files at r4.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @noaov1)

[meship-starkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 80 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

Can you document that?

Done.

[ilyalesokhin-starkware]

Reviewed 1 of 11 files at r4.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @noaov1)

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.79%. Comparing base (13ae058) to head (d8a248b).
Report is 32 commits behind head on main.

Files	Patch %	Lines
crates/papyrus_execution/src/lib.rs	0.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
+ Coverage   76.62%   76.79%   +0.16%     
==========================================
  Files         346      348       +2     
  Lines       36263    36319      +56     
  Branches    36263    36319      +56     
==========================================
+ Hits        27786    27890     +104     
+ Misses       6175     6133      -42     
+ Partials     2302     2296       -6     

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

[noaov1]

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

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

        "nop_entry_point_offset": -1,
        "range_check_gas_cost": 70,
        "pedersen_gas_cost": 4130,

How did we decide to set the gas cost for the builtins? It seems inconsistent. pedersen equals trace cells multiplied by 2, while the range check does not.

Code quote:

        "range_check_gas_cost": 70,
        "pedersen_gas_cost": 4130,

---

crates/blockifier/resources/versioned_constants.json line 106 at r5 (raw file):

        "bitwise_builtin_gas_cost": 594,
        "replace_class_gas_cost": {
            "step_gas_cost": 100,

Q RE the max(n_steps_resource, syscall_base_gas_cost) methodology. Here we refund the syscall base gas cost from the syscall required gas, hence we will get that the required gas for a syscall can be 0 (if no builtins are used). This is this the desired behaviour, right? If a syscall costs less than 100 gas than there is no need to charge more, right?
@ilyalesokhin-starkware

Code quote:

          "step_gas_cost": 100,

---

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 112 at r5 (raw file):

    .unwrap();
    // Note, this it the call info of the constructor call. Meaning the CallExecution is the
    // execution of the constructor and not the deploy syscall.

Suggestion:

    // Note, this it the call info of the constructor call (inner call).

[noaov1]

Reviewed 1 of 3 files at r1, 8 of 11 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

            "range_check_gas_cost": 15

        },

How come call contract got so much more expensive?
@ilyalesokhin-starkware

Code quote:

        "call_contract_gas_cost": {
            "entry_point_gas_cost": 1,
            "step_gas_cost": 827,
            "range_check_gas_cost": 15

        },

[ilyalesokhin-starkware]

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

How come call contract got so much more expensive?
@ilyalesokhin-starkware

The original price was probably wrong, but the new price seems expensive.
We need to make sure the os resources test measures it correctly.

[ilyalesokhin-starkware]

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

How did we decide to set the gas cost for the builtins? It seems inconsistent. pedersen equals trace cells multiplied by 2, while the range check does not.

We decided to make the rangecheck more expensive then their actual prices.

[ilyalesokhin-starkware]

crates/blockifier/resources/versioned_constants.json line 106 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Q RE the max(n_steps_resource, syscall_base_gas_cost) methodology. Here we refund the syscall base gas cost from the syscall required gas, hence we will get that the required gas for a syscall can be 0 (if no builtins are used). This is this the desired behaviour, right? If a syscall costs less than 100 gas than there is no need to charge more, right?
@ilyalesokhin-starkware

yes

[ilyalesokhin-starkware]

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

[noaov1]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @meship-starkware)

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

We decided to make the rangecheck more expensive then their actual prices.

Can you please explain why?

[meship-starkware]

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

---

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

The original price was probably wrong, but the new price seems expensive.
We need to make sure the os resources test measures it correctly.

I checked my pr that changed the os_resources. Before the changes, it was 690 steps, and after those changes (cairo1), it was 760.
I don't know where the 10 came from.

[ilyalesokhin-starkware]

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I checked my pr that changed the os_resources. Before the changes, it was 690 steps, and after those changes (cairo1), it was 760.
I don't know where the 10 came from.

"after those changes (cairo1), it was 760."

what changes?

[ilyalesokhin-starkware]

Reviewed 1 of 14 files at r6.
Reviewable status: 2 of 15 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please explain why?

We took a safety margin for the rangecheck is this cost is hardcoded in the contracts.

[meship-starkware]

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

---

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

"after those changes (cairo1), it was 760."

what changes?

changing the os resources contract from cairo0 to cairo1

[ilyalesokhin-starkware]

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

changing the os resources contract from cairo0 to cairo1

@Yoni-Starkware
Can you check if it makes sense?

[noaov1]

Reviewed 11 of 11 files at r7, 2 of 3 files at r8, all commit messages.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

We took a safety margin for the rangecheck is this cost is hardcoded in the contracts.

@meship-starkware - can you please add a documentation? (in the struct definition)

---

crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs line 111 at r8 (raw file):

    )
    .unwrap();
    // Note, this it the call info of the constructor call (inner call).

Suggestion:

// Note, this is the call info of the constructor call (inner call

[meship-starkware]

Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

@meship-starkware - can you please add a documentation? (in the struct definition)

Now that I look at the rust GasCost struct, I see that we did not add Bitwise and Pedersen builtins to this struct.
Should I do it as well? What is the purpose of this struct?

[noaov1]

Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Now that I look at the rust GasCost struct, I see that we did not add Bitwise and Pedersen builtins to this struct.
Should I do it as well? What is the purpose of this struct?

Yes. Nice catch!

[noaov1]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)

[meship-starkware]

Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)

---

crates/blockifier/resources/versioned_constants.json line 103 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Yes. Nice catch!

Dcomutation: Done.
Update GasCost struct next PR

[Yoni-Starkware]

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

---

crates/blockifier/resources/versioned_constants.json line 36 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

@Yoni-Starkware
Can you check if it makes sense?

Yes, TAL at the os_resources - https://github.com/starkware-industries/starkware/blob/43ecdaadb5dc20351ec1064792f810825f326012/src/starkware/starknet/definitions/versioned_constants.json#L192

That's your new builtins. The function select_builtins is expensive

[noaov1]

Reviewable status: 14 of 15 files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[noaov1]

Reviewed 1 of 11 files at r7, 1 of 2 files at r9.
Reviewable status: 14 of 15 files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[Yoni-Starkware]

Reviewed 1 of 3 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)