From 4e7676241f723df1e9cc1634d1e99b71c97dd558 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 | 35 ++++++++--- .../src/block_builder_test.rs | 62 ++++++++++--------- 3 files changed, 62 insertions(+), 40 deletions(-) diff --git a/crates/starknet_batcher/src/batcher_test.rs b/crates/starknet_batcher/src/batcher_test.rs index 05aede35bf..499cbcefaf 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..2c616ef528 100644 --- a/crates/starknet_batcher/src/block_builder.rs +++ b/crates/starknet_batcher/src/block_builder.rs @@ -14,7 +14,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 +47,25 @@ 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(transparent)] + FailOnError(FailOnErrorCause), #[error("The block builder was aborted.")] Aborted, } pub type BlockBuilderResult = Result; +#[derive(Debug, Error)] +pub enum FailOnErrorCause { + #[error("Block is full")] + BlockFull, + #[error("Deadline has been reached")] + DeadlineReached, + #[error("Transaction failed: {0}")] + TransactionFailed(BlockifierTransactionExecutorError), +} + #[cfg_attr(test, derive(Clone))] #[derive(Debug, PartialEq)] pub struct BlockExecutionArtifacts { @@ -120,7 +127,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 +196,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..7a2f0cb96f 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; @@ -131,18 +129,22 @@ fn empty_block_test_expectations() (mock_transaction_executor, mock_tx_provider, expected_block_artifacts) } -fn block_full_test_expectations( - input_txs: &[Transaction], - block_size: usize, -) -> (MockTransactionExecutorTrait, MockTransactionProvider, BlockExecutionArtifacts) { +fn mock_transaction_executor_block_full(input_txs: &[Transaction]) -> MockTransactionExecutorTrait { let input_txs_cloned = input_txs.to_vec(); let mut mock_transaction_executor = MockTransactionExecutorTrait::new(); - mock_transaction_executor .expect_add_txs_to_block() .times(1) .withf(move |blockifier_input| compare_tx_hashes(&input_txs_cloned, blockifier_input)) .return_once(move |_| vec![Ok(execution_info()), Err(TransactionExecutorError::BlockFull)]); + mock_transaction_executor +} + +fn block_full_test_expectations( + input_txs: &[Transaction], + block_size: usize, +) -> (MockTransactionExecutorTrait, MockTransactionProvider, BlockExecutionArtifacts) { + let mut mock_transaction_executor = mock_transaction_executor_block_full(input_txs); let expected_block_artifacts = set_close_block_expectations(&mut mock_transaction_executor, block_size); @@ -152,26 +154,30 @@ 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: &[Transaction]) -> MockTransactionExecutorTrait { + let input_txs_cloned = input_txs.to_vec(); 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_cloned, 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 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); - let mock_tx_provider = mock_tx_provider_limited_calls(1, vec![first_chunk_copy]); + let mock_tx_provider = mock_tx_provider_limited_calls(1, vec![first_chunk]); (mock_transaction_executor, mock_tx_provider, expected_block_artifacts) } @@ -388,17 +394,15 @@ async fn test_validate_block() { assert_eq!(result_block_artifacts, expected_block_artifacts); } +#[rstest] +#[case::block_full(test_txs(0..3), mock_transaction_executor_block_full(&input_txs), FailOnErrorCause::BlockFull)] +#[case::deadline_reached(test_txs(0..3), mock_transaction_executor_with_delay(&input_txs), FailOnErrorCause::DeadlineReached)] #[tokio::test] -async fn test_validate_block_with_error() { - let input_txs = test_txs(0..3); - let input_txs_clone = input_txs.clone(); - - let mut mock_transaction_executor = MockTransactionExecutorTrait::new(); - mock_transaction_executor - .expect_add_txs_to_block() - .times(1) - .withf(move |blockifier_input| compare_tx_hashes(&input_txs_clone, blockifier_input)) - .return_once(move |_| vec![Ok(execution_info()), Err(TransactionExecutorError::BlockFull)]); +async fn test_validate_block_with_error( + #[case] input_txs: Vec, + #[case] mut mock_transaction_executor: MockTransactionExecutorTrait, + #[case] expected_error: FailOnErrorCause, +) { mock_transaction_executor.expect_close_block().times(0); let mock_tx_provider = mock_tx_provider_limited_calls(1, vec![input_txs.to_vec()]); @@ -416,8 +420,8 @@ async fn test_validate_block_with_error() { .unwrap_err(); assert_matches!( - result, - BlockBuilderError::FailOnError(BlockifierTransactionExecutorError::BlockFull) + result, BlockBuilderError::FailOnError(err) + if err.to_string() == expected_error.to_string() ); }