From 0abafd7b0a4a14023561751c3bd017f315606421 Mon Sep 17 00:00:00 2001 From: Meshi Peled <141231558+meship-starkware@users.noreply.github.com> Date: Wed, 5 Jun 2024 14:10:49 +0300 Subject: [PATCH] fix(concurrency): in versioned storage - read the writes of the current transaction (#1944) --- .../src/concurrency/versioned_state.rs | 3 ++- .../src/concurrency/versioned_state_test.rs | 4 ++-- .../src/concurrency/versioned_storage.rs | 5 ++-- .../src/concurrency/versioned_storage_test.rs | 4 ++-- .../src/concurrency/worker_logic.rs | 4 +--- .../src/concurrency/worker_logic_test.rs | 23 ++++++++----------- 6 files changed, 19 insertions(+), 24 deletions(-) diff --git a/crates/blockifier/src/concurrency/versioned_state.rs b/crates/blockifier/src/concurrency/versioned_state.rs index 88e99eddc4..1fd716a7f1 100644 --- a/crates/blockifier/src/concurrency/versioned_state.rs +++ b/crates/blockifier/src/concurrency/versioned_state.rs @@ -93,7 +93,8 @@ impl VersionedState { if tx_index == 0 { return true; } - + // Ignore values written by the current transaction. + let tx_index = tx_index - 1; for (&(contract_address, storage_key), expected_value) in &reads.storage { let value = self.storage.read(tx_index, (contract_address, storage_key)).expect(READ_ERR); diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index 80fb14f4b3..3644092e85 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -174,10 +174,10 @@ fn test_versioned_state_proxy() { assert!( versioned_state_proxys[4].state().declared_contracts.read(4, another_class_hash).unwrap() ); - // Ignore the writes in the current transaction. + // Include the writes in the current transaction. assert_eq!( versioned_state_proxys[10].get_class_hash_at(contract_address).unwrap(), - class_hash_v7 + class_hash_v10 ); assert_eq!( versioned_state_proxys[2].get_compiled_class_hash(class_hash).unwrap(), diff --git a/crates/blockifier/src/concurrency/versioned_storage.rs b/crates/blockifier/src/concurrency/versioned_storage.rs index dba52e54c2..c55735c566 100644 --- a/crates/blockifier/src/concurrency/versioned_storage.rs +++ b/crates/blockifier/src/concurrency/versioned_storage.rs @@ -40,9 +40,8 @@ where V: Clone + Debug, { pub fn read(&self, tx_index: TxIndex, key: K) -> Option { - // Ignore the writes in the current transaction (may contain an `ESTIMATE` value). Reading - // the value written in this transaction should be handled by the state. - let value = self.writes.get(&key).and_then(|cell| cell.range(..tx_index).next_back()); + // TODO: Ignore `ESTIMATE` values (when added). + let value = self.writes.get(&key).and_then(|cell| cell.range(..=tx_index).next_back()); value.map(|(_, value)| value).or_else(|| self.cached_initial_values.get(&key)).cloned() } diff --git a/crates/blockifier/src/concurrency/versioned_storage_test.rs b/crates/blockifier/src/concurrency/versioned_storage_test.rs index 34b6349073..2f65fd71e3 100644 --- a/crates/blockifier/src/concurrency/versioned_storage_test.rs +++ b/crates/blockifier/src/concurrency/versioned_storage_test.rs @@ -32,8 +32,8 @@ fn test_versioned_storage() { // Read from the past. storage.write(2, 10, 78); assert_eq!(storage.read(1, 10).unwrap(), 31); - // Ignore the value written by the current transaction. - assert_eq!(storage.read(2, 10).unwrap(), 31); + // Include the value written by the current transaction. + assert_eq!(storage.read(2, 10).unwrap(), 78); // Read uninitialized cell. assert!(storage.read(1, 100).is_none()); diff --git a/crates/blockifier/src/concurrency/worker_logic.rs b/crates/blockifier/src/concurrency/worker_logic.rs index 4f7fca7bd3..105eb988cd 100644 --- a/crates/blockifier/src/concurrency/worker_logic.rs +++ b/crates/blockifier/src/concurrency/worker_logic.rs @@ -258,9 +258,7 @@ impl<'a, S: StateReader> WorkerExecutor<'a, S> { return true; } - let mut next_tx_versioned_state = self.state.pin_version(tx_index + 1); - let (sequencer_balance_value_low, sequencer_balance_value_high) = - next_tx_versioned_state + let (sequencer_balance_value_low, sequencer_balance_value_high) = tx_versioned_state .get_fee_token_balance( tx_context.block_context.block_info.sequencer_address, tx_context.fee_token_address(), diff --git a/crates/blockifier/src/concurrency/worker_logic_test.rs b/crates/blockifier/src/concurrency/worker_logic_test.rs index 9e51095080..2ca127da61 100644 --- a/crates/blockifier/src/concurrency/worker_logic_test.rs +++ b/crates/blockifier/src/concurrency/worker_logic_test.rs @@ -116,7 +116,7 @@ fn test_worker_execute() { // Read a write made by the transaction. assert_eq!( safe_versioned_state - .pin_version(tx_index + 1) + .pin_version(tx_index) .get_storage_at(test_contract_address, storage_key) .unwrap(), storage_value @@ -176,7 +176,7 @@ fn test_worker_execute() { worker_executor.execute(tx_index); // No write was made by the transaction. assert_eq!( - safe_versioned_state.pin_version(tx_index + 1).get_nonce_at(account_address).unwrap(), + safe_versioned_state.pin_version(tx_index).get_nonce_at(account_address).unwrap(), nonce!(1_u8) ); let execution_output = worker_executor.execution_outputs[tx_index].lock().unwrap(); @@ -195,7 +195,7 @@ fn test_worker_execute() { worker_executor.execute(tx_index); // Read a write made by the transaction. assert_eq!( - safe_versioned_state.pin_version(tx_index + 1).get_nonce_at(account_address).unwrap(), + safe_versioned_state.pin_version(tx_index).get_nonce_at(account_address).unwrap(), nonce!(2_u8) ); let execution_output = worker_executor.execution_outputs[tx_index].lock().unwrap(); @@ -289,7 +289,7 @@ fn test_worker_validate() { // Verify writes exist in state. assert_eq!( safe_versioned_state - .pin_version(tx_index + 1) + .pin_version(tx_index) .get_storage_at(test_contract_address, storage_key) .unwrap(), storage_value0 @@ -304,7 +304,7 @@ fn test_worker_validate() { // Verify writes were removed. assert_eq!( safe_versioned_state - .pin_version(tx_index + 1) + .pin_version(tx_index) .get_storage_at(test_contract_address, storage_key) .unwrap(), storage_value0 @@ -346,14 +346,11 @@ pub fn test_add_fee_to_sequencer_balance( sequencer_balance_low, sequencer_balance_high, ); - let next_tx_versioned_state = safe_versioned_state.pin_version(tx_index + 1); - - let new_sequencer_balance_value_low = next_tx_versioned_state - .get_storage_at(fee_token_address, sequencer_balance_key_low) - .unwrap(); - let new_sequencer_balance_value_high = next_tx_versioned_state - .get_storage_at(fee_token_address, sequencer_balance_key_high) - .unwrap(); + + let new_sequencer_balance_value_low = + tx_versioned_state.get_storage_at(fee_token_address, sequencer_balance_key_low).unwrap(); + let new_sequencer_balance_value_high = + tx_versioned_state.get_storage_at(fee_token_address, sequencer_balance_key_high).unwrap(); let expected_balance = (stark_felt_to_felt(sequencer_balance_low) + Felt252::from(actual_fee.0)).to_biguint();