From 00aeed87c99e54da850be714b2a5d5bcf1c8f899 Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Mon, 21 Oct 2024 10:40:19 +0300 Subject: [PATCH] feat(blockifier): cairo1 revert stack now part of ErrorStack --- .../blockifier/src/execution/entry_point.rs | 7 +- crates/blockifier/src/execution/errors.rs | 2 +- .../src/execution/execution_utils.rs | 6 +- .../blockifier/src/execution/stack_trace.rs | 65 ++++++++++++++++--- .../src/execution/stack_trace_test.rs | 26 +++++--- .../syscalls/syscall_tests/failure_format.rs | 7 +- .../src/transaction/account_transaction.rs | 7 +- crates/blockifier/src/transaction/errors.rs | 2 +- 8 files changed, 93 insertions(+), 29 deletions(-) diff --git a/crates/blockifier/src/execution/entry_point.rs b/crates/blockifier/src/execution/entry_point.rs index e251e15bfff..3f92c3b06c3 100644 --- a/crates/blockifier/src/execution/entry_point.rs +++ b/crates/blockifier/src/execution/entry_point.rs @@ -31,7 +31,7 @@ use crate::execution::errors::{ PreExecutionError, }; use crate::execution::execution_utils::execute_entry_point_call_wrapper; -use crate::execution::stack_trace::extract_trailing_cairo1_revert_trace; +use crate::execution::stack_trace::{extract_trailing_cairo1_revert_trace, Cairo1RevertHeader}; use crate::state::state_api::{State, StateResult}; use crate::transaction::objects::{HasRelatedFeeType, TransactionInfo}; use crate::transaction::transaction_types::TransactionType; @@ -182,7 +182,10 @@ impl CallEntryPoint { // If the execution of the outer call failed, revert the transction. if call_info.execution.failed { return Err(EntryPointExecutionError::ExecutionFailed { - error_trace: extract_trailing_cairo1_revert_trace(call_info), + error_trace: extract_trailing_cairo1_revert_trace( + call_info, + Cairo1RevertHeader::Execution, + ), }); } } diff --git a/crates/blockifier/src/execution/errors.rs b/crates/blockifier/src/execution/errors.rs index 21a81f385fb..e1e5bc23cad 100644 --- a/crates/blockifier/src/execution/errors.rs +++ b/crates/blockifier/src/execution/errors.rs @@ -81,7 +81,7 @@ impl From for PostExecutionError { pub enum EntryPointExecutionError { #[error(transparent)] CairoRunError(#[from] CairoRunError), - #[error("Execution failed. Failure reason:\n{error_trace}.")] + #[error("{error_trace}")] ExecutionFailed { error_trace: Cairo1RevertStack }, #[error("Internal error: {0}")] InternalError(String), diff --git a/crates/blockifier/src/execution/execution_utils.rs b/crates/blockifier/src/execution/execution_utils.rs index d2933a4955a..1cd423d3667 100644 --- a/crates/blockifier/src/execution/execution_utils.rs +++ b/crates/blockifier/src/execution/execution_utils.rs @@ -21,6 +21,7 @@ use starknet_api::deprecated_contract_class::Program as DeprecatedProgram; use starknet_api::transaction::Calldata; use starknet_types_core::felt::Felt; +use super::stack_trace::Cairo1RevertHeader; use crate::execution::call_info::{CallInfo, Retdata}; use crate::execution::contract_class::{ContractClass, TrackedResource}; use crate::execution::entry_point::{ @@ -84,7 +85,10 @@ pub fn execute_entry_point_call_wrapper( if call_info.execution.failed && !context.versioned_constants().enable_reverts { // Reverts are disabled. return Err(EntryPointExecutionError::ExecutionFailed { - error_trace: extract_trailing_cairo1_revert_trace(call_info), + error_trace: extract_trailing_cairo1_revert_trace( + call_info, + Cairo1RevertHeader::Execution, + ), }); } diff --git a/crates/blockifier/src/execution/stack_trace.rs b/crates/blockifier/src/execution/stack_trace.rs index 6c1cf922037..9c12308a813 100644 --- a/crates/blockifier/src/execution/stack_trace.rs +++ b/crates/blockifier/src/execution/stack_trace.rs @@ -97,6 +97,7 @@ impl From<&VmExceptionFrame> for String { #[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))] pub enum Frame { EntryPoint(EntryPointErrorFrame), + Cairo1Tail(Cairo1RevertStack), Vm(VmExceptionFrame), StringFrame(String), } @@ -105,6 +106,7 @@ impl From<&Frame> for String { fn from(value: &Frame) -> Self { match value { Frame::EntryPoint(entry_point_frame) => entry_point_frame.into(), + Frame::Cairo1Tail(cairo1_revert_stack) => cairo1_revert_stack.to_string(), Frame::Vm(vm_exception_frame) => vm_exception_frame.into(), Frame::StringFrame(error) => error.clone(), } @@ -113,19 +115,25 @@ impl From<&Frame> for String { impl From for Frame { fn from(value: EntryPointErrorFrame) -> Self { - Frame::EntryPoint(value) + Self::EntryPoint(value) + } +} + +impl From for Frame { + fn from(value: Cairo1RevertStack) -> Self { + Self::Cairo1Tail(value) } } impl From for Frame { fn from(value: VmExceptionFrame) -> Self { - Frame::Vm(value) + Self::Vm(value) } } impl From for Frame { fn from(value: String) -> Self { - Frame::StringFrame(value) + Self::StringFrame(value) } } @@ -156,7 +164,8 @@ impl ErrorStack { self.stack.push(frame); } } -#[derive(Debug)] +#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))] +#[derive(Clone, Debug)] pub struct Cairo1RevertFrame { pub contract_address: ContractAddress, pub class_hash: Option, @@ -188,8 +197,30 @@ impl Display for Cairo1RevertFrame { } } -#[derive(Debug)] +#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))] +#[derive(Clone, Copy, Debug)] +pub enum Cairo1RevertHeader { + Execution, + Validation, +} + +impl Display for Cairo1RevertHeader { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{}", + match self { + Self::Execution => "Execution failed. Failure reason:", + Self::Validation => "The `validate` entry point panicked with:", + } + ) + } +} + +#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))] +#[derive(Clone, Debug)] pub struct Cairo1RevertStack { + pub header: Cairo1RevertHeader, pub stack: Vec, pub last_retdata: Retdata, } @@ -198,7 +229,8 @@ impl Display for Cairo1RevertStack { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, - "{}", + "{}\n{}.\n", + self.header, self.stack .iter() .map(|frame| frame.to_string()) @@ -211,9 +243,15 @@ impl Display for Cairo1RevertStack { } } -pub fn extract_trailing_cairo1_revert_trace(root_call: &CallInfo) -> Cairo1RevertStack { - let fallback_value = - Cairo1RevertStack { stack: vec![], last_retdata: root_call.execution.retdata.clone() }; +pub fn extract_trailing_cairo1_revert_trace( + root_call: &CallInfo, + header: Cairo1RevertHeader, +) -> Cairo1RevertStack { + let fallback_value = Cairo1RevertStack { + header, + stack: vec![], + last_retdata: root_call.execution.retdata.clone(), + }; let entrypoint_failed_felt = Felt::from_hex(ENTRYPOINT_FAILED_ERROR) .unwrap_or_else(|_| panic!("{ENTRYPOINT_FAILED_ERROR} does not fit in a felt.")); @@ -262,6 +300,7 @@ pub fn extract_trailing_cairo1_revert_trace(root_call: &CallInfo) -> Cairo1Rever // Add one line per call, and append the failure reason. let Some(last_call) = error_calls.last() else { return fallback_value }; Cairo1RevertStack { + header, stack: error_calls.iter().map(Cairo1RevertFrame::from).collect(), last_retdata: last_call.execution.retdata.clone(), } @@ -302,6 +341,11 @@ pub fn gen_tx_execution_error_trace(error: &TransactionExecutionError) -> ErrorS constructor_selector.as_ref(), PreambleType::Constructor, ), + TransactionExecutionError::PanicInValidate { panic_reason } => { + let mut stack = ErrorStack::default(); + stack.push(panic_reason.clone().into()); + stack + } _ => { // Top-level error is unrelated to Cairo execution, no "real" frames. let mut stack = ErrorStack::default(); @@ -562,6 +606,9 @@ fn extract_entry_point_execution_error_into_stack_trace( EntryPointExecutionError::CairoRunError(cairo_run_error) => { extract_cairo_run_error_into_stack_trace(error_stack, depth, cairo_run_error) } + EntryPointExecutionError::ExecutionFailed { error_trace } => { + error_stack.push(error_trace.clone().into()) + } _ => error_stack.push(format!("{}\n", entry_point_error).into()), } } diff --git a/crates/blockifier/src/execution/stack_trace_test.rs b/crates/blockifier/src/execution/stack_trace_test.rs index 1d57f6952cb..8ae7ff22af9 100644 --- a/crates/blockifier/src/execution/stack_trace_test.rs +++ b/crates/blockifier/src/execution/stack_trace_test.rs @@ -18,7 +18,11 @@ use crate::abi::constants::CONSTRUCTOR_ENTRY_POINT_NAME; use crate::context::{BlockContext, ChainInfo}; use crate::execution::call_info::{CallExecution, CallInfo, Retdata}; use crate::execution::errors::EntryPointExecutionError; -use crate::execution::stack_trace::{extract_trailing_cairo1_revert_trace, Cairo1RevertStack}; +use crate::execution::stack_trace::{ + extract_trailing_cairo1_revert_trace, + Cairo1RevertHeader, + Cairo1RevertStack, +}; use crate::execution::syscalls::hint_processor::ENTRYPOINT_FAILED_ERROR; use crate::test_utils::contracts::FeatureContract; use crate::test_utils::initial_test_state::{fund_account, test_state}; @@ -605,7 +609,8 @@ An ASSERT_EQ instruction failed: 1 != 0. "The `validate` entry point panicked with: Error in contract (contract address: {contract_address:#064x}, class hash: {:#064x}, selector: \ {selector:#064x}): -0x496e76616c6964207363656e6172696f ('Invalid scenario').", +0x496e76616c6964207363656e6172696f ('Invalid scenario'). +", class_hash.0 ), }; @@ -837,14 +842,15 @@ fn test_cairo1_stack_extraction_inner_call_successful() { ..Default::default() }; let error = EntryPointExecutionError::ExecutionFailed { - error_trace: extract_trailing_cairo1_revert_trace(&callinfo), + error_trace: extract_trailing_cairo1_revert_trace(&callinfo, Cairo1RevertHeader::Execution), }; assert_eq!( error.to_string(), format!( "Execution failed. Failure reason: Error in contract (contract address: {:#064x}, class hash: _, selector: {:#064x}): -0xdeadbeef.", +0xdeadbeef. +", ContractAddress::default().0.key(), EntryPointSelector::default().0 ) @@ -873,8 +879,8 @@ fn test_ambiguous_inner_cairo1_failure() { ..Default::default() }; assert_matches!( - extract_trailing_cairo1_revert_trace(&call_info), - Cairo1RevertStack { stack, last_retdata } + extract_trailing_cairo1_revert_trace(&call_info, Cairo1RevertHeader::Execution), + Cairo1RevertStack { stack, last_retdata, .. } if stack.is_empty() && last_retdata == outer_retdata ); } @@ -913,8 +919,8 @@ fn test_inner_cairo1_failure_not_last(#[values(true, false)] last_is_failed: boo ..Default::default() }; assert_matches!( - extract_trailing_cairo1_revert_trace(&call_info), - Cairo1RevertStack { stack, last_retdata } + extract_trailing_cairo1_revert_trace(&call_info, Cairo1RevertHeader::Execution), + Cairo1RevertStack { stack, last_retdata, .. } if stack.len() == 2 && last_retdata == first_inner_retdata ); } @@ -929,8 +935,8 @@ fn test_cairo1_stack_extraction_not_failure_fallback() { ..Default::default() }; assert_matches!( - extract_trailing_cairo1_revert_trace(&successful_call), - Cairo1RevertStack { stack, last_retdata } + extract_trailing_cairo1_revert_trace(&successful_call, Cairo1RevertHeader::Execution), + Cairo1RevertStack { stack, last_retdata, .. } if stack.is_empty() && last_retdata == expected_retdata ); } 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 8148f960d0c..a483250c7f6 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/failure_format.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/failure_format.rs @@ -3,7 +3,7 @@ use starknet_types_core::felt::Felt; use crate::execution::call_info::{CallExecution, CallInfo, Retdata}; use crate::execution::errors::EntryPointExecutionError; -use crate::execution::stack_trace::extract_trailing_cairo1_revert_trace; +use crate::execution::stack_trace::{extract_trailing_cairo1_revert_trace, Cairo1RevertHeader}; #[test] fn test_syscall_failure_format() { @@ -14,14 +14,15 @@ fn test_syscall_failure_format() { ..Default::default() }; let error = EntryPointExecutionError::ExecutionFailed { - error_trace: extract_trailing_cairo1_revert_trace(&callinfo), + error_trace: extract_trailing_cairo1_revert_trace(&callinfo, Cairo1RevertHeader::Execution), }; assert_eq!( error.to_string(), format!( "Execution failed. Failure reason: Error in contract (contract address: {:#064x}, class hash: _, selector: {:#064x}): -{execution_failure} ('Execution failure').", +{execution_failure} ('Execution failure'). +", ContractAddress::default().0.key(), EntryPointSelector::default().0 ) diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index f07ac385ca7..b25c1238bbd 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -26,7 +26,7 @@ use crate::context::{BlockContext, TransactionContext}; use crate::execution::call_info::{CallInfo, Retdata}; use crate::execution::contract_class::ContractClass; use crate::execution::entry_point::{CallEntryPoint, CallType, EntryPointExecutionContext}; -use crate::execution::stack_trace::extract_trailing_cairo1_revert_trace; +use crate::execution::stack_trace::{extract_trailing_cairo1_revert_trace, Cairo1RevertHeader}; use crate::fee::fee_checks::{FeeCheckReportFields, PostExecutionReport}; use crate::fee::fee_utils::{ get_fee_by_gas_vector, @@ -945,7 +945,10 @@ impl ValidatableTransaction for AccountTransaction { if validate_call_info.execution.failed { return Err(TransactionExecutionError::PanicInValidate { - panic_reason: extract_trailing_cairo1_revert_trace(&validate_call_info), + panic_reason: extract_trailing_cairo1_revert_trace( + &validate_call_info, + Cairo1RevertHeader::Validation, + ), }); } diff --git a/crates/blockifier/src/transaction/errors.rs b/crates/blockifier/src/transaction/errors.rs index 1905c78eac0..62c717a2686 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:\n{panic_reason}.")] + #[error("{panic_reason}")] PanicInValidate { panic_reason: Cairo1RevertStack }, #[error("The `validate` entry point should return `VALID`. Got {actual:?}.")] InvalidValidateReturnData { actual: Retdata },