From 4f066981ebebcc5150b8cce27008fd230656fe3c Mon Sep 17 00:00:00 2001 From: Yoni <78365039+Yoni-Starkware@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:19:10 +0200 Subject: [PATCH] chore(cairo_native): temporarily revert EP gas deduction and align tests (#2214) --- .../execution/native/entry_point_execution.rs | 34 ++++++----- .../syscalls/syscall_tests/call_contract.rs | 14 ++--- .../syscalls/syscall_tests/deploy.rs | 30 +++------- .../syscalls/syscall_tests/emit_event.rs | 8 +-- .../syscalls/syscall_tests/get_block_hash.rs | 11 ++-- .../syscall_tests/get_class_hash_at.rs | 8 +-- .../syscalls/syscall_tests/keccak.rs | 8 +-- .../syscalls/syscall_tests/library_call.rs | 57 +++++++++++-------- .../syscalls/syscall_tests/replace_class.rs | 8 +-- .../execution/syscalls/syscall_tests/secp.rs | 12 ++-- .../syscall_tests/send_message_to_l1.rs | 8 +-- .../syscalls/syscall_tests/sha256.rs | 8 +-- .../syscall_tests/storage_read_write.rs | 8 +-- 13 files changed, 102 insertions(+), 112 deletions(-) diff --git a/crates/blockifier/src/execution/native/entry_point_execution.rs b/crates/blockifier/src/execution/native/entry_point_execution.rs index 5cd0c7289c..6fb3efdd13 100644 --- a/crates/blockifier/src/execution/native/entry_point_execution.rs +++ b/crates/blockifier/src/execution/native/entry_point_execution.rs @@ -1,7 +1,6 @@ use cairo_native::execution_result::ContractExecutionResult; use cairo_native::utils::BuiltinCosts; use cairo_vm::vm::runners::cairo_runner::ExecutionResources; -use num_traits::ToPrimitive; use starknet_api::execution_resources::GasAmount; use crate::execution::call_info::{CallExecution, CallInfo, ChargedResources, Retdata}; @@ -40,10 +39,13 @@ pub fn execute_entry_point_call( mul_mod: gas_costs.mul_mod_gas_cost, }; + // Fund the initial budget since the native executor charges it before the run. + // TODO(Yoni): revert once the VM is aligned with this. + let gas = syscall_handler.call.initial_gas + gas_costs.entry_point_initial_budget; let execution_result = contract_class.executor.run( entry_point.selector.0, &syscall_handler.call.calldata.0.clone(), - Some(syscall_handler.call.initial_gas.into()), + Some(gas), Some(builtin_costs), &mut syscall_handler, ); @@ -61,22 +63,24 @@ fn create_callinfo( call_result: ContractExecutionResult, syscall_handler: NativeSyscallHandler<'_>, ) -> Result { - let remaining_gas = - call_result.remaining_gas.to_u64().ok_or(PostExecutionError::MalformedReturnData { - error_message: format!( - "Unexpected remaining gas. Gas value is bigger than u64: {}", - call_result.remaining_gas - ), - })?; + let mut remaining_gas = call_result.remaining_gas; if remaining_gas > syscall_handler.call.initial_gas { - return Err(PostExecutionError::MalformedReturnData { - error_message: format!( - "Unexpected remaining gas. Used gas is greater than initial gas: {} > {}", - remaining_gas, syscall_handler.call.initial_gas - ), + if remaining_gas - syscall_handler.call.initial_gas + <= syscall_handler.context.gas_costs().entry_point_initial_budget + { + // Revert the refund. + // TODO(Yoni): temporary hack - this is probably a bug. Investigate and fix native. + remaining_gas = syscall_handler.call.initial_gas; + } else { + return Err(PostExecutionError::MalformedReturnData { + error_message: format!( + "Unexpected remaining gas. Used gas is greater than initial gas: {} > {}", + remaining_gas, syscall_handler.call.initial_gas + ), + } + .into()); } - .into()); } let gas_consumed = syscall_handler.call.initial_gas - remaining_gas; diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs index cd34175b7a..5f5138171c 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs @@ -72,22 +72,16 @@ fn test_call_contract_that_panics() { feature = "cairo_native", test_case( FeatureContract::TestContract(CairoVersion::Native), - FeatureContract::TestContract(CairoVersion::Native), - 190370; + FeatureContract::TestContract(CairoVersion::Native); "Call Contract between two contracts using Native" ) )] #[test_case( FeatureContract::TestContract(CairoVersion::Cairo1), - FeatureContract::TestContract(CairoVersion::Cairo1), - REQUIRED_GAS_CALL_CONTRACT_TEST; + FeatureContract::TestContract(CairoVersion::Cairo1); "Call Contract between two contracts using VM" )] -fn test_call_contract( - outer_contract: FeatureContract, - inner_contract: FeatureContract, - expected_gas: u64, -) { +fn test_call_contract(outer_contract: FeatureContract, inner_contract: FeatureContract) { let chain_info = &ChainInfo::create_for_testing(); let mut state = test_state(chain_info, BALANCE, &[(outer_contract, 1), (inner_contract, 1)]); @@ -110,7 +104,7 @@ fn test_call_contract( entry_point_call.execute_directly(&mut state).unwrap().execution, CallExecution { retdata: retdata![felt!(48_u8)], - gas_consumed: expected_gas, + gas_consumed: REQUIRED_GAS_CALL_CONTRACT_TEST, ..CallExecution::default() } ); diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs index 696a30fed5..5b417f0d73 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs @@ -14,12 +14,12 @@ use crate::test_utils::contracts::FeatureContract; use crate::test_utils::initial_test_state::test_state; use crate::test_utils::{calldata_for_deploy_test, trivial_external_entry_point_new, CairoVersion}; -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 205200;"VM")] +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1);"VM")] #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 215200;"Native") + test_case(FeatureContract::TestContract(CairoVersion::Native);"Native") )] -fn no_constructor(deployer_contract: FeatureContract, expected_gas: u64) { +fn no_constructor(deployer_contract: FeatureContract) { // TODO(Yoni): share the init code of the tests in this file. let empty_contract = FeatureContract::Empty(CairoVersion::Cairo1); @@ -41,11 +41,7 @@ fn no_constructor(deployer_contract: FeatureContract, expected_gas: u64) { let deploy_call = &entry_point_call.execute_directly(&mut state).unwrap(); assert_eq!( deploy_call.execution, - CallExecution { - retdata: retdata![], - gas_consumed: expected_gas, - ..CallExecution::default() - } + CallExecution { retdata: retdata![], gas_consumed: 205200, ..CallExecution::default() } ); let deployed_contract_address = calculate_contract_address( @@ -96,16 +92,12 @@ fn no_constructor_nonempty_calldata(deployer_contract: FeatureContract) { )); } -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1),214550, 4610;"VM")] +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1);"VM")] #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native),234550, 14610;"Native") + test_case(FeatureContract::TestContract(CairoVersion::Native);"Native") )] -fn with_constructor( - deployer_contract: FeatureContract, - expected_gas: u64, - expected_constructor_gas: u64, -) { +fn with_constructor(deployer_contract: FeatureContract) { let mut state = test_state(&ChainInfo::create_for_testing(), Fee(0), &[(deployer_contract, 1)]); let class_hash = deployer_contract.get_class_hash(); @@ -134,11 +126,7 @@ fn with_constructor( let deploy_call = &entry_point_call.execute_directly(&mut state).unwrap(); assert_eq!( deploy_call.execution, - CallExecution { - retdata: retdata![], - gas_consumed: expected_gas, - ..CallExecution::default() - } + CallExecution { retdata: retdata![], gas_consumed: 214550, ..CallExecution::default() } ); let constructor_call = &deploy_call.inner_calls[0]; @@ -150,7 +138,7 @@ fn with_constructor( // The test contract constructor returns its first argument. retdata: retdata![constructor_calldata[0]], // This reflects the gas cost of storage write syscall. - gas_consumed: expected_constructor_gas, + gas_consumed: 4610, ..CallExecution::default() } ); diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/emit_event.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/emit_event.rs index 4eb3e599cf..4598c482ad 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/emit_event.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/emit_event.rs @@ -28,10 +28,10 @@ const N_EMITTED_EVENTS: [Felt; 1] = [Felt::from_hex_unchecked("0x1")]; #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 57330; "Native") + test_case(FeatureContract::TestContract(CairoVersion::Native); "Native") )] -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 47330; "VM")] -fn positive_flow(test_contract: FeatureContract, expected_gas: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn positive_flow(test_contract: FeatureContract) { let call_info = emit_events(test_contract, &N_EMITTED_EVENTS, &KEYS, &DATA) .expect("emit_events failed with valued parameters"); let event = EventContent { @@ -43,7 +43,7 @@ fn positive_flow(test_contract: FeatureContract, expected_gas: u64) { call_info.execution, CallExecution { events: vec![OrderedEvent { order: 0, event }], - gas_consumed: expected_gas, + gas_consumed: 47330, ..Default::default() } ); diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/get_block_hash.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/get_block_hash.rs index 4c044c530c..0e273e7527 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/get_block_hash.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/get_block_hash.rs @@ -42,10 +42,10 @@ fn initialize_state(test_contract: FeatureContract) -> (CachedState { - "(0x7820213d2079 ('x != y'), 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED'))" + // 'x != y', 'ENTRYPOINT_FAILED'. + vec![felt!("0x7820213d2079"), felt!("0x454e545259504f494e545f4641494c4544")] } #[cfg(feature = "cairo_native")] - CairoVersion::Native => "0x7820213d2079 ('x != y')", + // 'x != y'. + CairoVersion::Native => vec![felt!("0x7820213d2079")], }; - assert_eq!(format_panic_data(&call_info.execution.retdata.0), expected_err); + + assert_eq!( + call_info.execution, + CallExecution { + retdata: Retdata(expected_err_retdata), + gas_consumed: 150980, + failed: true, + ..Default::default() + } + ); } #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 515110; "Native") + test_case(FeatureContract::TestContract(CairoVersion::Native); "Native") )] -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 475110; "VM")] -fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn test_nested_library_call(test_contract: FeatureContract) { // Todo(pwhite) 2024/10/28: Execution resources from the VM & Native are mesaured differently // helper function to change the expected resource values from both of executions // When gas is changed to be the same between VM and Native this should be removed. @@ -148,7 +155,7 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { entry_point_selector: selector_from_name("test_nested_library_call"), calldata: main_entry_point_calldata, class_hash: Some(test_class_hash), - initial_gas: if_native(&test_contract)(9999584180, 9999292440), + initial_gas: 9999292440, ..trivial_external_entry_point_new(test_contract) }; let nested_storage_entry_point = CallEntryPoint { @@ -157,7 +164,7 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { class_hash: Some(test_class_hash), code_address: None, call_type: CallType::Delegate, - initial_gas: if_native(&test_contract)(9999257700, 9998985960), + initial_gas: 9998985960, ..trivial_external_entry_point_new(test_contract) }; let library_entry_point = CallEntryPoint { @@ -172,12 +179,12 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { class_hash: Some(test_class_hash), code_address: None, call_type: CallType::Delegate, - initial_gas: if_native(&test_contract)(9999418680, 9999136940), + initial_gas: 9999136940, ..trivial_external_entry_point_new(test_contract) }; let storage_entry_point = CallEntryPoint { calldata: calldata![felt!(key), felt!(value)], - initial_gas: if_native(&test_contract)(9999096060, 9998834320), + initial_gas: 9998834320, ..nested_storage_entry_point }; @@ -204,7 +211,7 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { call: nested_storage_entry_point, execution: CallExecution { retdata: retdata![felt!(value + 1)], - gas_consumed: if_native(&test_contract)(26990, REQUIRED_GAS_STORAGE_READ_WRITE_TEST), + gas_consumed: REQUIRED_GAS_STORAGE_READ_WRITE_TEST, ..CallExecution::default() }, charged_resources: first_storage_entry_point_resources, @@ -230,7 +237,7 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { call: library_entry_point, execution: CallExecution { retdata: retdata![felt!(value + 1)], - gas_consumed: if_native(&test_contract)(187970, REQUIRED_GAS_LIBRARY_CALL_TEST), + gas_consumed: REQUIRED_GAS_LIBRARY_CALL_TEST, ..CallExecution::default() }, charged_resources: library_call_resources, @@ -243,7 +250,7 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { call: storage_entry_point, execution: CallExecution { retdata: retdata![felt!(value)], - gas_consumed: if_native(&test_contract)(26990, REQUIRED_GAS_STORAGE_READ_WRITE_TEST), + gas_consumed: REQUIRED_GAS_STORAGE_READ_WRITE_TEST, ..CallExecution::default() }, charged_resources: storage_entry_point_resources, @@ -269,7 +276,7 @@ fn test_nested_library_call(test_contract: FeatureContract, expected_gas: u64) { call: main_entry_point.clone(), execution: CallExecution { retdata: retdata![felt!(value)], - gas_consumed: expected_gas, + gas_consumed: 475110, ..CallExecution::default() }, charged_resources: main_call_resources, diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/replace_class.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/replace_class.rs index 485858642f..75ca53dbc2 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/replace_class.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/replace_class.rs @@ -57,10 +57,10 @@ fn cairo0_class_hash(test_contract: FeatureContract) { #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 15220; "Native") + test_case(FeatureContract::TestContract(CairoVersion::Native); "Native") )] -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 5220; "VM")] -fn positive_flow(test_contract: FeatureContract, gas_consumed: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn positive_flow(test_contract: FeatureContract) { let empty_contract = FeatureContract::Empty(CairoVersion::Cairo1); let empty_contract_cairo0 = FeatureContract::Empty(CairoVersion::Cairo0); let mut state = test_state( @@ -80,7 +80,7 @@ fn positive_flow(test_contract: FeatureContract, gas_consumed: u64) { }; assert_eq!( entry_point_call.execute_directly(&mut state).unwrap().execution, - CallExecution { gas_consumed, ..Default::default() } + CallExecution { gas_consumed: 5220, ..Default::default() } ); assert_eq!(state.get_class_hash_at(contract_address).unwrap(), new_class_hash); } diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs index 3a8d52f098..552843bbf1 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs @@ -9,8 +9,8 @@ use crate::test_utils::contracts::FeatureContract; use crate::test_utils::initial_test_state::test_state; use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, BALANCE}; -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 17034156; "VM")] -fn test_secp256k1(test_contract: FeatureContract, expected_gas: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn test_secp256k1(test_contract: FeatureContract) { let chain_info = &ChainInfo::create_for_testing(); let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]); @@ -23,12 +23,12 @@ fn test_secp256k1(test_contract: FeatureContract, expected_gas: u64) { pretty_assertions::assert_eq!( entry_point_call.execute_directly(&mut state).unwrap().execution, - CallExecution { gas_consumed: expected_gas, ..Default::default() } + CallExecution { gas_consumed: 17034156, ..Default::default() } ); } -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 27563600; "VM")] -fn test_secp256r1(test_contract: FeatureContract, expected_gas: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn test_secp256r1(test_contract: FeatureContract) { let chain_info = &ChainInfo::create_for_testing(); let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]); @@ -41,6 +41,6 @@ fn test_secp256r1(test_contract: FeatureContract, expected_gas: u64) { pretty_assertions::assert_eq!( entry_point_call.execute_directly(&mut state).unwrap().execution, - CallExecution { gas_consumed: expected_gas, ..Default::default() } + CallExecution { gas_consumed: 27563600, ..Default::default() } ); } diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/send_message_to_l1.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/send_message_to_l1.rs index b5ef29e404..5528c6f6ed 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/send_message_to_l1.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/send_message_to_l1.rs @@ -15,10 +15,10 @@ use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, BALANCE} #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 30960; "Native") + test_case(FeatureContract::TestContract(CairoVersion::Native); "Native") )] -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 20960; "VM")] -fn test_send_message_to_l1(test_contract: FeatureContract, expected_gas: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn test_send_message_to_l1(test_contract: FeatureContract) { let chain_info = &ChainInfo::create_for_testing(); let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]); @@ -49,7 +49,7 @@ fn test_send_message_to_l1(test_contract: FeatureContract, expected_gas: u64) { entry_point_call.execute_directly(&mut state).unwrap().execution, CallExecution { l2_to_l1_messages: vec![OrderedL2ToL1Message { order: 0, message }], - gas_consumed: expected_gas, + gas_consumed: 20960, ..Default::default() } ); diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/sha256.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/sha256.rs index b42a278da8..7497720078 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/sha256.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/sha256.rs @@ -12,10 +12,10 @@ use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, BALANCE} #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 891425; "Native") + test_case(FeatureContract::TestContract(CairoVersion::Native); "Native") )] -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), 881425; "VM")] -fn test_sha256(test_contract: FeatureContract, gas_consumed: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn test_sha256(test_contract: FeatureContract) { let chain_info = &ChainInfo::create_for_testing(); let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]); @@ -28,6 +28,6 @@ fn test_sha256(test_contract: FeatureContract, gas_consumed: u64) { pretty_assertions::assert_eq!( entry_point_call.execute_directly(&mut state).unwrap().execution, - CallExecution { gas_consumed, ..CallExecution::from_retdata(retdata![]) } + CallExecution { gas_consumed: 881425, ..CallExecution::from_retdata(retdata![]) } ); } diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/storage_read_write.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/storage_read_write.rs index 557c58416d..198ed7fa40 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/storage_read_write.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/storage_read_write.rs @@ -15,10 +15,10 @@ use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, BALANCE} #[cfg_attr( feature = "cairo_native", - test_case(FeatureContract::TestContract(CairoVersion::Native), 26990; "Native") + test_case(FeatureContract::TestContract(CairoVersion::Native); "Native") )] -#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), REQUIRED_GAS_STORAGE_READ_WRITE_TEST; "VM")] -fn test_storage_read_write(test_contract: FeatureContract, expected_gas: u64) { +#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")] +fn test_storage_read_write(test_contract: FeatureContract) { let chain_info = &ChainInfo::create_for_testing(); let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]); @@ -35,7 +35,7 @@ fn test_storage_read_write(test_contract: FeatureContract, expected_gas: u64) { entry_point_call.execute_directly(&mut state).unwrap().execution, CallExecution { retdata: retdata![value], - gas_consumed: expected_gas, + gas_consumed: REQUIRED_GAS_STORAGE_READ_WRITE_TEST, ..CallExecution::default() } );