From c1e599b7cdbe73c199edf0fa270866ffd052b470 Mon Sep 17 00:00:00 2001 From: Mirko von Leipzig Date: Mon, 4 Dec 2023 13:20:28 +0200 Subject: [PATCH 1/2] feat: CallInfo conversion should not be fallible --- crates/executor/src/error.rs | 6 ------ crates/executor/src/simulate.rs | 31 +++++++++++-------------------- crates/executor/src/types.rs | 16 ++++++---------- 3 files changed, 17 insertions(+), 36 deletions(-) diff --git a/crates/executor/src/error.rs b/crates/executor/src/error.rs index a06000ed46..c165024959 100644 --- a/crates/executor/src/error.rs +++ b/crates/executor/src/error.rs @@ -81,12 +81,6 @@ pub enum TransactionExecutionError { Custom(anyhow::Error), } -impl From for TransactionExecutionError { - fn from(e: BlockifierTransactionExecutionError) -> Self { - Self::Custom(e.into()) - } -} - impl From for TransactionExecutionError { fn from(e: StateError) -> Self { match e { diff --git a/crates/executor/src/simulate.rs b/crates/executor/src/simulate.rs index a36ec9dbcf..6d46499180 100644 --- a/crates/executor/src/simulate.rs +++ b/crates/executor/src/simulate.rs @@ -84,7 +84,7 @@ 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) => { @@ -125,7 +125,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 +163,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,19 +290,10 @@ fn to_trace( transaction_type: TransactionType, execution_info: blockifier::transaction::objects::TransactionExecutionInfo, state_diff: StateDiff, -) -> Result { - 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()?; +) -> 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); let trace = match transaction_type { TransactionType::Declare => TransactionTrace::Declare(DeclareTransactionTrace { @@ -313,7 +304,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 +314,16 @@ 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) + trace } diff --git a/crates/executor/src/types.rs b/crates/executor/src/types.rs index fd7d946b67..7618cd7149 100644 --- a/crates/executor/src/types.rs +++ b/crates/executor/src/types.rs @@ -174,19 +174,15 @@ pub struct ExecutionResources { pub segment_arena_builtin: usize, } -impl TryFrom for FunctionInvocation { - type Error = blockifier::transaction::errors::TransactionExecutionError; - - fn try_from( - call_info: blockifier::execution::call_info::CallInfo, - ) -> Result { +impl From 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::, _>>()?; + .map(Into::into) + .collect(); let events = call_info .execution @@ -203,7 +199,7 @@ impl TryFrom for FunctionInvocation .map(IntoFelt::into_felt) .collect(); - Ok(Self { + Self { calldata: call_info .call .calldata @@ -227,7 +223,7 @@ impl TryFrom for FunctionInvocation messages, result, execution_resources: call_info.vm_resources.into(), - }) + } } } From 09c11cd32c82f3ae24d1c06eb57767bc35ba6a28 Mon Sep 17 00:00:00 2001 From: Mirko von Leipzig Date: Mon, 4 Dec 2023 13:57:17 +0200 Subject: [PATCH 2/2] fix(rpc): workaround opaque blockifier errors Some blockifier errors are opaque and hide useful information. --- CHANGELOG.md | 6 +++ crates/executor/src/error.rs | 78 +++++++++++++++++++++++++++++++++ crates/executor/src/estimate.rs | 5 +-- crates/executor/src/simulate.rs | 11 ++--- crates/executor/src/types.rs | 6 +-- 5 files changed, 89 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd167ddd72..5deb80c0b2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/crates/executor/src/error.rs b/crates/executor/src/error.rs index c165024959..014dd6ba52 100644 --- a/crates/executor/src/error.rs +++ b/crates/executor/src/error.rs @@ -101,3 +101,81 @@ impl From 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); + } + } +} diff --git a/crates/executor/src/estimate.rs b/crates/executor/src/estimate.rs index e22ad89bf0..0295ce4a01 100644 --- a/crates/executor/src/estimate.rs +++ b/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)); } } } diff --git a/crates/executor/src/simulate.rs b/crates/executor/src/simulate.rs index 6d46499180..3e8a10ece1 100644 --- a/crates/executor/src/simulate.rs +++ b/crates/executor/src/simulate.rs @@ -89,10 +89,7 @@ pub fn simulate( } 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)); } } } @@ -295,7 +292,7 @@ fn to_trace( 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); - let trace = match transaction_type { + match transaction_type { TransactionType::Declare => TransactionTrace::Declare(DeclareTransactionTrace { validate_invocation, fee_transfer_invocation, @@ -323,7 +320,5 @@ fn to_trace( function_invocation: maybe_function_invocation, state_diff, }), - }; - - trace + } } diff --git a/crates/executor/src/types.rs b/crates/executor/src/types.rs index 7618cd7149..e5fe0f038f 100644 --- a/crates/executor/src/types.rs +++ b/crates/executor/src/types.rs @@ -178,11 +178,7 @@ impl From 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(Into::into) - .collect(); + let internal_calls = call_info.inner_calls.into_iter().map(Into::into).collect(); let events = call_info .execution