fix(batcher): treat deadline as error in validate flow

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()

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<Transaction>),
-    #[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<T> = Result<T, BlockBuilderError>;
 
+#[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),
+                    ));
                 }
             }
         }

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<Transaction>,
+) -> 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)
     );
 }