From 3bc9a7f95ccd1f0692e15cb7b5d661cdd6139ab8 Mon Sep 17 00:00:00 2001 From: Dafna Matsry Date: Tue, 10 Dec 2024 12:25:32 +0200 Subject: [PATCH] refactor(starknet_batcher): move around some types and utils to a common utils file --- crates/starknet_batcher/src/batcher.rs | 82 ++--------- crates/starknet_batcher/src/batcher_test.rs | 9 +- crates/starknet_batcher/src/lib.rs | 1 + .../starknet_batcher/src/proposal_manager.rs | 66 +-------- .../src/proposal_manager_test.rs | 9 +- crates/starknet_batcher/src/utils.rs | 137 ++++++++++++++++++ 6 files changed, 155 insertions(+), 149 deletions(-) create mode 100644 crates/starknet_batcher/src/utils.rs diff --git a/crates/starknet_batcher/src/batcher.rs b/crates/starknet_batcher/src/batcher.rs index 8e0a341cf0..17f083a207 100644 --- a/crates/starknet_batcher/src/batcher.rs +++ b/crates/starknet_batcher/src/batcher.rs @@ -1,13 +1,11 @@ use std::collections::HashMap; use std::sync::Arc; -use blockifier::abi::constants; use blockifier::state::global_cache::GlobalContractCache; -use chrono::Utc; #[cfg(test)] use mockall::automock; use papyrus_storage::state::{StateStorageReader, StateStorageWriter}; -use starknet_api::block::{BlockHashAndNumber, BlockNumber}; +use starknet_api::block::BlockNumber; use starknet_api::executable_transaction::Transaction; use starknet_api::state::ThinStateDiff; use starknet_batcher_types::batcher_types::{ @@ -33,26 +31,25 @@ use starknet_sequencer_infra::component_definitions::ComponentStarter; use tracing::{debug, error, info, instrument, trace}; use crate::block_builder::{ - BlockBuilderError, BlockBuilderExecutionParams, BlockBuilderFactory, BlockBuilderFactoryTrait, BlockMetadata, }; use crate::config::BatcherConfig; -use crate::proposal_manager::{ - GenerateProposalError, - ProposalError, - ProposalManager, - ProposalManagerTrait, - ProposalOutput, - ProposalResult, -}; +use crate::proposal_manager::{GenerateProposalError, ProposalManager, ProposalManagerTrait}; use crate::transaction_provider::{ DummyL1ProviderClient, ProposeTransactionProvider, ValidateTransactionProvider, }; +use crate::utils::{ + deadline_as_instant, + proposal_status_from, + verify_block_input, + ProposalOutput, + ProposalResult, +}; type OutputStreamReceiver = tokio::sync::mpsc::UnboundedReceiver; type InputStreamSender = tokio::sync::mpsc::Sender; @@ -458,65 +455,4 @@ impl From for BatcherError { } } -impl From for BatcherError { - fn from(err: ProposalError) -> Self { - match err { - ProposalError::BlockBuilderError(..) => BatcherError::InternalError, - ProposalError::Aborted => BatcherError::ProposalAborted, - } - } -} - impl ComponentStarter for Batcher {} - -pub fn deadline_as_instant(deadline: chrono::DateTime) -> BatcherResult { - let time_to_deadline = deadline - chrono::Utc::now(); - let as_duration = - time_to_deadline.to_std().map_err(|_| BatcherError::TimeToDeadlineError { deadline })?; - Ok((std::time::Instant::now() + as_duration).into()) -} - -fn verify_block_input( - height: BlockNumber, - block_number: BlockNumber, - retrospective_block_hash: Option, -) -> BatcherResult<()> { - verify_non_empty_retrospective_block_hash(height, retrospective_block_hash)?; - verify_block_number(height, block_number)?; - Ok(()) -} - -fn verify_non_empty_retrospective_block_hash( - height: BlockNumber, - retrospective_block_hash: Option, -) -> BatcherResult<()> { - if height >= BlockNumber(constants::STORED_BLOCK_HASH_BUFFER) - && retrospective_block_hash.is_none() - { - return Err(BatcherError::MissingRetrospectiveBlockHash); - } - Ok(()) -} - -fn verify_block_number(height: BlockNumber, block_number: BlockNumber) -> BatcherResult<()> { - if block_number != height { - return Err(BatcherError::InvalidBlockNumber { active_height: height, block_number }); - } - Ok(()) -} - -// Return the appropriate ProposalStatus for a given ProposalError. -fn proposal_status_from(proposal_error: ProposalError) -> BatcherResult { - match proposal_error { - ProposalError::BlockBuilderError(err) => { - if let BlockBuilderError::FailOnError(_) = err.as_ref() { - // The proposal either failed due to bad input (e.g. invalid transactions), or - // couldn't finish in time. - Ok(ProposalStatus::InvalidProposal) - } else { - Err(BatcherError::InternalError) - } - } - ProposalError::Aborted => Err(BatcherError::ProposalAborted), - } -} diff --git a/crates/starknet_batcher/src/batcher_test.rs b/crates/starknet_batcher/src/batcher_test.rs index 5f62087bc8..b1d61b855e 100644 --- a/crates/starknet_batcher/src/batcher_test.rs +++ b/crates/starknet_batcher/src/batcher_test.rs @@ -48,15 +48,10 @@ use crate::block_builder::{ MockBlockBuilderTrait, }; use crate::config::BatcherConfig; -use crate::proposal_manager::{ - GenerateProposalError, - ProposalError, - ProposalManagerTrait, - ProposalOutput, - ProposalResult, -}; +use crate::proposal_manager::{GenerateProposalError, ProposalManagerTrait}; use crate::test_utils::test_txs; use crate::transaction_provider::NextTxs; +use crate::utils::{ProposalError, ProposalOutput, ProposalResult}; const INITIAL_HEIGHT: BlockNumber = BlockNumber(3); const STREAMING_CHUNK_SIZE: usize = 3; diff --git a/crates/starknet_batcher/src/lib.rs b/crates/starknet_batcher/src/lib.rs index f25938de6e..199f354469 100644 --- a/crates/starknet_batcher/src/lib.rs +++ b/crates/starknet_batcher/src/lib.rs @@ -16,6 +16,7 @@ mod transaction_executor; mod transaction_provider; #[cfg(test)] mod transaction_provider_test; +mod utils; // Re-export so it can be used in the general config of the sequencer node without depending on // blockifier. diff --git a/crates/starknet_batcher/src/proposal_manager.rs b/crates/starknet_batcher/src/proposal_manager.rs index 69cc25540e..055a532f32 100644 --- a/crates/starknet_batcher/src/proposal_manager.rs +++ b/crates/starknet_batcher/src/proposal_manager.rs @@ -1,18 +1,14 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; use async_trait::async_trait; -use indexmap::IndexMap; -use starknet_api::block_hash::state_diff_hash::calculate_state_diff_hash; -use starknet_api::core::{ContractAddress, Nonce}; -use starknet_api::state::ThinStateDiff; -use starknet_api::transaction::TransactionHash; -use starknet_batcher_types::batcher_types::{ProposalCommitment, ProposalId}; +use starknet_batcher_types::batcher_types::ProposalId; use thiserror::Error; use tokio::sync::Mutex; use tracing::{debug, error, info, instrument, Instrument}; -use crate::block_builder::{BlockBuilderError, BlockBuilderTrait, BlockExecutionArtifacts}; +use crate::block_builder::{BlockBuilderError, BlockBuilderTrait}; +use crate::utils::{ProposalError, ProposalOutput, ProposalResult, ProposalTask}; #[derive(Debug, Error)] pub enum GenerateProposalError { @@ -32,14 +28,6 @@ pub enum GenerateProposalError { ProposalAlreadyExists { proposal_id: ProposalId }, } -#[derive(Clone, Debug, Error)] -pub enum ProposalError { - #[error(transparent)] - BlockBuilderError(Arc), - #[error("Proposal was aborted")] - Aborted, -} - #[async_trait] pub trait ProposalManagerTrait: Send + Sync { async fn spawn_proposal( @@ -68,12 +56,6 @@ pub trait ProposalManagerTrait: Send + Sync { async fn reset(&mut self); } -// Represents a spawned task of building new block proposal. -struct ProposalTask { - abort_signal_sender: tokio::sync::oneshot::Sender<()>, - join_handle: tokio::task::JoinHandle<()>, -} - /// Main struct for handling block proposals. /// Taking care of: /// - Proposing new blocks. @@ -91,16 +73,6 @@ pub(crate) struct ProposalManager { executed_proposals: Arc>>>, } -pub type ProposalResult = Result; - -#[derive(Debug, Default, PartialEq)] -pub struct ProposalOutput { - pub state_diff: ThinStateDiff, - pub commitment: ProposalCommitment, - pub tx_hashes: HashSet, - pub nonces: HashMap, -} - #[async_trait] impl ProposalManagerTrait for ProposalManager { /// Starts a new block proposal generation task for the given proposal_id. @@ -227,33 +199,3 @@ impl ProposalManager { } } } - -impl From for ProposalOutput { - fn from(artifacts: BlockExecutionArtifacts) -> Self { - let commitment_state_diff = artifacts.commitment_state_diff; - let nonces = HashMap::from_iter( - commitment_state_diff - .address_to_nonce - .iter() - .map(|(address, nonce)| (*address, *nonce)), - ); - - // TODO: Get these from the transactions. - let deployed_contracts = IndexMap::new(); - let declared_classes = IndexMap::new(); - let state_diff = ThinStateDiff { - deployed_contracts, - storage_diffs: commitment_state_diff.storage_updates, - declared_classes, - nonces: commitment_state_diff.address_to_nonce, - // TODO: Remove this when the structure of storage diffs changes. - deprecated_declared_classes: Vec::new(), - replaced_classes: IndexMap::new(), - }; - let commitment = - ProposalCommitment { state_diff_commitment: calculate_state_diff_hash(&state_diff) }; - let tx_hashes = HashSet::from_iter(artifacts.execution_infos.keys().copied()); - - Self { state_diff, commitment, tx_hashes, nonces } - } -} diff --git a/crates/starknet_batcher/src/proposal_manager_test.rs b/crates/starknet_batcher/src/proposal_manager_test.rs index 2f92a8492b..67646a6f28 100644 --- a/crates/starknet_batcher/src/proposal_manager_test.rs +++ b/crates/starknet_batcher/src/proposal_manager_test.rs @@ -4,13 +4,8 @@ use starknet_api::executable_transaction::Transaction; use starknet_batcher_types::batcher_types::ProposalId; use crate::block_builder::{BlockBuilderTrait, BlockExecutionArtifacts, MockBlockBuilderTrait}; -use crate::proposal_manager::{ - GenerateProposalError, - ProposalError, - ProposalManager, - ProposalManagerTrait, - ProposalOutput, -}; +use crate::proposal_manager::{GenerateProposalError, ProposalManager, ProposalManagerTrait}; +use crate::utils::{ProposalError, ProposalOutput}; const BLOCK_GENERATION_TIMEOUT: tokio::time::Duration = tokio::time::Duration::from_secs(1); diff --git a/crates/starknet_batcher/src/utils.rs b/crates/starknet_batcher/src/utils.rs new file mode 100644 index 0000000000..18c1d48343 --- /dev/null +++ b/crates/starknet_batcher/src/utils.rs @@ -0,0 +1,137 @@ +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; + +use blockifier::abi::constants; +use chrono::Utc; +use indexmap::IndexMap; +use starknet_api::block::{BlockHashAndNumber, BlockNumber}; +use starknet_api::block_hash::state_diff_hash::calculate_state_diff_hash; +use starknet_api::core::{ContractAddress, Nonce}; +use starknet_api::state::ThinStateDiff; +use starknet_api::transaction::TransactionHash; +use starknet_batcher_types::batcher_types::{BatcherResult, ProposalCommitment, ProposalStatus}; +use starknet_batcher_types::errors::BatcherError; +use thiserror::Error; + +use crate::block_builder::{BlockBuilderError, BlockExecutionArtifacts}; + +pub(crate) type ProposalResult = Result; + +// Represents an error completing a proposal. +#[derive(Clone, Debug, Error)] +pub(crate) enum ProposalError { + #[error(transparent)] + BlockBuilderError(Arc), + #[error("Proposal was aborted")] + Aborted, +} + +impl From for BatcherError { + fn from(err: ProposalError) -> Self { + match err { + ProposalError::BlockBuilderError(..) => BatcherError::InternalError, + ProposalError::Aborted => BatcherError::ProposalAborted, + } + } +} + +// Represents a spawned task of building new block proposal. +pub(crate) struct ProposalTask { + pub abort_signal_sender: tokio::sync::oneshot::Sender<()>, + pub join_handle: tokio::task::JoinHandle<()>, +} + +#[derive(Debug, Default, PartialEq)] +pub(crate) struct ProposalOutput { + pub state_diff: ThinStateDiff, + pub commitment: ProposalCommitment, + pub tx_hashes: HashSet, + pub nonces: HashMap, +} + +impl From for ProposalOutput { + fn from(artifacts: BlockExecutionArtifacts) -> Self { + let commitment_state_diff = artifacts.commitment_state_diff; + let nonces = HashMap::from_iter( + commitment_state_diff + .address_to_nonce + .iter() + .map(|(address, nonce)| (*address, *nonce)), + ); + + // TODO: Get these from the transactions. + let deployed_contracts = IndexMap::new(); + let declared_classes = IndexMap::new(); + let state_diff = ThinStateDiff { + deployed_contracts, + storage_diffs: commitment_state_diff.storage_updates, + declared_classes, + nonces: commitment_state_diff.address_to_nonce, + // TODO: Remove this when the structure of storage diffs changes. + deprecated_declared_classes: Vec::new(), + replaced_classes: IndexMap::new(), + }; + let commitment = + ProposalCommitment { state_diff_commitment: calculate_state_diff_hash(&state_diff) }; + let tx_hashes = HashSet::from_iter(artifacts.execution_infos.keys().copied()); + + Self { state_diff, commitment, tx_hashes, nonces } + } +} + +pub(crate) fn deadline_as_instant( + deadline: chrono::DateTime, +) -> BatcherResult { + let time_to_deadline = deadline - chrono::Utc::now(); + let as_duration = + time_to_deadline.to_std().map_err(|_| BatcherError::TimeToDeadlineError { deadline })?; + Ok((std::time::Instant::now() + as_duration).into()) +} + +pub(crate) fn verify_block_input( + height: BlockNumber, + block_number: BlockNumber, + retrospective_block_hash: Option, +) -> BatcherResult<()> { + verify_non_empty_retrospective_block_hash(height, retrospective_block_hash)?; + verify_block_number(height, block_number)?; + Ok(()) +} + +pub(crate) fn verify_non_empty_retrospective_block_hash( + height: BlockNumber, + retrospective_block_hash: Option, +) -> BatcherResult<()> { + if height >= BlockNumber(constants::STORED_BLOCK_HASH_BUFFER) + && retrospective_block_hash.is_none() + { + return Err(BatcherError::MissingRetrospectiveBlockHash); + } + Ok(()) +} + +pub(crate) fn verify_block_number( + height: BlockNumber, + block_number: BlockNumber, +) -> BatcherResult<()> { + if block_number != height { + return Err(BatcherError::InvalidBlockNumber { active_height: height, block_number }); + } + Ok(()) +} + +// Return the appropriate ProposalStatus for a given ProposalError. +pub(crate) fn proposal_status_from(proposal_error: ProposalError) -> BatcherResult { + match proposal_error { + ProposalError::BlockBuilderError(err) => { + if let BlockBuilderError::FailOnError(_) = err.as_ref() { + // The proposal either failed due to bad input (e.g. invalid transactions), or + // couldn't finish in time. + Ok(ProposalStatus::InvalidProposal) + } else { + Err(BatcherError::InternalError) + } + } + ProposalError::Aborted => Err(BatcherError::ProposalAborted), + } +}