starknet_committer,starknet_os,apollo_committer: relocate commitment infos to starknet_committer

itamar-starkware · itamar-starkware · commit 247fbd5bc3b9 · 2026-06-14T01:43:44.000+03:00
diff --git a/crates/apollo_batcher/src/commitment_manager/commitment_manager_impl.rs b/crates/apollo_batcher/src/commitment_manager/commitment_manager_impl.rs
@@ -551,7 +551,6 @@ impl<S: StateCommitterTrait> CommitmentManager<S> {
                     COMMITMENT_MANAGER_REVERT_BLOCK_LATENCY.increment(task_duration);
                     COMMITMENT_MANAGER_REVERT_BLOCK_COUNT.increment(1);
                 }
-                #[cfg(feature = "os_input")]
                 CommitterRequestLabelValue::ReadPathsAndCommitBlock => {
                     debug!(
                         "Read paths and commit block latency for block {height}: {task_duration} \
diff --git a/crates/apollo_batcher/src/commitment_manager/types.rs b/crates/apollo_batcher/src/commitment_manager/types.rs
@@ -110,7 +110,6 @@ impl TaskTimer {
             CommitterRequestLabelValue::CommitBlock => {
                 self.commit.insert(height, instant);
             }
-            #[cfg(feature = "os_input")]
             CommitterRequestLabelValue::ReadPathsAndCommitBlock => {
                 self.commit.insert(height, instant);
             }
@@ -128,7 +127,6 @@ impl TaskTimer {
     ) -> Option<u128> {
         let map = match task {
             CommitterRequestLabelValue::CommitBlock => &mut self.commit,
-            #[cfg(feature = "os_input")]
             CommitterRequestLabelValue::ReadPathsAndCommitBlock => &mut self.commit,
             CommitterRequestLabelValue::RevertBlock => &mut self.revert,
         };
diff --git a/crates/apollo_committer/src/committer.rs b/crates/apollo_committer/src/committer.rs
@@ -684,8 +684,12 @@ fn update_metrics(
         n_writes,
         durations,
         modifications_counts,
+        // `fetched_witnesses_count` exists whenever `starknet_committer/os_input` is enabled, which
+        // can happen independently of this crate's `os_input` (e.g. via `apollo_committer_types`).
+        // `..` tolerates that mismatch; the field is still bound and used under our own `os_input`.
         #[cfg(feature = "os_input")]
         fetched_witnesses_count,
+        ..
     }: &BlockMeasurement,
 ) {
     BLOCKS_COMMITTED.increment(1);
diff --git a/crates/apollo_committer_types/src/communication.rs b/crates/apollo_committer_types/src/communication.rs
@@ -13,7 +13,7 @@ use async_trait::async_trait;
 #[cfg(any(feature = "testing", test))]
 use mockall::automock;
 use serde::{Deserialize, Serialize};
-use strum::{AsRefStr, EnumDiscriminants, EnumIter, IntoStaticStr, VariantNames};
+use strum::{AsRefStr, EnumIter, IntoStaticStr, VariantNames};
 
 use crate::committer_types::{
     CommitBlockRequest,
@@ -55,19 +55,38 @@ pub trait CommitterClient: Send + Sync {
     ) -> CommitterClientResult<ReadPathsAndCommitBlockResponse>;
 }
 
-#[derive(Serialize, Deserialize, Clone, AsRefStr, EnumDiscriminants)]
-#[strum_discriminants(
-    name(CommitterRequestLabelValue),
-    derive(IntoStaticStr, EnumIter, VariantNames),
-    strum(serialize_all = "snake_case")
-)]
+#[derive(Serialize, Deserialize, Clone, AsRefStr)]
 pub enum CommitterRequest {
     CommitBlock(CommitBlockRequest),
     RevertBlock(RevertBlockRequest),
     #[cfg(feature = "os_input")]
     ReadPathsAndCommitBlock(ReadPathsAndCommitBlockRequest),
 }
 
+/// Payload-free discriminants of [`CommitterRequest`], used as a metric label. Independent of
+/// `os_input` on purpose: the label set is identical in every build, while the request variant and
+/// its payload stay feature-gated. Hand-written rather than derived via `EnumDiscriminants` so the
+/// `os_input`-only request variant does not gate the label out — otherwise every consumer would
+/// have to re-gate on its own `os_input`, which diverges across crates under `--all-features`.
+#[derive(Clone, Copy, Debug, PartialEq, Eq, IntoStaticStr, EnumIter, VariantNames)]
+#[strum(serialize_all = "snake_case")]
+pub enum CommitterRequestLabelValue {
+    CommitBlock,
+    RevertBlock,
+    ReadPathsAndCommitBlock,
+}
+
+impl From<&CommitterRequest> for CommitterRequestLabelValue {
+    fn from(request: &CommitterRequest) -> Self {
+        match request {
+            CommitterRequest::CommitBlock(_) => Self::CommitBlock,
+            CommitterRequest::RevertBlock(_) => Self::RevertBlock,
+            #[cfg(feature = "os_input")]
+            CommitterRequest::ReadPathsAndCommitBlock(_) => Self::ReadPathsAndCommitBlock,
+        }
+    }
+}
+
 impl_debug_for_infra_requests_and_responses!(CommitterRequest);
 impl_labeled_request!(CommitterRequest, CommitterRequestLabelValue);
 impl PrioritizedRequest for CommitterRequest {}
diff --git a/crates/starknet_committer/src/patricia_merkle_tree/types.rs b/crates/starknet_committer/src/patricia_merkle_tree/types.rs
@@ -7,7 +7,7 @@ use starknet_patricia::impl_from_hex_for_felt_wrapper;
 use starknet_patricia::patricia_merkle_tree::filled_tree::tree::FilledTreeImpl;
 use starknet_patricia::patricia_merkle_tree::node_data::inner_node::PreimageMap;
 use starknet_patricia::patricia_merkle_tree::node_data::leaf::SkeletonLeaf;
-use starknet_patricia::patricia_merkle_tree::types::NodeIndex;
+use starknet_patricia::patricia_merkle_tree::types::{NodeIndex, SubTreeHeight};
 use starknet_types_core::felt::{Felt, FromStrError};
 
 use crate::block_committer::input::{try_node_index_into_contract_address, StarknetStorageValue};
@@ -57,6 +57,40 @@ pub struct StarknetForestProofs {
     pub contracts_trie_storage_proofs: HashMap<ContractAddress, PreimageMap>,
 }
 
+#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct CommitmentInfo {
+    pub previous_root: HashOutput,
+    pub updated_root: HashOutput,
+    pub tree_height: SubTreeHeight,
+    // TODO(Dori, 1/8/2025): The value type here should probably be more specific (NodeData<L> for
+    //   L: Leaf). This poses a problem in deserialization, as a serialized edge node and a
+    //   serialized contract state leaf are both currently vectors of 3 field elements; as the
+    //   semantics of the values are unimportant for the OS commitments, we make do with a vector
+    //   of field elements as values for now.
+    pub commitment_facts: HashMap<HashOutput, Vec<Felt>>,
+}
+
+#[cfg(any(feature = "testing", test))]
+impl Default for CommitmentInfo {
+    fn default() -> CommitmentInfo {
+        CommitmentInfo {
+            previous_root: HashOutput::default(),
+            updated_root: HashOutput::default(),
+            tree_height: SubTreeHeight::ACTUAL_HEIGHT,
+            commitment_facts: HashMap::default(),
+        }
+    }
+}
+
+/// Contains all commitment information for a block's state trees.
+#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
+pub struct StateCommitmentInfos {
+    pub contracts_trie_commitment_info: CommitmentInfo,
+    pub classes_trie_commitment_info: CommitmentInfo,
+    pub storage_tries_commitment_infos: HashMap<ContractAddress, CommitmentInfo>,
+}
+
 impl StarknetForestProofs {
     pub fn build<Layout>(
         classes_trie_proof: PreimageMap,
diff --git a/crates/starknet_os/src/commitment_infos.rs b/crates/starknet_os/src/commitment_infos.rs
@@ -11,50 +11,18 @@ use starknet_committer::db::facts_db::create_facts_tree::get_leaves;
 use starknet_committer::patricia_merkle_tree::leaf::leaf_impl::ContractState;
 use starknet_committer::patricia_merkle_tree::tree::fetch_previous_and_new_patricia_paths;
 use starknet_committer::patricia_merkle_tree::types::RootHashes;
+// `CommitmentInfo` and `StateCommitmentInfos` were relocated to `starknet_committer` so that
+// lower layers (e.g. `apollo_storage`) can store them without depending on `starknet_os`. They
+// are re-exported here to keep this module's public API stable for OS consumers.
+pub use starknet_committer::patricia_merkle_tree::types::{CommitmentInfo, StateCommitmentInfos};
 use starknet_patricia::patricia_merkle_tree::node_data::inner_node::flatten_preimages;
 use starknet_patricia::patricia_merkle_tree::original_skeleton_tree::errors::OriginalSkeletonTreeError;
 use starknet_patricia::patricia_merkle_tree::traversal::TraversalError;
 use starknet_patricia::patricia_merkle_tree::types::{NodeIndex, SortedLeafIndices, SubTreeHeight};
 use starknet_patricia_storage::db_object::EmptyKeyContext;
 use starknet_patricia_storage::map_storage::MapStorage;
-use starknet_types_core::felt::Felt;
 use thiserror::Error;
 
-#[cfg_attr(feature = "deserialize", derive(serde::Deserialize))]
-#[cfg_attr(feature = "deserialize", serde(deny_unknown_fields))]
-#[derive(Debug)]
-pub struct CommitmentInfo {
-    pub previous_root: HashOutput,
-    pub updated_root: HashOutput,
-    pub tree_height: SubTreeHeight,
-    // TODO(Dori, 1/8/2025): The value type here should probably be more specific (NodeData<L> for
-    //   L: Leaf). This poses a problem in deserialization, as a serialized edge node and a
-    //   serialized contract state leaf are both currently vectors of 3 field elements; as the
-    //   semantics of the values are unimportant for the OS commitments, we make do with a vector
-    //   of field elements as values for now.
-    pub commitment_facts: HashMap<HashOutput, Vec<Felt>>,
-}
-
-#[cfg(any(feature = "testing", test))]
-impl Default for CommitmentInfo {
-    fn default() -> CommitmentInfo {
-        CommitmentInfo {
-            previous_root: HashOutput::default(),
-            updated_root: HashOutput::default(),
-            tree_height: SubTreeHeight::ACTUAL_HEIGHT,
-            commitment_facts: HashMap::default(),
-        }
-    }
-}
-
-// TODO(Aviv): Use this struct in `OsBlockInput`
-/// Contains all commitment information for a block's state trees.
-pub struct StateCommitmentInfos {
-    pub contracts_trie_commitment_info: CommitmentInfo,
-    pub classes_trie_commitment_info: CommitmentInfo,
-    pub storage_tries_commitment_infos: HashMap<ContractAddress, CommitmentInfo>,
-}
-
 /// Error type for commitment infos creation.
 #[derive(Debug, Error)]
 pub enum CommitmentInfosError {
@@ -66,83 +34,82 @@ pub enum CommitmentInfosError {
     FetchStorageProofs(#[from] TraversalError),
 }
 
-impl StateCommitmentInfos {
-    /// Creates the commitment infos for the OS from previous and new state roots and the
-    /// keys that were read during execution.
-    pub async fn new(
-        previous_state_roots: &StateRoots,
-        new_state_roots: &StateRoots,
-        commitments: &mut MapStorage,
-        accessed_keys: &AccessedKeys,
-    ) -> Result<Self, CommitmentInfosError> {
-        let addresses: Vec<ContractAddress> =
-            accessed_keys.accessed_contracts.iter().copied().collect();
-
-        let previous_storage_roots = get_storage_roots(
-            &addresses,
-            previous_state_roots.contracts_trie_root_hash,
-            commitments,
-        )
-        .await?;
-        let new_storage_roots =
-            get_storage_roots(&addresses, new_state_roots.contracts_trie_root_hash, commitments)
-                .await?;
+/// Creates the commitment infos for the OS from previous and new state roots and the
+/// keys that were read during execution.
+// TODO(ItamarS): Temporary — to be deleted once the committer builds `StateCommitmentInfos` from
+// its own storage; tests against that new committer API will be added then. Kept here (as a free
+// function rather than an inherent method) because the struct now lives in `starknet_committer`
+// and the orphan rule forbids an inherent impl on a foreign type.
+pub async fn build_state_commitment_infos(
+    previous_state_roots: &StateRoots,
+    new_state_roots: &StateRoots,
+    commitments: &mut MapStorage,
+    accessed_keys: &AccessedKeys,
+) -> Result<StateCommitmentInfos, CommitmentInfosError> {
+    let addresses: Vec<ContractAddress> =
+        accessed_keys.accessed_contracts.iter().copied().collect();
 
-        let storage_proofs = fetch_previous_and_new_patricia_paths(
-            commitments,
-            RootHashes {
-                previous_root_hash: previous_state_roots.classes_trie_root_hash,
-                new_root_hash: new_state_roots.classes_trie_root_hash,
-            },
-            RootHashes {
-                previous_root_hash: previous_state_roots.contracts_trie_root_hash,
-                new_root_hash: new_state_roots.contracts_trie_root_hash,
-            },
-            accessed_keys,
-        )
-        .await?;
+    let previous_storage_roots =
+        get_storage_roots(&addresses, previous_state_roots.contracts_trie_root_hash, commitments)
+            .await?;
+    let new_storage_roots =
+        get_storage_roots(&addresses, new_state_roots.contracts_trie_root_hash, commitments)
+            .await?;
 
-        let contracts_trie_commitment_info = CommitmentInfo {
-            previous_root: previous_state_roots.contracts_trie_root_hash,
-            updated_root: new_state_roots.contracts_trie_root_hash,
-            tree_height: SubTreeHeight::ACTUAL_HEIGHT,
-            commitment_facts: flatten_preimages(&storage_proofs.contracts_trie_proof.nodes),
-        };
-        let classes_trie_commitment_info = CommitmentInfo {
-            previous_root: previous_state_roots.classes_trie_root_hash,
-            updated_root: new_state_roots.classes_trie_root_hash,
-            tree_height: SubTreeHeight::ACTUAL_HEIGHT,
-            commitment_facts: flatten_preimages(&storage_proofs.classes_trie_proof),
-        };
-        let storage_tries_commitment_infos = previous_storage_roots
-            .iter()
-            .map(|(address, previous_root_hash)| {
-                // Not all contracts in `previous_storage_roots` have storage proofs. For
-                // example, a contract that only had its nonce changed.
-                let storage_proof = flatten_preimages(
-                    storage_proofs
-                        .contracts_trie_storage_proofs
-                        .get(address)
-                        .unwrap_or(&HashMap::new()),
-                );
-                (
-                    *address,
-                    CommitmentInfo {
-                        previous_root: *previous_root_hash,
-                        updated_root: new_storage_roots[address],
-                        tree_height: SubTreeHeight::ACTUAL_HEIGHT,
-                        commitment_facts: storage_proof,
-                    },
-                )
-            })
-            .collect();
+    let storage_proofs = fetch_previous_and_new_patricia_paths(
+        commitments,
+        RootHashes {
+            previous_root_hash: previous_state_roots.classes_trie_root_hash,
+            new_root_hash: new_state_roots.classes_trie_root_hash,
+        },
+        RootHashes {
+            previous_root_hash: previous_state_roots.contracts_trie_root_hash,
+            new_root_hash: new_state_roots.contracts_trie_root_hash,
+        },
+        accessed_keys,
+    )
+    .await?;
 
-        Ok(Self {
-            contracts_trie_commitment_info,
-            classes_trie_commitment_info,
-            storage_tries_commitment_infos,
+    let contracts_trie_commitment_info = CommitmentInfo {
+        previous_root: previous_state_roots.contracts_trie_root_hash,
+        updated_root: new_state_roots.contracts_trie_root_hash,
+        tree_height: SubTreeHeight::ACTUAL_HEIGHT,
+        commitment_facts: flatten_preimages(&storage_proofs.contracts_trie_proof.nodes),
+    };
+    let classes_trie_commitment_info = CommitmentInfo {
+        previous_root: previous_state_roots.classes_trie_root_hash,
+        updated_root: new_state_roots.classes_trie_root_hash,
+        tree_height: SubTreeHeight::ACTUAL_HEIGHT,
+        commitment_facts: flatten_preimages(&storage_proofs.classes_trie_proof),
+    };
+    let storage_tries_commitment_infos = previous_storage_roots
+        .iter()
+        .map(|(address, previous_root_hash)| {
+            // Not all contracts in `previous_storage_roots` have storage proofs. For
+            // example, a contract that only had its nonce changed.
+            let storage_proof = flatten_preimages(
+                storage_proofs
+                    .contracts_trie_storage_proofs
+                    .get(address)
+                    .unwrap_or(&HashMap::new()),
+            );
+            (
+                *address,
+                CommitmentInfo {
+                    previous_root: *previous_root_hash,
+                    updated_root: new_storage_roots[address],
+                    tree_height: SubTreeHeight::ACTUAL_HEIGHT,
+                    commitment_facts: storage_proof,
+                },
+            )
         })
-    }
+        .collect();
+
+    Ok(StateCommitmentInfos {
+        contracts_trie_commitment_info,
+        classes_trie_commitment_info,
+        storage_tries_commitment_infos,
+    })
 }
 
 /// Fetches the storage root hash of each contract from the contracts trie at the given root.
diff --git a/crates/starknet_os_flow_tests/src/test_manager.rs b/crates/starknet_os_flow_tests/src/test_manager.rs
@@ -65,7 +65,7 @@ use starknet_committer::block_committer::input::{
 };
 use starknet_committer::db::facts_db::FactsDb;
 use starknet_committer::db::forest_trait::StorageInitializer;
-use starknet_os::commitment_infos::StateCommitmentInfos;
+use starknet_os::commitment_infos::build_state_commitment_infos;
 use starknet_os::hints::hint_implementation::state_diff_encryption::utils::compute_public_keys;
 use starknet_os::io::os_input::{OsBlockInput, OsHints, OsHintsConfig, StarknetOsInput};
 use starknet_os::io::os_output::{MessageToL2, OsStateDiff, StarknetOsRunnerOutput};
@@ -940,7 +940,7 @@ impl<S: FlowTestState> TestBuilder<S> {
             .expect("Failed to commit state diff.");
             map_storage = db.consume_storage();
 
-            let commitment_infos = StateCommitmentInfos::new(
+            let commitment_infos = build_state_commitment_infos(
                 &previous_state_roots,
                 &new_state_roots,
                 &mut map_storage,
diff --git a/crates/starknet_transaction_prover/src/running/storage_proofs.rs b/crates/starknet_transaction_prover/src/running/storage_proofs.rs
@@ -8,7 +8,11 @@ use serde::{Deserialize, Serialize};
 use starknet_api::block::BlockNumber;
 use starknet_api::core::{ClassHash, ContractAddress, Nonce};
 use starknet_api::hash::{HashOutput, StateRoots};
-use starknet_os::commitment_infos::{CommitmentInfo, StateCommitmentInfos};
+use starknet_os::commitment_infos::{
+    build_state_commitment_infos,
+    CommitmentInfo,
+    StateCommitmentInfos,
+};
 use starknet_patricia::patricia_merkle_tree::node_data::inner_node::{
     flatten_preimages,
     Preimage,
@@ -348,7 +352,7 @@ impl RpcStorageProofsProvider {
 
         // TODO(Aviv): Try to undertand if we can create classes trie commitment info
         // without the compiled class hashes.
-        let mut commitment_infos = StateCommitmentInfos::new(
+        let mut commitment_infos = build_state_commitment_infos(
             &previous_state_roots,
             &new_roots,
             &mut map_storage,

Original file line number	Diff line number	Diff line change
`@@ -551,7 +551,6 @@ impl<S: StateCommitterTrait> CommitmentManager<S> {`
`551`	`551`	`COMMITMENT_MANAGER_REVERT_BLOCK_LATENCY.increment(task_duration);`
`552`	`552`	`COMMITMENT_MANAGER_REVERT_BLOCK_COUNT.increment(1);`
`553`	`553`	`}`
`554`		`- #[cfg(feature = "os_input")]`
`555`	`554`	`CommitterRequestLabelValue::ReadPathsAndCommitBlock => {`
`556`	`555`	`debug!(`
`557`	`556`	`"Read paths and commit block latency for block {height}: {task_duration} \`