From d3224ef6200185f3742334edf371114f57319b99 Mon Sep 17 00:00:00 2001 From: Yael Doweck Date: Mon, 18 Nov 2024 16:00:05 +0200 Subject: [PATCH] fix(batcher): treat deadline as error in validate flow --- crates/starknet_batcher/src/batcher_test.rs | 5 +- crates/starknet_batcher/src/block_builder.rs | 43 ++++++++++++--- .../src/block_builder_test.rs | 54 ++++++++++++++----- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/crates/starknet_batcher/src/batcher_test.rs b/crates/starknet_batcher/src/batcher_test.rs index bf41a7b916..beff5bbf89 100644 --- a/crates/starknet_batcher/src/batcher_test.rs +++ b/crates/starknet_batcher/src/batcher_test.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use assert_matches::assert_matches; use async_trait::async_trait; -use blockifier::blockifier::transaction_executor::TransactionExecutorError; use chrono::Utc; use futures::future::BoxFuture; use futures::FutureExt; @@ -37,7 +36,7 @@ use starknet_mempool_types::communication::MockMempoolClient; use starknet_mempool_types::mempool_types::CommitBlockArgs; use crate::batcher::{Batcher, MockBatcherStorageReaderTrait, MockBatcherStorageWriterTrait}; -use crate::block_builder::BlockBuilderError; +use crate::block_builder::{BlockBuilderError, FailOnErrorCause}; use crate::config::BatcherConfig; use crate::proposal_manager::{ GenerateProposalError, @@ -262,7 +261,7 @@ async fn send_finish_to_an_invalid_proposal() { .return_once(|_, _, _, _| { async move { Ok(()) } }.boxed()); let proposal_error = GetProposalResultError::BlockBuilderError(Arc::new( - BlockBuilderError::FailOnError(TransactionExecutorError::BlockFull), + BlockBuilderError::FailOnError(FailOnErrorCause::BlockFull), )); proposal_manager .expect_wrap_await_proposal_commitment() diff --git a/crates/starknet_batcher/src/block_builder.rs b/crates/starknet_batcher/src/block_builder.rs index a16e974eee..9787aecb12 100644 --- a/crates/starknet_batcher/src/block_builder.rs +++ b/crates/starknet_batcher/src/block_builder.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::fmt; use async_trait::async_trait; use blockifier::blockifier::block::{BlockInfo, GasPrices}; @@ -14,7 +15,6 @@ use blockifier::context::{BlockContext, ChainInfo}; use blockifier::state::cached_state::CommitmentStateDiff; use blockifier::state::errors::StateError; use blockifier::state::global_cache::GlobalContractCache; -use blockifier::transaction::errors::TransactionExecutionError as BlockifierTransactionExecutionError; use blockifier::transaction::objects::TransactionExecutionInfo; use blockifier::transaction::transaction_execution::Transaction as BlockifierTransaction; use blockifier::versioned_constants::{VersionedConstants, VersionedConstantsOverrides}; @@ -48,17 +48,32 @@ pub enum BlockBuilderError { #[error(transparent)] GetTransactionError(#[from] TransactionProviderError), #[error(transparent)] - TransactionExecutionError(#[from] BlockifierTransactionExecutionError), - #[error(transparent)] StreamTransactionsError(#[from] tokio::sync::mpsc::error::SendError), - #[error("Build block with fail_on_err mode, failed on error {}.", _0)] - FailOnError(BlockifierTransactionExecutorError), + #[error("Build block with fail_on_err mode, failed on error {0}.")] + FailOnError(FailOnErrorCause), #[error("The block builder was aborted.")] Aborted, } pub type BlockBuilderResult = Result; +#[derive(Debug)] +pub enum FailOnErrorCause { + BlockFull, + DeadlineReached, + TransactionFailed(BlockifierTransactionExecutorError), +} + +impl fmt::Display for FailOnErrorCause { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + FailOnErrorCause::BlockFull => write!(f, "Block is full"), + FailOnErrorCause::DeadlineReached => write!(f, "Deadline was reached"), + FailOnErrorCause::TransactionFailed(err) => write!(f, "Transaction failed: {}", err), + } + } +} + #[cfg_attr(test, derive(Clone))] #[derive(Debug, PartialEq)] pub struct BlockExecutionArtifacts { @@ -120,7 +135,14 @@ impl BlockBuilderTrait for BlockBuilder { let mut block_is_full = false; let mut execution_infos = IndexMap::new(); // TODO(yael 6/10/2024): delete the timeout condition once the executor has a timeout - while !block_is_full && tokio::time::Instant::now() < self.execution_params.deadline { + while !block_is_full { + if tokio::time::Instant::now() >= self.execution_params.deadline { + info!("Block builder deadline reached."); + if self.execution_params.fail_on_err { + return Err(BlockBuilderError::FailOnError(FailOnErrorCause::DeadlineReached)); + } + break; + } if self.abort_signal_receiver.try_recv().is_ok() { info!("Received abort signal. Aborting block builder."); return Err(BlockBuilderError::Aborted); @@ -182,14 +204,19 @@ async fn collect_execution_results_and_stream_txs( } // TODO(yael 18/9/2024): add timeout error handling here once this // feature is added. - Err(BlockifierTransactionExecutorError::BlockFull) if !fail_on_err => { + Err(BlockifierTransactionExecutorError::BlockFull) => { info!("Block is full"); + if fail_on_err { + return Err(BlockBuilderError::FailOnError(FailOnErrorCause::BlockFull)); + } return Ok(true); } Err(err) => { debug!("Transaction {:?} failed with error: {}.", input_tx, err); if fail_on_err { - return Err(BlockBuilderError::FailOnError(err)); + return Err(BlockBuilderError::FailOnError( + FailOnErrorCause::TransactionFailed(err), + )); } } } diff --git a/crates/starknet_batcher/src/block_builder_test.rs b/crates/starknet_batcher/src/block_builder_test.rs index 0f6f690645..80c82b69f5 100644 --- a/crates/starknet_batcher/src/block_builder_test.rs +++ b/crates/starknet_batcher/src/block_builder_test.rs @@ -1,8 +1,5 @@ use assert_matches::assert_matches; -use blockifier::blockifier::transaction_executor::{ - TransactionExecutorError, - TransactionExecutorError as BlockifierTransactionExecutorError, -}; +use blockifier::blockifier::transaction_executor::TransactionExecutorError; use blockifier::bouncer::BouncerWeights; use blockifier::fee::fee_checks::FeeCheckError; use blockifier::transaction::objects::{RevertError, TransactionExecutionInfo}; @@ -24,6 +21,7 @@ use crate::block_builder::{ BlockBuilderResult, BlockBuilderTrait, BlockExecutionArtifacts, + FailOnErrorCause, }; use crate::test_utils::test_txs; use crate::transaction_executor::MockTransactionExecutorTrait; @@ -152,21 +150,27 @@ fn block_full_test_expectations( (mock_transaction_executor, mock_tx_provider, expected_block_artifacts) } -fn test_expectations_with_delay( - input_txs: &[Transaction], -) -> (MockTransactionExecutorTrait, MockTransactionProvider, BlockExecutionArtifacts) { - let first_chunk = input_txs[0..TX_CHUNK_SIZE].to_vec(); - let first_chunk_copy = first_chunk.clone(); +fn mock_transaction_executor_with_delay( + input_txs: Vec, +) -> MockTransactionExecutorTrait { let mut mock_transaction_executor = MockTransactionExecutorTrait::new(); - mock_transaction_executor .expect_add_txs_to_block() .times(1) - .withf(move |blockifier_input| compare_tx_hashes(&first_chunk, blockifier_input)) + .withf(move |blockifier_input| compare_tx_hashes(&input_txs, blockifier_input)) .return_once(move |_| { std::thread::sleep(std::time::Duration::from_secs(BLOCK_GENERATION_DEADLINE_SECS)); (0..TX_CHUNK_SIZE).map(move |_| Ok(execution_info())).collect() }); + mock_transaction_executor +} + +fn test_expectations_with_delay( + input_txs: &[Transaction], +) -> (MockTransactionExecutorTrait, MockTransactionProvider, BlockExecutionArtifacts) { + let first_chunk = input_txs[0..TX_CHUNK_SIZE].to_vec(); + let first_chunk_copy = first_chunk.clone(); + let mut mock_transaction_executor = mock_transaction_executor_with_delay(first_chunk); let expected_block_artifacts = set_close_block_expectations(&mut mock_transaction_executor, TX_CHUNK_SIZE); @@ -416,8 +420,32 @@ async fn test_validate_block_with_error() { .unwrap_err(); assert_matches!( - result, - BlockBuilderError::FailOnError(BlockifierTransactionExecutorError::BlockFull) + result, BlockBuilderError::FailOnError(err) + if matches!(err, FailOnErrorCause::BlockFull) + ); +} + +#[tokio::test] +async fn test_validate_block_deadline_reached() { + let input_txs = test_txs(0..3); + let mock_transaction_executor = mock_transaction_executor_with_delay(input_txs.clone()); + let mock_tx_provider = mock_tx_provider_limited_calls(1, vec![input_txs]); + + let (_abort_sender, abort_receiver) = tokio::sync::oneshot::channel(); + let result = run_build_block( + mock_transaction_executor, + mock_tx_provider, + None, + true, + abort_receiver, + BLOCK_GENERATION_DEADLINE_SECS, + ) + .await + .unwrap_err(); + + assert_matches!( + result, BlockBuilderError::FailOnError(err) + if matches!(err, FailOnErrorCause::DeadlineReached) ); }