Merge pull request #1610 from eqlabs/fix/blockifier-error-propagation

fix: work-around opaque execution errors

CHANGELOG.md

@@ -7,6 +7,12 @@ More expansive patch notes and explanations may be found in the specific [pathfi
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+### Fixed
+
+- Execution errors are opaque and don't always include the root cause.
+
 ## [0.10.0] - 2023-11-29
 
 ### Added

crates/executor/src/error.rs

@@ -81,12 +81,6 @@ pub enum TransactionExecutionError {
     Custom(anyhow::Error),
 }
 
-impl From<BlockifierTransactionExecutionError> for TransactionExecutionError {
-    fn from(e: BlockifierTransactionExecutionError) -> Self {
-        Self::Custom(e.into())
-    }
-}
-
 impl From<StateError> for TransactionExecutionError {
     fn from(e: StateError) -> Self {
         match e {
@@ -107,3 +101,81 @@ impl From<anyhow::Error> for TransactionExecutionError {
         Self::Internal(value)
     }
 }
+
+impl TransactionExecutionError {
+    pub fn new(transaction_index: usize, error: BlockifierTransactionExecutionError) -> Self {
+        Self::ExecutionError {
+            transaction_index,
+            error: match &error {
+                // Some variants don't propagate their child's error so we do this manually until it is
+                // fixed in the blockifier. We have a test to ensure we don't miss fix.
+                BlockifierTransactionExecutionError::ContractConstructorExecutionFailed(x) => {
+                    format!("{error} {x}")
+                }
+                BlockifierTransactionExecutionError::ExecutionError(x) => format!("{error} {x}"),
+                BlockifierTransactionExecutionError::ValidateTransactionError(x) => {
+                    format!("{error} {x}")
+                }
+                other => other.to_string(),
+            },
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    mod transaction_errors_are_mapped_correctly {
+        //! Some variants in the blockifier are opaque and omit the inner error's data. We've patched this manually
+        //! and this tests ensures we don't accidentally stutter once the blockifier fixes this.
+        use super::*;
+        use blockifier::execution::errors::EntryPointExecutionError;
+
+        #[test]
+        fn contract_constructor_execution_failed() {
+            let child = EntryPointExecutionError::RecursionDepthExceeded;
+            let expected = format!("Contract constructor execution has failed. {child}");
+
+            let err =
+                BlockifierTransactionExecutionError::ContractConstructorExecutionFailed(child);
+            let err = TransactionExecutionError::new(0, err);
+            let err = match err {
+                TransactionExecutionError::ExecutionError { error, .. } => error,
+                _ => unreachable!("unexpected variant"),
+            };
+
+            assert_eq!(err, expected);
+        }
+
+        #[test]
+        fn execution_error() {
+            let child = EntryPointExecutionError::RecursionDepthExceeded;
+            let expected = format!("Transaction execution has failed. {child}");
+
+            let err = BlockifierTransactionExecutionError::ExecutionError(child);
+            let err = TransactionExecutionError::new(0, err);
+            let err = match err {
+                TransactionExecutionError::ExecutionError { error, .. } => error,
+                _ => unreachable!("unexpected variant"),
+            };
+
+            assert_eq!(err, expected);
+        }
+
+        #[test]
+        fn validate_transaction_error() {
+            let child = EntryPointExecutionError::RecursionDepthExceeded;
+            let expected = format!("Transaction validation has failed. {child}");
+
+            let err = BlockifierTransactionExecutionError::ValidateTransactionError(child);
+            let err = TransactionExecutionError::new(0, err);
+            let err = match err {
+                TransactionExecutionError::ExecutionError { error, .. } => error,
+                _ => unreachable!("unexpected variant"),
+            };
+
+            assert_eq!(err, expected);
+        }
+    }
+}

crates/executor/src/estimate.rs

@@ -59,10 +59,7 @@ pub fn estimate(
             }
             Err(error) => {
                 tracing::debug!(%error, %transaction_idx, "Transaction estimation failed");
-                return Err(TransactionExecutionError::ExecutionError {
-                    transaction_index: transaction_idx,
-                    error: error.to_string(),
-                });
+                return Err(TransactionExecutionError::new(transaction_idx, error));
             }
         }
     }

crates/executor/src/simulate.rs

@@ -84,15 +84,12 @@ pub fn simulate(
                         gas_price,
                         overall_fee: tx_info.actual_fee.0.into(),
                     },
-                    trace: to_trace(transaction_type, tx_info, state_diff)?,
+                    trace: to_trace(transaction_type, tx_info, state_diff),
                 });
             }
             Err(error) => {
                 tracing::debug!(%error, %transaction_idx, "Transaction simulation failed");
-                return Err(TransactionExecutionError::ExecutionError {
-                    transaction_index: transaction_idx,
-                    error: error.to_string(),
-                });
+                return Err(TransactionExecutionError::new(transaction_idx, error));
             }
         }
     }
@@ -125,7 +122,7 @@ pub fn trace_one(
         let state_diff = to_state_diff(&mut tx_state, tx_declared_deprecated_class_hash)?;
         tx_state.commit();
 
-        let trace = to_trace(tx_type, tx_info, state_diff)?;
+        let trace = to_trace(tx_type, tx_info, state_diff);
         if hash == target_transaction_hash {
             return Ok(trace);
         }
@@ -163,7 +160,7 @@ pub fn trace_all(
         let state_diff = to_state_diff(&mut tx_state, tx_declared_deprecated_class_hash)?;
         tx_state.commit();
 
-        let trace = to_trace(tx_type, tx_info, state_diff)?;
+        let trace = to_trace(tx_type, tx_info, state_diff);
         ret.push((hash, trace));
     }
 
@@ -290,21 +287,12 @@ fn to_trace(
     transaction_type: TransactionType,
     execution_info: blockifier::transaction::objects::TransactionExecutionInfo,
     state_diff: StateDiff,
-) -> Result<TransactionTrace, blockifier::transaction::errors::TransactionExecutionError> {
-    let validate_invocation = execution_info
-        .validate_call_info
-        .map(TryInto::try_into)
-        .transpose()?;
-    let maybe_function_invocation = execution_info
-        .execute_call_info
-        .map(TryInto::try_into)
-        .transpose();
-    let fee_transfer_invocation = execution_info
-        .fee_transfer_call_info
-        .map(TryInto::try_into)
-        .transpose()?;
-
-    let trace = match transaction_type {
+) -> TransactionTrace {
+    let validate_invocation = execution_info.validate_call_info.map(Into::into);
+    let maybe_function_invocation = execution_info.execute_call_info.map(Into::into);
+    let fee_transfer_invocation = execution_info.fee_transfer_call_info.map(Into::into);
+
+    match transaction_type {
         TransactionType::Declare => TransactionTrace::Declare(DeclareTransactionTrace {
             validate_invocation,
             fee_transfer_invocation,
@@ -313,7 +301,7 @@ fn to_trace(
         TransactionType::DeployAccount => {
             TransactionTrace::DeployAccount(DeployAccountTransactionTrace {
                 validate_invocation,
-                constructor_invocation: maybe_function_invocation?,
+                constructor_invocation: maybe_function_invocation,
                 fee_transfer_invocation,
                 state_diff,
             })
@@ -323,16 +311,14 @@ fn to_trace(
             execute_invocation: if let Some(reason) = execution_info.revert_error {
                 ExecuteInvocation::RevertedReason(reason)
             } else {
-                ExecuteInvocation::FunctionInvocation(maybe_function_invocation?)
+                ExecuteInvocation::FunctionInvocation(maybe_function_invocation)
             },
             fee_transfer_invocation,
             state_diff,
         }),
         TransactionType::L1Handler => TransactionTrace::L1Handler(L1HandlerTransactionTrace {
-            function_invocation: maybe_function_invocation?,
+            function_invocation: maybe_function_invocation,
             state_diff,
         }),
-    };
-
-    Ok(trace)
+    }
 }

crates/executor/src/types.rs

@@ -174,19 +174,11 @@ pub struct ExecutionResources {
     pub segment_arena_builtin: usize,
 }
 
-impl TryFrom<blockifier::execution::call_info::CallInfo> for FunctionInvocation {
-    type Error = blockifier::transaction::errors::TransactionExecutionError;
-
-    fn try_from(
-        call_info: blockifier::execution::call_info::CallInfo,
-    ) -> Result<Self, Self::Error> {
+impl From<blockifier::execution::call_info::CallInfo> for FunctionInvocation {
+    fn from(call_info: blockifier::execution::call_info::CallInfo) -> Self {
         let messages = ordered_l2_to_l1_messages(&call_info);
 
-        let internal_calls = call_info
-            .inner_calls
-            .into_iter()
-            .map(TryInto::try_into)
-            .collect::<Result<Vec<_>, _>>()?;
+        let internal_calls = call_info.inner_calls.into_iter().map(Into::into).collect();
 
         let events = call_info
             .execution
@@ -203,7 +195,7 @@ impl TryFrom<blockifier::execution::call_info::CallInfo> for FunctionInvocation
             .map(IntoFelt::into_felt)
             .collect();
 
-        Ok(Self {
+        Self {
             calldata: call_info
                 .call
                 .calldata
@@ -227,7 +219,7 @@ impl TryFrom<blockifier::execution::call_info::CallInfo> for FunctionInvocation
             messages,
             result,
             execution_resources: call_info.vm_resources.into(),
-        })
+        }
     }
 }