From ba483895a5cfa19a9efbcd716cb8a810a03766fb 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 the validate flow --- crates/starknet_batcher/src/batcher_test.rs | 2 +- crates/starknet_batcher/src/block_builder.rs | 16 ++++--- .../src/block_builder_test.rs | 48 +++++++++++++++---- 3 files changed, 50 insertions(+), 16 deletions(-) diff --git a/crates/starknet_batcher/src/batcher_test.rs b/crates/starknet_batcher/src/batcher_test.rs index 73f86e0a97..486514733b 100644 --- a/crates/starknet_batcher/src/batcher_test.rs +++ b/crates/starknet_batcher/src/batcher_test.rs @@ -262,7 +262,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(TransactionExecutorError::BlockFull.to_string()), )); 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..7b381f9e68 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,11 +47,9 @@ 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), + FailOnError(String), #[error("The block builder was aborted.")] Aborted, } @@ -120,7 +117,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("deadline reached".to_string())); + } + break; + } if self.abort_signal_receiver.try_recv().is_ok() { info!("Received abort signal. Aborting block builder."); return Err(BlockBuilderError::Aborted); @@ -189,7 +193,7 @@ async fn collect_execution_results_and_stream_txs( Err(err) => { debug!("Transaction {:?} failed with error: {}.", input_tx, err); if fail_on_err { - return Err(BlockBuilderError::FailOnError(err)); + return Err(BlockBuilderError::FailOnError(err.to_string())); } } } diff --git a/crates/starknet_batcher/src/block_builder_test.rs b/crates/starknet_batcher/src/block_builder_test.rs index 0f6f690645..f28e5415a1 100644 --- a/crates/starknet_batcher/src/block_builder_test.rs +++ b/crates/starknet_batcher/src/block_builder_test.rs @@ -152,21 +152,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 +422,32 @@ async fn test_validate_block_with_error() { .unwrap_err(); assert_matches!( - result, - BlockBuilderError::FailOnError(BlockifierTransactionExecutorError::BlockFull) + result, BlockBuilderError::FailOnError(err) + if err == BlockifierTransactionExecutorError::BlockFull.to_string() + ); +} + +#[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 err == *"deadline reached" ); }