From a7493d10716bccdb7acc05274684f2f9e25ee5cf Mon Sep 17 00:00:00 2001 From: fmoletta <99273364+fmoletta@users.noreply.github.com> Date: Tue, 5 Sep 2023 21:29:49 +0300 Subject: [PATCH] fix: Read before writing when executing the deprecated version of `storate_write` syscall (#1006) * Read before writing * Add test * Fix some tests * Fix test * Fix test * Fix test * Fix test * Fix tests * Fix tests * Fix tests --- ...precated_business_logic_syscall_handler.rs | 36 ++++++++++++-- src/transaction/l1_handler.rs | 2 +- tests/complex_contracts/amm_contracts/amm.rs | 12 ++++- .../amm_contracts/amm_proxy.rs | 18 +++++-- tests/complex_contracts/nft/erc721.rs | 26 ++++++---- tests/increase_balance.rs | 2 +- tests/internals.rs | 48 ++++++++++++------- tests/storage.rs | 2 +- tests/syscalls.rs | 13 ++--- 9 files changed, 115 insertions(+), 44 deletions(-) diff --git a/src/syscalls/deprecated_business_logic_syscall_handler.rs b/src/syscalls/deprecated_business_logic_syscall_handler.rs index 1de042db5..6f8108de4 100644 --- a/src/syscalls/deprecated_business_logic_syscall_handler.rs +++ b/src/syscalls/deprecated_business_logic_syscall_handler.rs @@ -872,8 +872,9 @@ impl<'a, S: StateReader> DeprecatedBLSyscallHandler<'a, S> { address: Address, value: Felt252, ) -> Result<(), SyscallHandlerError> { - self.starknet_storage_state - .write(&felt_to_hash(&address.0), value); + let address = felt_to_hash(&address.0); + self.starknet_storage_state.read(&address)?; + self.starknet_storage_state.write(&address, value); Ok(()) } @@ -978,7 +979,7 @@ mod tests { state::cached_state::CachedState, state::in_memory_state_reader::InMemoryStateReader, syscalls::syscall_handler_errors::SyscallHandlerError, - utils::{test_utils::*, Address}, + utils::{felt_to_hash, test_utils::*, Address}, }; use cairo_vm::felt::Felt252; use cairo_vm::hint_processor::hint_processor_definition::HintProcessorLogic; @@ -993,8 +994,8 @@ mod tests { }, vm::{errors::memory_errors::MemoryError, vm_core::VirtualMachine}, }; - use num_traits::Zero; - use std::{any::Any, borrow::Cow, collections::HashMap}; + use num_traits::{One, Zero}; + use std::{any::Any, borrow::Cow, collections::HashMap, sync::Arc}; type DeprecatedBLSyscallHandler<'a> = super::DeprecatedBLSyscallHandler<'a, InMemoryStateReader>; @@ -1111,4 +1112,29 @@ mod tests { Ok(value) if value == Felt252::zero() ); } + + #[test] + fn test_storage_write_update_initial_values() { + // Initialize state reader with value + let mut state_reader = InMemoryStateReader::default(); + state_reader.address_to_storage.insert( + (Address(Felt252::one()), felt_to_hash(&Felt252::one())), + Felt252::zero(), + ); + // Create empty-cached state + let mut state = CachedState::new(Arc::new(state_reader), HashMap::new()); + let mut syscall_handler = DeprecatedBLSyscallHandler::default_with(&mut state); + // Perform write + assert!(syscall_handler + .syscall_storage_write(Address(Felt252::one()), Felt252::one()) + .is_ok()); + // Check that initial values have beed updated in the cache + assert_eq!( + state.cache().storage_initial_values, + HashMap::from([( + (Address(Felt252::one()), felt_to_hash(&Felt252::one())), + Felt252::zero() + )]) + ) + } } diff --git a/src/transaction/l1_handler.rs b/src/transaction/l1_handler.rs index ea4bb95a8..d08d8ccf5 100644 --- a/src/transaction/l1_handler.rs +++ b/src/transaction/l1_handler.rs @@ -323,7 +323,7 @@ mod test { }, events: vec![], l2_to_l1_messages: vec![], - storage_read_values: vec![0.into()], + storage_read_values: vec![0.into(), 0.into()], accessed_storage_keys: HashSet::from([[ 4, 40, 11, 247, 0, 35, 63, 18, 141, 159, 101, 81, 182, 2, 213, 216, 100, 110, 5, 5, 101, 122, 13, 252, 204, 72, 77, 8, 58, 226, 194, 24, diff --git a/tests/complex_contracts/amm_contracts/amm.rs b/tests/complex_contracts/amm_contracts/amm.rs index 6f987ed64..a750eb3f8 100644 --- a/tests/complex_contracts/amm_contracts/amm.rs +++ b/tests/complex_contracts/amm_contracts/amm.rs @@ -97,6 +97,7 @@ fn amm_init_pool_test() { }, class_hash: Some(class_hash), accessed_storage_keys, + storage_read_values: vec![Felt252::zero(), Felt252::zero()], ..Default::default() }; @@ -182,7 +183,12 @@ fn amm_add_demo_tokens_test() { }, class_hash: Some(class_hash), accessed_storage_keys: accessed_storage_keys_add_demo_token, - storage_read_values: vec![Felt252::zero(), Felt252::zero()], + storage_read_values: vec![ + Felt252::zero(), + Felt252::zero(), + Felt252::zero(), + Felt252::zero(), + ], ..Default::default() }; @@ -351,6 +357,10 @@ fn amm_swap_test() { 10000.into(), 100.into(), 100.into(), + 10000.into(), + 100.into(), + 100.into(), + 10000.into(), ] .to_vec(), ..Default::default() diff --git a/tests/complex_contracts/amm_contracts/amm_proxy.rs b/tests/complex_contracts/amm_contracts/amm_proxy.rs index ed6834e09..b4196d833 100644 --- a/tests/complex_contracts/amm_contracts/amm_proxy.rs +++ b/tests/complex_contracts/amm_contracts/amm_proxy.rs @@ -1,6 +1,7 @@ use crate::complex_contracts::utils::*; use cairo_vm::felt::Felt252; use cairo_vm::vm::runners::cairo_runner::ExecutionResources; +use num_traits::Zero; use starknet_crypto::FieldElement; use starknet_in_rust::definitions::block_context::BlockContext; use starknet_in_rust::services::api::contract_classes::deprecated_contract_class::ContractClass; @@ -84,6 +85,7 @@ fn amm_proxy_init_pool_test() { }, class_hash: Some(contract_class_hash), accessed_storage_keys, + storage_read_values: vec![Felt252::zero(), Felt252::zero()], ..Default::default() }]; @@ -288,7 +290,7 @@ fn amm_proxy_add_demo_token_test() { entry_point_selector: Some(amm_entrypoint_selector), entry_point_type: Some(EntryPointType::External), calldata: calldata.clone()[1..].to_vec(), - storage_read_values: vec![0.into(), 0.into()], + storage_read_values: vec![0.into(), 0.into(), 0.into(), 0.into()], execution_resources: ExecutionResources { n_steps: 397, n_memory_holes: 42, @@ -539,8 +541,18 @@ fn amm_proxy_swap() { entry_point_type: Some(EntryPointType::External), calldata: calldata.clone()[1..].to_vec(), retdata: [90.into()].to_vec(), - storage_read_values: [100.into(), 1000.into(), 1000.into(), 100.into(), 200.into()] - .to_vec(), + storage_read_values: [ + 100.into(), + 1000.into(), + 1000.into(), + 100.into(), + 100.into(), + 1000.into(), + 200.into(), + 200.into(), + 1000.into(), + ] + .to_vec(), execution_resources: ExecutionResources { n_steps: 826, n_memory_holes: 92, diff --git a/tests/complex_contracts/nft/erc721.rs b/tests/complex_contracts/nft/erc721.rs index 417eb18cd..0961d9ff8 100644 --- a/tests/complex_contracts/nft/erc721.rs +++ b/tests/complex_contracts/nft/erc721.rs @@ -474,7 +474,7 @@ fn erc721_test_approve() { let expected_read_result = vec![]; // Checks only the storage variable ERC721_operator_approvals - let storage_read_values = vec![Felt252::from(666), Felt252::from(666)]; + let storage_read_values = vec![Felt252::from(666), Felt252::zero(), Felt252::from(666)]; // Ask for the owner of the token let mut accessed_storage_keys = @@ -577,7 +577,7 @@ fn erc721_set_approval_for_all() { let expected_read_result = vec![]; // Only writes in operator_approvals - let storage_read_values = vec![]; + let storage_read_values = vec![Felt252::zero()]; // Writes to the operator the new set value let accessed_storage_keys = get_accessed_keys( @@ -696,14 +696,20 @@ fn erc721_transfer_from_test() { accessed_storage_keys.insert(balance_to); let expected_read_values = vec![ - Felt252::from(666), - Felt252::from(666), - Felt252::from(666), - Felt252::from(666), - Felt252::from(1), - Felt252::zero(), - Felt252::zero(), - Felt252::zero(), + 666.into(), + 666.into(), + 666.into(), + 0.into(), + 666.into(), + 1.into(), + 0.into(), + 1.into(), + 0.into(), + 0.into(), + 0.into(), + 0.into(), + 0.into(), + 666.into(), ]; let approval_event_hash = Felt252::from_bytes_be(&calculate_sn_keccak("Approval".as_bytes())); diff --git a/tests/increase_balance.rs b/tests/increase_balance.rs index 141583e7b..678182e5d 100644 --- a/tests/increase_balance.rs +++ b/tests/increase_balance.rs @@ -112,7 +112,7 @@ fn hello_starknet_increase_balance() { let mut expected_accessed_storage_keys = HashSet::new(); expected_accessed_storage_keys.insert(expected_key); - let expected_storage_read_values = vec![Felt252::zero()]; + let expected_storage_read_values = vec![Felt252::zero(), Felt252::zero()]; let expected_call_info = CallInfo { caller_address: Address(0.into()), diff --git a/tests/internals.rs b/tests/internals.rs index d089ac2c1..7d6d6ab92 100644 --- a/tests/internals.rs +++ b/tests/internals.rs @@ -469,6 +469,10 @@ fn expected_fee_transfer_call_info( storage_read_values: vec![ INITIAL_BALANCE.clone(), Felt252::zero(), + INITIAL_BALANCE.clone(), + Felt252::zero(), + Felt252::zero(), + Felt252::zero(), Felt252::zero(), Felt252::zero(), ], @@ -636,15 +640,19 @@ fn expected_fee_transfer_info(fee: u128) -> CallInfo { ], }], storage_read_values: vec![ + INITIAL_BALANCE.clone(), + Felt252::zero(), INITIAL_BALANCE.clone(), Felt252::zero(), Felt252::zero(), Felt252::zero(), + Felt252::zero(), + Felt252::zero(), ], accessed_storage_keys: HashSet::from([ [ 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, - 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19, + 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18, ], [ 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, @@ -652,7 +660,7 @@ fn expected_fee_transfer_info(fee: u128) -> CallInfo { ], [ 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, - 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18, + 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19, ], [ 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, @@ -698,26 +706,30 @@ fn expected_fib_fee_transfer_info(fee: u128) -> CallInfo { storage_read_values: vec![ INITIAL_BALANCE.clone() - Felt252::from(2476), Felt252::zero(), + INITIAL_BALANCE.clone() - Felt252::from(2476), + Felt252::zero(), + Felt252::from(2476), + Felt252::zero(), Felt252::from(2476), Felt252::zero(), ], accessed_storage_keys: HashSet::from([ - [ - 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, - 119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235, - ], [ 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, - 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19, + 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18, ], [ - 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, - 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18, + 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, + 119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235, ], [ 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, 119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 236, ], + [ + 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, + 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19, + ], ]), } } @@ -832,27 +844,32 @@ fn expected_declare_fee_transfer_info(fee: u128) -> CallInfo { storage_read_values: vec![ INITIAL_BALANCE.clone(), Felt252::zero(), + INITIAL_BALANCE.clone(), + Felt252::zero(), + Felt252::zero(), + Felt252::zero(), Felt252::zero(), Felt252::zero(), ], accessed_storage_keys: HashSet::from([ [ 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, - 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18, - ], - [ - 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, - 119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235, + 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19, ], [ 7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51, - 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19, + 135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18, ], [ 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, 119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 236, ], + [ + 2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154, + 119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235, + ], ]), + execution_resources: ExecutionResources { n_steps: 525, n_memory_holes: 59, @@ -1287,7 +1304,6 @@ fn test_invoke_tx() { // transaction. let result = invoke_tx.execute(state, block_context, 0).unwrap(); let expected_execution_info = expected_transaction_execution_info(block_context); - assert_eq!(result, expected_execution_info); } diff --git a/tests/storage.rs b/tests/storage.rs index f13bc141c..b75a55ba6 100644 --- a/tests/storage.rs +++ b/tests/storage.rs @@ -124,7 +124,7 @@ fn integration_storage_test() { ..Default::default() }, class_hash: Some(class_hash), - storage_read_values: vec![42.into()], + storage_read_values: vec![0.into(), 42.into()], accessed_storage_keys: expected_accessed_storage_keys, ..Default::default() }; diff --git a/tests/syscalls.rs b/tests/syscalls.rs index 03bfd188f..6672c7d5e 100644 --- a/tests/syscalls.rs +++ b/tests/syscalls.rs @@ -190,7 +190,7 @@ fn call_contract_syscall() { None, [], [], - [10.into()], + [0.into(), 10.into()], [calculate_sn_keccak("lib_state".as_bytes())].into_iter(), [( [2u8; 32], @@ -229,7 +229,7 @@ fn call_contract_syscall() { "1785358123477195475640323002883645042461033713657726545236059599395452130340" )), entry_point_type: Some(EntryPointType::External), - storage_read_values: vec![10.into()], + storage_read_values: vec![10.into(), 10.into()], accessed_storage_keys: [[ 3, 189, 169, 58, 108, 116, 165, 116, 249, 48, 17, 133, 28, 149, 186, 141, 157, 76, 34, 41, 77, 210, 154, 246, 164, 151, 207, 138, 139, 182, 155, 161, @@ -685,7 +685,7 @@ fn library_call_syscall() { None, [], [], - [11.into()], + [0.into(), 11.into()], [calculate_sn_keccak("lib_state".as_bytes())].into_iter(), [( [2; 32], @@ -722,7 +722,7 @@ fn library_call_syscall() { "1785358123477195475640323002883645042461033713657726545236059599395452130340" )), entry_point_type: Some(EntryPointType::External), - storage_read_values: vec![10.into()], + storage_read_values: vec![10.into(), 10.into()], accessed_storage_keys: [[ 3, 189, 169, 58, 108, 116, 165, 116, 249, 48, 17, 133, 28, 149, 186, 141, 157, 76, 34, 41, 77, 210, 154, 246, 164, 151, 207, 138, 139, 182, 155, 161, @@ -800,6 +800,7 @@ fn library_call_l1_handler_syscall() { ]] .into_iter() .collect(), + storage_read_values: vec![0.into()], execution_resources: ExecutionResources { n_steps: 40, ..Default::default() @@ -998,7 +999,7 @@ fn test_deploy_and_call_contract_syscall() { entry_point_type: Some(EntryPointType::External), calldata: vec![4.into()], retdata: vec![(constructor_constant.clone() * Felt252::new(4))], - storage_read_values: vec![constructor_constant], + storage_read_values: vec![constructor_constant.clone()], accessed_storage_keys: HashSet::from([constant_storage_key]), execution_resources: ExecutionResources { n_steps: 52, @@ -1023,7 +1024,7 @@ fn test_deploy_and_call_contract_syscall() { entry_point_type: Some(EntryPointType::External), calldata: vec![new_constant.clone()], retdata: vec![], - storage_read_values: vec![], + storage_read_values: vec![constructor_constant], accessed_storage_keys: HashSet::from([constant_storage_key]), execution_resources: ExecutionResources { n_steps: 40,