Merge pull request #13021 from starkware-libs/yonatan/merge-main-v0.14.1-committer-into-main-v0.14.2-1772610549

Merge main-v0.14.1-committer into main-v0.14.2

Author: Yoni-Starkware

Cargo.lock

@@ -38,6 +38,7 @@ cairo-vm.workspace = true
 chrono.workspace = true
 futures.workspace = true
 indexmap.workspace = true
+lru.workspace = true
 reqwest = { workspace = true, features = ["json"] }
 serde.workspace = true
 starknet_api.workspace = true

crates/apollo_batcher/Cargo.toml

@@ -71,8 +71,10 @@ use indexmap::{IndexMap, IndexSet};
 #[cfg(test)]
 use mockall::automock;
 use starknet_api::block::{BlockHash, BlockNumber, UnixTimestamp};
-use starknet_api::block_hash::block_hash_calculator::PartialBlockHashComponents;
-use starknet_api::block_hash::state_diff_hash::calculate_state_diff_hash;
+use starknet_api::block_hash::block_hash_calculator::{
+    PartialBlockHash,
+    PartialBlockHashComponents,
+};
 use starknet_api::consensus_transaction::InternalConsensusTransaction;
 use starknet_api::core::{ContractAddress, GlobalRoot, Nonce, StateDiffCommitment};
 use starknet_api::state::{StateNumber, ThinStateDiff};
@@ -849,7 +851,25 @@ impl Batcher {
         );
         trace!("Rejected transactions: {:#?}, State diff: {:#?}.", rejected_tx_hashes, state_diff);
 
-        let state_diff_commitment = calculate_state_diff_hash(&state_diff);
+        // Proposal commitment is the the partial block hash when it's available, and None
+        // otherwise. The commitment is computed here to set the prev_proposal_commitment cache. As
+        // this cache is only used for blocks obtained through the decision_reached flow, it can
+        // be None for old (pre 0.13.2) blocks.
+        let proposal_commitment = match &storage_commitment_block_hash {
+            StorageCommitmentBlockHash::Partial(components) => Some((
+                height,
+                ProposalCommitment {
+                    partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
+                        components,
+                    )
+                    .map_err(|e| {
+                        error!("Failed to compute partial block hash: {}", e);
+                        BatcherError::InternalError
+                    })?,
+                },
+            )),
+            StorageCommitmentBlockHash::ParentHash(_) => None,
+        };
 
         // Commit the proposal to the storage.
         self.storage_writer
@@ -859,8 +879,8 @@ impl Batcher {
                 BatcherError::InternalError
             })?;
         info!("Successfully committed proposal for block {} to storage.", height);
-        self.prev_proposal_commitment =
-            Some((height, ProposalCommitment { state_diff_commitment }));
+
+        self.prev_proposal_commitment = proposal_commitment;
 
         // Notify the L1 provider of the new block.
         let rejected_l1_handler_tx_hashes = rejected_tx_hashes
@@ -1160,21 +1180,30 @@ impl Batcher {
                 Ok(Some(commitment))
             }
             None => {
-                // Parent proposal commitment is not cached. Compute it from the stored state diff.
-                let state_diff = self
+                // Parent proposal commitment is not cached. Read partial block hash
+                // components from storage and compute the partial block hash.
+                let (_, components) = self
                     .storage_reader
-                    .get_state_diff(prev_height)
+                    .get_parent_hash_and_partial_block_hash_components(prev_height)
                     .map_err(|err| {
                         error!(
-                            "Failed to read state diff for previous height {prev_height}: {}",
+                            "Failed to read partial block hash components for previous height \
+                             {prev_height}: {}",
                             err
                         );
                         BatcherError::InternalError
-                    })?
-                    .expect("Missing state diff for previous height.");
+                    })?;
+                let components =
+                    components.expect("Missing partial block hash components for previous height.");
 
                 Ok(Some(ProposalCommitment {
-                    state_diff_commitment: calculate_state_diff_hash(&state_diff),
+                    partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
+                        &components,
+                    )
+                    .map_err(|e| {
+                        error!("Failed to compute partial block hash: {}", e);
+                        BatcherError::InternalError
+                    })?,
                 }))
             }
         }
@@ -1249,13 +1278,26 @@ impl Batcher {
 
     pub fn get_block_hash(&mut self, block_number: BlockNumber) -> BatcherResult<BlockHash> {
         self.get_commitment_results_and_write_to_storage()?;
-        self.storage_reader
+        let cache = &mut self.commitment_manager.recent_block_hashes_cache;
+
+        // Try to get the block hash from the cache.
+        if let Some(block_hash) = cache.get(&block_number) {
+            return Ok(*block_hash);
+        }
+
+        // If the block hash is not in the cache, fetch it from storage.
+        info!("Block hash of block {block_number} not found in cache, fetching from storage.");
+        // TODO(Nimrod): Consider adding block-hashes cache misses metric.
+        let block_hash = self
+            .storage_reader
             .get_block_hash(block_number)
             .map_err(|err| {
                 error!("Failed to get block hash from storage: {err}");
                 BatcherError::InternalError
             })?
-            .ok_or(BatcherError::BlockHashNotFound(block_number))
+            .ok_or(BatcherError::BlockHashNotFound(block_number))?;
+        cache.put(block_number, block_hash);
+        Ok(block_hash)
     }
 
     fn get_commitment_results_and_write_to_storage(&mut self) -> BatcherResult<()> {

crates/apollo_batcher/src/batcher.rs

@@ -52,8 +52,11 @@ use starknet_api::block::{
     BlockNumber,
     StarknetVersion,
 };
-use starknet_api::block_hash::block_hash_calculator::PartialBlockHashComponents;
-use starknet_api::block_hash::state_diff_hash::calculate_state_diff_hash;
+use starknet_api::block_hash::block_hash_calculator::{
+    calculate_block_hash,
+    PartialBlockHash,
+    PartialBlockHashComponents,
+};
 use starknet_api::consensus_transaction::InternalConsensusTransaction;
 use starknet_api::core::{ClassHash, CompiledClassHash, GlobalRoot, Nonce};
 use starknet_api::state::ThinStateDiff;
@@ -180,7 +183,12 @@ async fn finished_proposal_info() -> FinishedProposalInfo {
 }
 
 fn parent_proposal_commitment() -> ProposalCommitment {
-    ProposalCommitment { state_diff_commitment: calculate_state_diff_hash(&test_state_diff()) }
+    ProposalCommitment {
+        partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
+            &PartialBlockHashComponents::default(),
+        )
+        .expect("default partial block hash components are valid"),
+    }
 }
 
 fn validate_block_input(proposal_id: ProposalId) -> ValidateBlockInput {
@@ -1419,6 +1427,14 @@ async fn decision_reached() {
         )
         .returning(|_, _, _| Ok(()));
 
+    mock_dependencies
+        .storage_reader
+        .expect_get_parent_hash_and_partial_block_hash_components()
+        .with(eq(INITIAL_HEIGHT.prev().unwrap()))
+        .returning(|_| {
+            Ok((Some(BlockHash::default()), Some(PartialBlockHashComponents::default())))
+        });
+
     mock_create_builder_for_propose_block(
         &mut mock_dependencies.clients.block_builder_factory,
         vec![],
@@ -1491,6 +1507,14 @@ async fn test_execution_info_order_is_kept() {
         .collect();
     verify_indexed_execution_infos(&execution_infos);
 
+    mock_dependencies
+        .storage_reader
+        .expect_get_parent_hash_and_partial_block_hash_components()
+        .with(eq(INITIAL_HEIGHT.prev().unwrap()))
+        .returning(|_| {
+            Ok((Some(BlockHash::default()), Some(PartialBlockHashComponents::default())))
+        });
+
     mock_create_builder_for_propose_block(
         &mut mock_dependencies.clients.block_builder_factory,
         vec![],
@@ -1576,6 +1600,14 @@ async fn decision_reached_return_success_when_l1_commit_block_fails(
 
     mock_dependencies.clients.mempool_client.expect_commit_block().returning(|_| Ok(()));
 
+    mock_dependencies
+        .storage_reader
+        .expect_get_parent_hash_and_partial_block_hash_components()
+        .with(eq(INITIAL_HEIGHT.prev().unwrap()))
+        .returning(|_| {
+            Ok((Some(BlockHash::default()), Some(PartialBlockHashComponents::default())))
+        });
+
     mock_create_builder_for_propose_block(
         &mut mock_dependencies.clients.block_builder_factory,
         vec![],
@@ -1640,32 +1672,27 @@ async fn get_block_hash_not_found() {
 #[tokio::test]
 async fn get_block_hash_after_reading_commitment_results() {
     let mut mock_dependencies = MockDependencies::default();
-    let block_hash = BlockHash::default();
+    let global_root = GlobalRoot::default();
+    let partial_components =
+        PartialBlockHashComponents { block_number: INITIAL_HEIGHT, ..Default::default() };
+    let parent_hash = BlockHash::default();
+    let expected_block_hash =
+        calculate_block_hash(&partial_components, global_root, parent_hash).unwrap();
 
     // Should be called by the commitment manager when finalizing results and writing them to
     // storage.
     mock_dependencies
         .storage_reader
         .expect_get_parent_hash_and_partial_block_hash_components()
         .with(eq(INITIAL_HEIGHT))
-        .returning(|height| {
-            let partial = PartialBlockHashComponents { block_number: height, ..Default::default() };
-            Ok((Some(BlockHash::default()), Some(partial)))
-        });
+        .returning(move |_| Ok((Some(parent_hash), Some(partial_components.clone()))));
     mock_dependencies
         .storage_writer
         .expect_set_global_root_and_block_hash()
         .times(1)
-        .with(eq(INITIAL_HEIGHT), eq(GlobalRoot::default()), always())
+        .with(eq(INITIAL_HEIGHT), eq(global_root), always())
         .returning(|_, _, _| Ok(()));
 
-    mock_dependencies
-        .storage_reader
-        .expect_get_block_hash()
-        .times(1)
-        .with(eq(INITIAL_HEIGHT))
-        .returning(move |_| Ok(Some(block_hash)));
-
     let mut batcher = create_batcher(mock_dependencies).await;
 
     // Send a commitment task directly to the state committer so a result will be available.
@@ -1678,7 +1705,7 @@ async fn get_block_hash_after_reading_commitment_results() {
     wait_for_n_items(&mut batcher.commitment_manager.results_receiver, 1).await;
 
     let result = batcher.get_block_hash(INITIAL_HEIGHT);
-    assert_eq!(result, Ok(block_hash));
+    assert_eq!(result, Ok(expected_block_hash));
     assert_eq!(
         get_number_of_items_in_channel_from_receiver(&batcher.commitment_manager.results_receiver),
         0