chore(blockifier): change deserialization of versioned_constants #2818

Yonatan-Starkware · 2024-12-19T10:57:35Z

No description provided.

reviewable-StarkWare · 2024-12-19T10:57:42Z

This change is

Yonatan-Starkware · 2024-12-19T10:57:51Z

chore(blockifier): change deserialization of versioned_constants #2818 👈 (View in Graphite)
chore(blockifier): split builtin gas costs from base gas costs #2514 : 1 other dependent PR (#2838 )
chore(blockifier): add syscalls_gas_costs struct #2359 : 1 other dependent PR (#2501 )
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-12-19T10:58:31Z

Artifacts upload workflows:

graphite-app · 2024-12-22T14:07:44Z

crates/blockifier/src/versioned_constants.rs

+    fn set_syscall_price(&self, selector: &SyscallSelector, amount: u64) {
+        match *selector {
+            SyscallSelector::CallContract => self.call_contract = amount,
+            SyscallSelector::Deploy => self.deploy = amount,
+            SyscallSelector::EmitEvent => self.emit_event = amount,
+            SyscallSelector::GetBlockHash => self.get_block_hash = amount,
+            SyscallSelector::GetExecutionInfo => self.get_execution_info = amount,
+            SyscallSelector::GetClassHashAt => self.get_class_hash_at = amount,
+            SyscallSelector::Keccak => {self.keccak = amount;
+                self.keccak_round_cost = 180000},
+            SyscallSelector::Sha256ProcessBlock => self.sha256_process_block = amount,
+            SyscallSelector::LibraryCall => self.library_call = amount,
+            SyscallSelector::ReplaceClass => self.replace_class = amount,
+            SyscallSelector::Secp256k1Add => self.secp256k1_add = amount,
+            SyscallSelector::Secp256k1GetPointFromX => self.secp256k1_get_point_from_x = amount,
+            SyscallSelector::Secp256k1GetXy => self.secp256k1_get_xy = amount,
+            SyscallSelector::Secp256k1Mul => self.secp256k1_mul = amount,
+            SyscallSelector::Secp256k1New => self.secp256k1_new = amount,
+            SyscallSelector::Secp256r1Add => self.secp256r1_add = amount,
+            SyscallSelector::Secp256r1GetPointFromX => self.secp256r1_get_point_from_x = amount,
+            SyscallSelector::Secp256r1GetXy => self.secp256r1_get_xy = amount,
+            SyscallSelector::Secp256r1Mul => self.secp256r1_mul = amount,
+            SyscallSelector::Secp256r1New => self.secp256r1_new = amount,
+            SyscallSelector::SendMessageToL1 => self.send_message_to_l1 = amount,
+            SyscallSelector::StorageRead => self.storage_read = amount,
+            SyscallSelector::StorageWrite => self.storage_write = amount,
+            SyscallSelector::DelegateCall
+            | SyscallSelector::DelegateL1Handler
+            | SyscallSelector::GetBlockNumber
+            | SyscallSelector::GetBlockTimestamp
+            | SyscallSelector::GetCallerAddress
+            | SyscallSelector::GetContractAddress
+            | SyscallSelector::GetTxInfo
+            | SyscallSelector::GetSequencerAddress
+            | SyscallSelector::GetTxSignature
+            | SyscallSelector::LibraryCallL1Handler => {
+                return Err(GasCostsError::DeprecatedSyscall { selector: *selector });
+            }
+        }
+    }


The function signature set_syscall_price needs a return type of Result<(), GasCostsError> to match its implementation, which attempts to return an error in the deprecated syscall case. Currently the function signature indicates it returns nothing (implicit unit type) but the implementation includes an Err return.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-12-22T14:07:46Z

crates/blockifier/src/versioned_constants.rs

+            let constants = Arc::new(OsConstants {
+                gas_costs,
+                validate_rounding_consts: tmp_versioned_constants.tmp_os_constants.validate_rounding_consts,
+                os_contract_addresses: tmp_versioned_constants.tmp_os_constants.os_contract_addresses,
+            });
+
+        } else {
+            let constants = Arc::new(OsConstants {
+                gas_costs: tmp_versioned_constants.tmp_os_constants.gas_costs,
+                validate_rounding_consts: tmp_versioned_constants.tmp_os_constants.validate_rounding_consts,
+                os_contract_addresses: tmp_versioned_constants.tmp_os_constants.os_contract_addresses,
+            });


The constants variable is created but not used in the final VersionedConstants struct construction. Please assign constants to the os_constants field in the Ok(VersionedConstants { ... }) return value.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-12-22T14:07:47Z

crates/blockifier/src/versioned_constants.rs

+    fn is_syscall_gas_costs_none (&self) ->bool {
+        self.gas_costs.is_none()
+    }


The is_syscall_gas_costs_none() method is calling is_none() on the wrong field. It should be self.gas_costs.syscalls.is_none() since syscalls is the Option field that needs to be checked, not gas_costs itself which is a struct.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-12-22T14:08:17Z

crates/blockifier/src/versioned_constants.rs

+    fn set_syscall_price(&self, selector: &SyscallSelector, amount: u64) {
+        match *selector {
+            SyscallSelector::CallContract => self.call_contract = amount,
+            SyscallSelector::Deploy => self.deploy = amount,
+            SyscallSelector::EmitEvent => self.emit_event = amount,
+            SyscallSelector::GetBlockHash => self.get_block_hash = amount,
+            SyscallSelector::GetExecutionInfo => self.get_execution_info = amount,
+            SyscallSelector::GetClassHashAt => self.get_class_hash_at = amount,
+            SyscallSelector::Keccak => {self.keccak = amount;
+                self.keccak_round_cost = 180000},
+            SyscallSelector::Sha256ProcessBlock => self.sha256_process_block = amount,
+            SyscallSelector::LibraryCall => self.library_call = amount,
+            SyscallSelector::ReplaceClass => self.replace_class = amount,
+            SyscallSelector::Secp256k1Add => self.secp256k1_add = amount,
+            SyscallSelector::Secp256k1GetPointFromX => self.secp256k1_get_point_from_x = amount,
+            SyscallSelector::Secp256k1GetXy => self.secp256k1_get_xy = amount,
+            SyscallSelector::Secp256k1Mul => self.secp256k1_mul = amount,
+            SyscallSelector::Secp256k1New => self.secp256k1_new = amount,
+            SyscallSelector::Secp256r1Add => self.secp256r1_add = amount,
+            SyscallSelector::Secp256r1GetPointFromX => self.secp256r1_get_point_from_x = amount,
+            SyscallSelector::Secp256r1GetXy => self.secp256r1_get_xy = amount,
+            SyscallSelector::Secp256r1Mul => self.secp256r1_mul = amount,
+            SyscallSelector::Secp256r1New => self.secp256r1_new = amount,
+            SyscallSelector::SendMessageToL1 => self.send_message_to_l1 = amount,
+            SyscallSelector::StorageRead => self.storage_read = amount,
+            SyscallSelector::StorageWrite => self.storage_write = amount,
+            SyscallSelector::DelegateCall
+            | SyscallSelector::DelegateL1Handler
+            | SyscallSelector::GetBlockNumber
+            | SyscallSelector::GetBlockTimestamp
+            | SyscallSelector::GetCallerAddress
+            | SyscallSelector::GetContractAddress
+            | SyscallSelector::GetTxInfo
+            | SyscallSelector::GetSequencerAddress
+            | SyscallSelector::GetTxSignature
+            | SyscallSelector::LibraryCallL1Handler => {
+                return Err(GasCostsError::DeprecatedSyscall { selector: *selector });
+            }
+        }
+    }


The function signature needs to be modified to:

fn set_syscall_price(&mut self, selector: &SyscallSelector, amount: u64) -> Result<(), GasCostsError>

Two changes are required:

Add mut to allow modification of the struct's fields

Return Result<(), GasCostsError> to properly handle the error case

Additionally, add Ok(()) after the successful assignments in the match statement to return success.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-12-22T14:08:18Z

crates/blockifier/src/versioned_constants.rs

+    fn is_syscall_gas_costs_none (&self) ->bool {
+        self.gas_costs.is_none()
+    }


The method is checking the wrong field - it should be checking self.gas_costs.syscalls.is_none() rather than self.gas_costs.is_none(). The gas_costs field itself is not optional, but its syscalls field is.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Yonatan-Starkware mentioned this pull request Dec 19, 2024

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

Merged

Yonatan-Starkware requested review from meship-starkware and noaov1 December 19, 2024 10:57

Yonatan-Starkware marked this pull request as ready for review December 19, 2024 10:58

Yonatan-Starkware force-pushed the yonatank/blockifier/moving_gas_costs_to_os_resources branch from e46d6c7 to 6edcfc1 Compare December 19, 2024 12:15

Yonatan-Starkware force-pushed the yonatank/blockifier/split_builtin_gas_costs_from_base_gas_costs branch from 1fb5bb8 to cf1721c Compare December 19, 2024 12:18

Yonatan-Starkware force-pushed the yonatank/blockifier/moving_gas_costs_to_os_resources branch from 6edcfc1 to 122d0e3 Compare December 19, 2024 12:18

Yonatan-Starkware force-pushed the yonatank/blockifier/split_builtin_gas_costs_from_base_gas_costs branch from cf1721c to 9b7c19f Compare December 19, 2024 12:40

Yonatan-Starkware force-pushed the yonatank/blockifier/moving_gas_costs_to_os_resources branch from 122d0e3 to 2dac98b Compare December 19, 2024 12:40

Yonatan-Starkware mentioned this pull request Dec 19, 2024

chore(blockifier): impl deserialize for versioned_constants #2838

Merged

Yonatan-Starkware force-pushed the yonatank/blockifier/split_builtin_gas_costs_from_base_gas_costs branch from 9b7c19f to 5214bf3 Compare December 22, 2024 07:56

This was referenced Dec 22, 2024

chore(blockifier): remove fee_transfer_gas_cost #2858

Merged

chore(blockifier): remove syscall_gas_costs from os_constans #2864

Open

Yonatan-Starkware force-pushed the yonatank/blockifier/split_builtin_gas_costs_from_base_gas_costs branch from 5214bf3 to 813f4d2 Compare December 22, 2024 13:39

Yonatan-Starkware changed the base branch from yonatank/blockifier/split_builtin_gas_costs_from_base_gas_costs to graphite-base/2818 December 22, 2024 14:06

Yonatan-Starkware force-pushed the graphite-base/2818 branch from 813f4d2 to 4f26fc6 Compare December 22, 2024 14:06

Yonatan-Starkware force-pushed the yonatank/blockifier/moving_gas_costs_to_os_resources branch from 2dac98b to 9731180 Compare December 22, 2024 14:06

Yonatan-Starkware changed the base branch from graphite-base/2818 to main December 22, 2024 14:07

chore(blockifier): change deserialization of versioned_constants

f5fabe7

Yonatan-Starkware force-pushed the yonatank/blockifier/moving_gas_costs_to_os_resources branch from 9731180 to f5fabe7 Compare December 22, 2024 14:07

graphite-app bot reviewed Dec 22, 2024

View reviewed changes

Yonatan-Starkware closed this Dec 22, 2024

Yonatan-Starkware deleted the yonatank/blockifier/moving_gas_costs_to_os_resources branch December 22, 2024 15:27

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(blockifier): change deserialization of versioned_constants #2818

chore(blockifier): change deserialization of versioned_constants #2818

Yonatan-Starkware commented Dec 19, 2024

reviewable-StarkWare commented Dec 19, 2024

Yonatan-Starkware commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

graphite-app bot Dec 22, 2024

graphite-app bot Dec 22, 2024

graphite-app bot Dec 22, 2024

graphite-app bot Dec 22, 2024

graphite-app bot Dec 22, 2024

chore(blockifier): change deserialization of versioned_constants #2818

chore(blockifier): change deserialization of versioned_constants #2818

Conversation

Yonatan-Starkware commented Dec 19, 2024

reviewable-StarkWare commented Dec 19, 2024

Yonatan-Starkware commented Dec 19, 2024 • edited Loading

github-actions bot commented Dec 19, 2024 • edited Loading

graphite-app bot Dec 22, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 22, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 22, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 22, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 22, 2024

Choose a reason for hiding this comment

Yonatan-Starkware commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading