Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(cairo_native): temporarily revert EP gas deduction and align tests #2214

Merged
merged 1 commit into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 19 additions & 15 deletions crates/blockifier/src/execution/native/entry_point_execution.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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,
);
Expand All @@ -61,22 +63,24 @@ fn create_callinfo(
call_result: ContractExecutionResult,
syscall_handler: NativeSyscallHandler<'_>,
) -> Result<CallInfo, EntryPointExecutionError> {
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)]);

Expand All @@ -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()
}
);
Expand Down
30 changes: 9 additions & 21 deletions crates/blockifier/src/execution/syscalls/syscall_tests/deploy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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];
Expand All @@ -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()
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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()
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ fn initialize_state(test_contract: FeatureContract) -> (CachedState<DictStateRea

#[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, expected_gas: u64) {
#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")]
fn positive_flow(test_contract: FeatureContract) {
let (mut state, block_number, block_hash) = initialize_state(test_contract);

let calldata = calldata![block_number];
Expand All @@ -57,10 +57,7 @@ fn positive_flow(test_contract: FeatureContract, expected_gas: u64) {

assert_eq!(
entry_point_call.clone().execute_directly(&mut state).unwrap().execution,
CallExecution {
gas_consumed: expected_gas,
..CallExecution::from_retdata(retdata![block_hash])
}
CallExecution { gas_consumed: 5220, ..CallExecution::from_retdata(retdata![block_hash]) }
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ use crate::test_utils::{trivial_external_entry_point_new, CairoVersion, BALANCE}
/// 3. Execution succeeds with expected gas for valid cases.
/// 4. Execution fails if `address` has a different `class_hash`.
/// 5. Execution succeeds and returns `class_hash` = 0 if `address` is absent.
#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1), REQUIRED_GAS_GET_CLASS_HASH_AT_TEST; "VM")]
#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")]
#[cfg_attr(
feature = "cairo_native",
test_case(FeatureContract::TestContract(CairoVersion::Native), 17830; "Native"))
test_case(FeatureContract::TestContract(CairoVersion::Native); "Native"))
]
fn test_get_class_hash_at(test_contract: FeatureContract, expected_gas: u64) {
fn test_get_class_hash_at(test_contract: FeatureContract) {
let chain_info = &ChainInfo::create_for_testing();
let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]);
let address = contract_address!("0x111");
Expand All @@ -42,7 +42,7 @@ fn test_get_class_hash_at(test_contract: FeatureContract, expected_gas: u64) {
positive_call_info.execution,
CallExecution {
retdata: retdata!(),
gas_consumed: expected_gas,
gas_consumed: REQUIRED_GAS_GET_CLASS_HASH_AT_TEST,
failed: false,
..CallExecution::default()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ 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), 254910; "VM")]
#[test_case(FeatureContract::TestContract(CairoVersion::Cairo1); "VM")]
#[cfg_attr(
feature = "cairo_native",
test_case(FeatureContract::TestContract(CairoVersion::Native), 264910; "Native")
test_case(FeatureContract::TestContract(CairoVersion::Native); "Native")
)]
fn test_keccak(test_contract: FeatureContract, expected_gas: u64) {
fn test_keccak(test_contract: FeatureContract) {
let chain_info = &ChainInfo::create_for_testing();
let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]);

Expand All @@ -28,6 +28,6 @@ fn test_keccak(test_contract: FeatureContract, expected_gas: u64) {

pretty_assertions::assert_eq!(
entry_point_call.execute_directly(&mut state).unwrap().execution,
CallExecution { gas_consumed: expected_gas, ..CallExecution::from_retdata(retdata![]) }
CallExecution { gas_consumed: 254910, ..CallExecution::from_retdata(retdata![]) }
);
}
Loading
Loading