chore(blockifier): add syscalls_gas_costs struct #2359

Yonatan-Starkware · 2024-11-28T16:19:48Z

No description provided.

reviewable-StarkWare · 2024-11-28T16:19:55Z

This change is

Yonatan-Starkware · 2024-11-28T16:19:56Z

chore(blockifier): add syscalls_gas_costs struct #2359 : 2 dependent PRs (#2501 , #2514 ) 👈 (View in Graphite)
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 · 2024-11-28T16:21:37Z

Yoni-Starkware

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

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

#[derive(Debug, Default, Deserialize)]
pub struct SyscallsGasCosts {

Suggestion:

SyscallGasCosts

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

    // OS gas costs.
    pub transaction_gas_cost: u64,
    pub syscalls_gas_costs: SyscallsGasCosts,

Suggestion:

syscall_gas_costs

crates/blockifier/resources/versioned_constants_0_13_4.json line 120 at r1 (raw file):

        },
        "validated": "VALID",
        "syscalls_gas_costs": {

We are going to remove these fields from the VC eventually, right?
so better to wait with this PR

Code quote:

        "syscalls_gas_costs": {

Yoni-Starkware

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

crates/blockifier/resources/versioned_constants_0_13_4.json line 121 at r2 (raw file):

        "validated": "VALID",
        "syscalls_gas_costs": {
            "call_contract_gas_cost": {

Now you can remove the _gas_cost suffix

Suggestion:

"call_contract"

Yoni-Starkware

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

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

#[derive(Debug, Default, Deserialize)]
pub struct SyscallsGasCosts {
    pub call_contract_gas_cost: u64,

Now you can remove this suffix

Suggestion:

pub call_contract: u64,

Yoni-Starkware

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

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

            BuiltinName::mul_mod => self.base_gas_costs.mul_mod_gas_cost,
            BuiltinName::segment_arena => return Err(GasCostsError::VirtualBuiltin),
            BuiltinName::output | BuiltinName::ecdsa => {

You should have a price for this builtin as well now

Code quote:

 BuiltinName::ecdsa

Yoni-Starkware

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

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

        }        
    }
}

Please remove this impl. This is only used in the parsing phase, so just do it there

Code quote:

impl BaseGasCosts {
    fn get(&self, key: &str) -> u64 {
        match key {
            "step_gas_cost" => self.step_gas_cost,
            "memory_hole_gas_cost" => self.memory_hole_gas_cost,
            "range_check_gas_cost" => self.range_check_gas_cost,
            "keccak_builtin_gas_cost" => self.keccak_builtin_gas_cost,
            "pedersen_gas_cost" => self.pedersen_gas_cost,
            "bitwise_builtin_gas_cost" => self.bitwise_builtin_gas_cost,
            "ecop_gas_cost" => self.ecop_gas_cost,
            "poseidon_gas_cost" => self.poseidon_gas_cost,
            "add_mod_gas_cost" => self.add_mod_gas_cost,
            "mul_mod_gas_cost" => self.mul_mod_gas_cost,
            "default_initial_gas_cost" => self.default_initial_gas_cost,
            "entry_point_initial_budget" => self.entry_point_initial_budget,
            "syscall_base_gas_cost" => self.syscall_base_gas_cost,
            "transaction_gas_cost" => self.transaction_gas_cost,
            _ => panic! ("unvalid key"),
        }
    }
}

meship-starkware

Reviewed 1 of 3 files at r1, 10 of 13 files at r3.
Reviewable status: 10 of 13 files reviewed, 9 unresolved discussions (waiting on @avi-starkware, @Yonatan-Starkware, and @Yoni-Starkware)

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

Previously, Yoni-Starkware (Yoni) wrote…

You should have a price for this builtin as well now

The price should be 10561; it is not part of the VC for now. Should we add a PR like the keccak one to it?

crates/blockifier/resources/versioned_constants_0_13_4.json line 98 at r2 (raw file):

        "poseidon_gas_cost": 491,
        "add_mod_gas_cost": 230,
        "mul_mod_gas_cost": 604,

It does not have to be on this PR, but please extract the builtins to a sub-category as well

Suggestion:

        Builtins_l2_gas_cost: {
        "range_check": 70,
        "keccak_builtin": 136189,
        "pedersen": 4050,
        "bitwise_builtin": 583,
        "ecop": 4085,
        "poseidon": 491,
        "add_mod": 230,
        "mul_mod": 604,
    }

crates/blockifier/resources/versioned_constants_0_13_4.json line 120 at r2 (raw file):

        },
        "validated": "VALID",
        "syscalls_gas_costs": {

That would make Tzahi happy

Suggestion:

"syscalls_l2_gas_costs

crates/blockifier/src/versioned_constants_test.rs line 201 at r2 (raw file):

            "step_gas_cost": 3
        },
        "transaction_gas_cost": {

Can you add one nestes syscall to this test?

Yonatan-Starkware · 2024-12-01T13:44:17Z

crates/blockifier/resources/versioned_constants_0_13_4.json line 98 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It does not have to be on this PR, but please extract the builtins to a sub-category as well

next PR, this one is already big enough

Yoni-Starkware

Reviewed 4 of 7 files at r5.
Reviewable status: 11 of 19 files reviewed, 10 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yonatan-Starkware)

crates/blockifier/resources/versioned_constants_0_13_1_1.json line 651 at r6 (raw file):

        ]
    }
}

Revert this on all files

Yoni-Starkware

Reviewable status: 11 of 19 files reviewed, 9 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @Yonatan-Starkware)

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

Previously, meship-starkware (Meshi Peled) wrote…

The price should be 10561; it is not part of the VC for now. Should we add a PR like the keccak one to it?

Yes

crates/blockifier/src/versioned_constants.rs line 667 at r6 (raw file):

pub struct GasCosts {
    pub base_gas_costs: BaseGasCosts, 
    pub syscall_gas_costs: SyscallGasCosts,

Suggestion:

    pub base: BaseGasCosts,
    pub syscalls: SyscallGasCosts,

meship-starkware

Reviewed 1 of 13 files at r3, 7 of 11 files at r4, 6 of 7 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), 9 unresolved discussions (waiting on @avi-starkware and @Yonatan-Starkware)

meship-starkware

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

Yonatan-Starkware · 2024-12-05T15:17:14Z

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

Previously, Yoni-Starkware (Yoni) wrote…

Yes

should the price be 0 in the old jsons? or the above?

meship-starkware

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

Yoni-Starkware

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

Yoni-Starkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

Yonatan-Starkware · 2024-12-08T11:25:02Z

crates/blockifier/resources/versioned_constants_0_13_4.json line 120 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

We are going to remove these fields from the VC eventually, right?
so better to wait with this PR

Done.

Yonatan-Starkware

Reviewable status: 11 of 21 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @meship-starkware, and @Yoni-Starkware)

crates/blockifier/src/versioned_constants_test.rs line 141 at r9 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Is this necessary?

Done.

crates/blockifier/src/versioned_constants.rs line 755 at r9 (raw file):

            syscalls : syscalls,
        };
        Ok(gas_costs)

Done.

Yoni-Starkware

Reviewed 6 of 10 files at r15, 4 of 4 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

meship-starkware

Reviewed 3 of 4 files at r14, 6 of 10 files at r15, 4 of 4 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

meship-starkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

meship-starkware

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

This was referenced Nov 28, 2024

chore(blockifier): add keccak_builtin_gas_cost #2327

Merged

feat(blockifier): add impl for GasCosts in versioned_constants #2067

Merged

Yonatan-Starkware mentioned this pull request Nov 28, 2024

feat(blockifier): add get_syscall_gas_cost to versioned constants impl #2188

Merged

Yonatan-Starkware mentioned this pull request Nov 28, 2024

chore(blockifier): replace entry_point_gas_cost with initial_budget #2247

Merged

Yonatan-Starkware requested review from avi-starkware, meship-starkware and Yoni-Starkware November 28, 2024 16:20

Yonatan-Starkware marked this pull request as ready for review November 28, 2024 16:20

Yoni-Starkware requested changes Dec 1, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_keccak_builtin_gas_cost branch from 8537aae to e574b59 Compare December 1, 2024 09:19

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from 7ab50eb to 2f6b4f1 Compare December 1, 2024 09:19

Yoni-Starkware requested changes Dec 1, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from 2f6b4f1 to 20c662c Compare December 1, 2024 12:03

meship-starkware requested changes Dec 1, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_keccak_builtin_gas_cost branch from e574b59 to 33c6748 Compare December 1, 2024 12:51

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch 3 times, most recently from 617ea6d to ddb025c Compare December 1, 2024 13:43

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from ddb025c to bb04196 Compare December 1, 2024 13:59

Yoni-Starkware requested changes Dec 1, 2024

View reviewed changes

meship-starkware reviewed Dec 1, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from bb04196 to 760b8b7 Compare December 1, 2024 15:58

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from ad4e1f9 to a3d5e5a Compare December 5, 2024 10:03

Yonatan-Starkware force-pushed the yonatank/blockifier/add_keccak_builtin_gas_cost branch from 18c0d49 to cafa73f Compare December 5, 2024 11:13

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from a3d5e5a to d4e2e1f Compare December 5, 2024 11:46

meship-starkware approved these changes Dec 5, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_keccak_builtin_gas_cost branch from cafa73f to 8d7c3a5 Compare December 5, 2024 15:12

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from d4e2e1f to 9d8115f Compare December 5, 2024 15:12

Yonatan-Starkware mentioned this pull request Dec 5, 2024

chore(blockifier): add ecdsa_gas_cost #2501

Closed

Yonatan-Starkware changed the base branch from yonatank/blockifier/add_keccak_builtin_gas_cost to graphite-base/2359 December 5, 2024 16:15

Yonatan-Starkware force-pushed the graphite-base/2359 branch from 8d7c3a5 to 882df1b Compare December 5, 2024 16:15

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from 9d8115f to 44a4a6f Compare December 5, 2024 16:16

Yonatan-Starkware changed the base branch from graphite-base/2359 to main December 5, 2024 16:16

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from 44a4a6f to 82b0083 Compare December 5, 2024 16:16

meship-starkware approved these changes Dec 5, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from 82b0083 to cc35539 Compare December 8, 2024 08:45

Yonatan-Starkware mentioned this pull request Dec 8, 2024

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

Merged

Yoni-Starkware approved these changes Dec 8, 2024

View reviewed changes

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch 2 times, most recently from a4e9622 to b6948ba Compare December 8, 2024 09:56

chore(blockifier): add syscalls_gas_costs struct

347b59e

Yonatan-Starkware force-pushed the yonatank/blockifier/add_syscalls_gas_costs_struct branch from b6948ba to 347b59e Compare December 8, 2024 10:28

Yonatan-Starkware commented Dec 8, 2024

View reviewed changes

Yoni-Starkware approved these changes Dec 8, 2024

View reviewed changes

meship-starkware approved these changes Dec 8, 2024

View reviewed changes

Yonatan-Starkware merged commit a0dd2fb into main Dec 8, 2024
13 checks passed

github-actions bot locked and limited conversation to collaborators Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(blockifier): add syscalls_gas_costs struct #2359

chore(blockifier): add syscalls_gas_costs struct #2359

Yonatan-Starkware commented Nov 28, 2024

reviewable-StarkWare commented Nov 28, 2024

Yonatan-Starkware commented Nov 28, 2024 •

edited

Loading

github-actions bot commented Nov 28, 2024 •

edited

Loading

Yoni-Starkware left a comment

Yoni-Starkware left a comment

Yoni-Starkware left a comment

Yoni-Starkware left a comment

Yoni-Starkware left a comment

meship-starkware left a comment

Yonatan-Starkware commented Dec 1, 2024

Yoni-Starkware left a comment

Yoni-Starkware left a comment

meship-starkware left a comment

meship-starkware left a comment

Yonatan-Starkware commented Dec 5, 2024

meship-starkware left a comment

Yoni-Starkware left a comment

Yoni-Starkware left a comment

Yonatan-Starkware commented Dec 8, 2024

Yonatan-Starkware left a comment

Yoni-Starkware left a comment

meship-starkware left a comment

meship-starkware left a comment

meship-starkware left a comment

chore(blockifier): add syscalls_gas_costs struct #2359

chore(blockifier): add syscalls_gas_costs struct #2359

Conversation

Yonatan-Starkware commented Nov 28, 2024

reviewable-StarkWare commented Nov 28, 2024

Yonatan-Starkware commented Nov 28, 2024 • edited Loading

github-actions bot commented Nov 28, 2024 • edited Loading

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

Yonatan-Starkware commented Dec 1, 2024

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

Yonatan-Starkware commented Dec 5, 2024

meship-starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

Yonatan-Starkware commented Dec 8, 2024

Yonatan-Starkware left a comment

Choose a reason for hiding this comment

Yoni-Starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

meship-starkware left a comment

Choose a reason for hiding this comment

Yonatan-Starkware commented Nov 28, 2024 •

edited

Loading

github-actions bot commented Nov 28, 2024 •

edited

Loading