From 36b4800063a4328db35ade3b655a444ef8c5c2de Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Sat, 19 Oct 2024 17:55:14 +0300 Subject: [PATCH] feat(blockifier): add address and selector for intermediate cairo1 revert errors --- crates/blockifier/src/execution/errors.rs | 2 +- .../blockifier/src/execution/stack_trace.rs | 44 +++++++++++- .../src/execution/stack_trace_test.rs | 72 +++++++++++++------ .../syscalls/syscall_tests/failure_format.rs | 30 ++++---- crates/blockifier/src/transaction/errors.rs | 2 +- 5 files changed, 105 insertions(+), 45 deletions(-) diff --git a/crates/blockifier/src/execution/errors.rs b/crates/blockifier/src/execution/errors.rs index 9c5e0e5d11d..922508b83f3 100644 --- a/crates/blockifier/src/execution/errors.rs +++ b/crates/blockifier/src/execution/errors.rs @@ -80,7 +80,7 @@ impl From for PostExecutionError { pub enum EntryPointExecutionError { #[error(transparent)] CairoRunError(#[from] CairoRunError), - #[error("Execution failed. Failure reason: {error_trace}.")] + #[error("Execution failed. Failure reason:\n{error_trace}.")] ExecutionFailed { error_trace: String }, #[error("Internal error: {0}")] InternalError(String), diff --git a/crates/blockifier/src/execution/stack_trace.rs b/crates/blockifier/src/execution/stack_trace.rs index 0c02444da89..9573fe33155 100644 --- a/crates/blockifier/src/execution/stack_trace.rs +++ b/crates/blockifier/src/execution/stack_trace.rs @@ -5,9 +5,10 @@ use cairo_vm::vm::errors::vm_errors::VirtualMachineError; use itertools::Itertools; use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector}; use starknet_api::execution_utils::format_panic_data; +use starknet_types_core::felt::Felt; use super::deprecated_syscalls::hint_processor::DeprecatedSyscallExecutionError; -use super::syscalls::hint_processor::SyscallExecutionError; +use super::syscalls::hint_processor::{SyscallExecutionError, ENTRYPOINT_FAILED_ERROR}; use crate::execution::call_info::CallInfo; use crate::execution::errors::{ConstructorEntryPointExecutionError, EntryPointExecutionError}; use crate::transaction::errors::TransactionExecutionError; @@ -154,8 +155,45 @@ impl ErrorStack { } } -pub fn extract_trailing_cairo1_revert_trace(root_callinfo: &CallInfo) -> String { - format_panic_data(&root_callinfo.execution.retdata.0) +pub fn extract_trailing_cairo1_revert_trace(root_call: &CallInfo) -> String { + let fallback_value = format_panic_data(&root_call.execution.retdata.0); + let entrypoint_failed_felt = Felt::from_hex(ENTRYPOINT_FAILED_ERROR) + .unwrap_or_else(|_| panic!("{ENTRYPOINT_FAILED_ERROR} does not fit in a felt.")); + + // Compute the failing call chain. + let mut error_calls: Vec<&CallInfo> = vec![]; + let mut next_call = Some(root_call); + while let Some(call) = next_call { + // It is possible that a failing contract managed to call another (non-failing) contract + // before hitting an error; stop iteration if the current call was successful. + if !call.execution.failed { + break; + } + error_calls.push(call); + // If the last felt in the retdata is not the failure felt, stop iteration. + // Even if the next inner call is also in failed state, assume a scenario where the current + // call panicked after ignoring the error result of the inner call. + if call.execution.retdata.0.last() != Some(&entrypoint_failed_felt) { + break; + } + // For stack trace extraction, the last call chain is all that's relevant: sibling calls are + // not a source of error. + next_call = call.inner_calls.last(); + } + + // Add one line per call, and append the failure reason. + let Some(last_call) = error_calls.last() else { return fallback_value }; + error_calls + .iter() + .map(|call_info| { + format!( + "Error in contract (contract address: {:#064x}, selector: {:#064x}):", + call_info.call.storage_address.0.key(), + call_info.call.entry_point_selector.0, + ) + }) + .chain([format_panic_data(&last_call.execution.retdata.0)]) + .join("\n") } /// Extracts the error trace from a `TransactionExecutionError`. This is a top level function. diff --git a/crates/blockifier/src/execution/stack_trace_test.rs b/crates/blockifier/src/execution/stack_trace_test.rs index dff6a9804c8..7d5b8e1d203 100644 --- a/crates/blockifier/src/execution/stack_trace_test.rs +++ b/crates/blockifier/src/execution/stack_trace_test.rs @@ -213,8 +213,14 @@ An ASSERT_EQ instruction failed: 1 != 0. "Transaction execution has failed: 0: Error in the called contract (contract address: {account_address_felt:#064x}, class hash: \ {account_contract_hash:#064x}, selector: {execute_selector_felt:#064x}): -Execution failed. Failure reason: (0x6661696c ('fail'), 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED'), 0x454e545259504f494e545f4641494c4544 ('ENTRYPOINT_FAILED')). +Execution failed. Failure reason: +Error in contract (contract address: {account_address_felt:#064x}, selector: \ + {execute_selector_felt:#064x}): +Error in contract (contract address: {test_contract_address_felt:#064x}, selector: \ + {external_entry_point_selector_felt:#064x}): +Error in contract (contract address: {test_contract_address_2_felt:#064x}, selector: \ + {inner_entry_point_selector_felt:#064x}): +0x6661696c ('fail'). " ); @@ -330,9 +336,14 @@ Unknown location (pc=0:{expected_pc1}) "Transaction execution has failed: 0: Error in the called contract (contract address: {account_address_felt:#064x}, class hash: \ {account_contract_hash:#064x}, selector: {execute_selector_felt:#064x}): -Execution failed. Failure reason: ({expected_error}, 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED'), 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED')). +Execution failed. Failure reason: +Error in contract (contract address: {account_address_felt:#064x}, selector: \ + {execute_selector_felt:#064x}): +Error in contract (contract address: {contract_address_felt:#064x}, selector: \ + {invoke_call_chain_selector_felt:#064x}): +Error in contract (contract address: {contract_address_felt:#064x}, selector: \ + {invoke_call_chain_selector_felt:#064x}): +{expected_error}. " ) } @@ -478,10 +489,16 @@ Unknown location (pc=0:{expected_pc3}) "Transaction execution has failed: 0: Error in the called contract (contract address: {account_address_felt:#064x}, class hash: \ {account_contract_hash:#064x}, selector: {execute_selector_felt:#064x}): -Execution failed. Failure reason: ({expected_error}, 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED'), 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED'), 0x454e545259504f494e545f4641494c4544 \ - ('ENTRYPOINT_FAILED')). +Execution failed. Failure reason: +Error in contract (contract address: {account_address_felt:#064x}, selector: \ + {execute_selector_felt:#064x}): +Error in contract (contract address: {address_felt:#064x}, selector: \ + {invoke_call_chain_selector_felt:#064x}): +Error in contract (contract address: {address_felt:#064x}, selector: \ + {invoke_call_chain_selector_felt:#064x}): +Error in contract (contract address: {address_felt:#064x}, selector: \ + {last_func_selector_felt:#064x}): +{expected_error}. " ) } @@ -578,9 +595,11 @@ An ASSERT_EQ instruction failed: 1 != 0. ", class_hash.0 ), - CairoVersion::Cairo1 => "The `validate` entry point panicked with \ - 0x496e76616c6964207363656e6172696f ('Invalid scenario')." - .into(), + CairoVersion::Cairo1 => format!( + "The `validate` entry point panicked with: +Error in contract (contract address: {contract_address:#064x}, selector: {selector:#064x}): +0x496e76616c6964207363656e6172696f ('Invalid scenario')." + ), }; // Clean pc locations from the trace. @@ -633,22 +652,26 @@ fn test_account_ctor_frame_stack_trace( hash: {:#064x}, selector: {expected_selector:#064x}): ", class_hash.0 - ) + match cairo_version { - CairoVersion::Cairo0 => { - "Error at pc=0:223: + ) + .to_string() + + &match cairo_version { + CairoVersion::Cairo0 => "Error at pc=0:223: Cairo traceback (most recent call last): Unknown location (pc=0:195) Unknown location (pc=0:179) An ASSERT_EQ instruction failed: 1 != 0. " - } - CairoVersion::Cairo1 => { - "Execution failed. Failure reason: 0x496e76616c6964207363656e6172696f ('Invalid \ - scenario'). + .to_string(), + CairoVersion::Cairo1 => format!( + "Execution failed. Failure reason: +Error in contract (contract address: {expected_address:#064x}, selector: \ + {expected_selector:#064x}): +0x496e76616c6964207363656e6172696f ('Invalid scenario'). " - } - }; + ) + .to_string(), + }; // Compare expected and actual error. let error = deploy_account_tx.execute(state, &block_context, true, true).unwrap_err(); @@ -774,10 +797,13 @@ Error at pc=0:{}: {frame_1} Error at pc=0:{}: {frame_2} -Execution failed. Failure reason: 0x496e76616c6964207363656e6172696f ('Invalid scenario'). +Execution failed. Failure reason: +Error in contract (contract address: {expected_address:#064x}, selector: {:#064x}): +0x496e76616c6964207363656e6172696f ('Invalid scenario'). ", execute_offset + 205, - deploy_offset + 194 + deploy_offset + 194, + ctor_selector.0 ) } }; diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/failure_format.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/failure_format.rs index 803da943815..f439d2cf6c5 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/failure_format.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/failure_format.rs @@ -1,4 +1,4 @@ -use cairo_lang_utils::byte_array::BYTE_ARRAY_MAGIC; +use starknet_api::core::{ContractAddress, EntryPointSelector}; use starknet_types_core::felt::Felt; use crate::execution::call_info::{CallExecution, CallInfo, Retdata}; @@ -7,21 +7,8 @@ use crate::execution::stack_trace::extract_trailing_cairo1_revert_trace; #[test] fn test_syscall_failure_format() { - let error_data = Retdata( - vec![ - // Magic to indicate that this is a byte array. - BYTE_ARRAY_MAGIC, - // The number of full words in the byte array. - "0x00", - // The pending word of the byte array: "Execution failure" - "0x457865637574696f6e206661696c757265", - // The length of the pending word. - "0x11", - ] - .into_iter() - .map(|x| Felt::from_hex(x).unwrap()) - .collect(), - ); + let execution_failure = "0x457865637574696f6e206661696c757265"; + let error_data = Retdata(vec![Felt::from_hex(execution_failure).unwrap()]); let callinfo = CallInfo { execution: CallExecution { retdata: error_data, failed: true, ..Default::default() }, ..Default::default() @@ -29,5 +16,14 @@ fn test_syscall_failure_format() { let error = EntryPointExecutionError::ExecutionFailed { error_trace: extract_trailing_cairo1_revert_trace(&callinfo), }; - assert_eq!(error.to_string(), "Execution failed. Failure reason: \"Execution failure\"."); + assert_eq!( + error.to_string(), + format!( + "Execution failed. Failure reason: +Error in contract (contract address: {:#064x}, selector: {:#064x}): +{execution_failure} ('Execution failure').", + ContractAddress::default().0.key(), + EntryPointSelector::default().0 + ) + ); } diff --git a/crates/blockifier/src/transaction/errors.rs b/crates/blockifier/src/transaction/errors.rs index f0f0066039a..adcf00744f6 100644 --- a/crates/blockifier/src/transaction/errors.rs +++ b/crates/blockifier/src/transaction/errors.rs @@ -103,7 +103,7 @@ pub enum TransactionExecutionError { FeeCheckError(#[from] FeeCheckError), #[error(transparent)] FromStr(#[from] FromStrError), - #[error("The `validate` entry point panicked with {panic_reason}.")] + #[error("The `validate` entry point panicked with:\n{panic_reason}.")] PanicInValidate { panic_reason: String }, #[error("The `validate` entry point should return `VALID`. Got {actual:?}.")] InvalidValidateReturnData { actual: Retdata },