From 44f85b600e5cdce9ef6accba409484c3f091d271 Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Sun, 20 Oct 2024 11:50:38 +0300 Subject: [PATCH] feat(blockifier): limit characters in cairo1 revert trace --- .../blockifier/src/execution/stack_trace.rs | 88 ++++++++++++- .../src/execution/stack_trace_test.rs | 123 +++++++++++++++++- 2 files changed, 205 insertions(+), 6 deletions(-) diff --git a/crates/blockifier/src/execution/stack_trace.rs b/crates/blockifier/src/execution/stack_trace.rs index 88bdf72c7f..2a52ea831d 100644 --- a/crates/blockifier/src/execution/stack_trace.rs +++ b/crates/blockifier/src/execution/stack_trace.rs @@ -1,4 +1,5 @@ use std::fmt::{Display, Formatter}; +use std::sync::LazyLock; use cairo_vm::types::relocatable::Relocatable; use cairo_vm::vm::errors::cairo_run_errors::CairoRunError; @@ -163,6 +164,16 @@ pub struct Cairo1RevertFrame { pub selector: EntryPointSelector, } +pub static MIN_CAIRO1_FRAME_LENGTH: LazyLock = LazyLock::new(|| { + let frame = Cairo1RevertFrame { + contract_address: ContractAddress::default(), + class_hash: Some(ClassHash::default()), + selector: EntryPointSelector::default(), + }; + // +1 for newline. + format!("{}", frame).len() + 1 +}); + impl From<&&CallInfo> for Cairo1RevertFrame { fn from(callinfo: &&CallInfo) -> Self { Self { @@ -194,16 +205,83 @@ pub struct Cairo1RevertStack { pub last_retdata: Retdata, } +impl Cairo1RevertStack { + pub const TRUNCATION_SEPARATOR: &'static str = "\n..."; +} + +pub static MIN_CAIRO1_FRAMES_STACK_LENGTH: LazyLock = LazyLock::new(|| { + // Two frames (first and last) + separator. + 2 * *MIN_CAIRO1_FRAME_LENGTH + Cairo1RevertStack::TRUNCATION_SEPARATOR.len() +}); + impl Display for Cairo1RevertStack { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // Total string length is limited by TRACE_LENGTH_CAP. + + // Prioritize the failure reason felts over the frames. + // If the failure reason is too long to include a minimal frame trace, display only the + // failure reason (truncated if necessary). + let failure_reason = format_panic_data(&self.last_retdata.0); + if failure_reason.len() >= TRACE_LENGTH_CAP - *MIN_CAIRO1_FRAMES_STACK_LENGTH { + let output = if failure_reason.len() <= TRACE_LENGTH_CAP { + failure_reason + } else { + failure_reason + .chars() + .take(TRACE_LENGTH_CAP - Self::TRUNCATION_SEPARATOR.len()) + .collect::() + + Self::TRUNCATION_SEPARATOR + }; + return write!(f, "{}", output); + } + + let untruncated_string = self + .stack + .iter() + .map(|frame| frame.to_string()) + .chain([failure_reason.clone()]) + .join("\n"); + if untruncated_string.len() <= TRACE_LENGTH_CAP { + return write!(f, "{}", untruncated_string); + } + + // If the number of frames is too large, drop frames above the last frame (two frames are + // not too many, as checked above with MIN_CAIRO1_FRAMES_STACK_LENGTH). + let n_frames_to_drop = (untruncated_string.len() - TRACE_LENGTH_CAP + + Self::TRUNCATION_SEPARATOR.len()) + .div_ceil(*MIN_CAIRO1_FRAME_LENGTH); + + // If the number of frames is not as expected, fall back to the failure reason. + let final_string = + match (self.stack.get(..self.stack.len() - n_frames_to_drop - 1), self.stack.last()) { + (Some(frames), Some(last_frame)) => { + let combined_string = frames + .iter() + .map(|frame| frame.to_string()) + .chain([ + String::from(Self::TRUNCATION_SEPARATOR), + last_frame.to_string(), + failure_reason, + ]) + .join("\n"); + if combined_string.len() <= TRACE_LENGTH_CAP { + combined_string + } else { + // If the combined string is too long, truncate it. + combined_string + .chars() + .take(TRACE_LENGTH_CAP - Self::TRUNCATION_SEPARATOR.len()) + .collect::() + + Self::TRUNCATION_SEPARATOR + } + } + _ => failure_reason, + }; write!( f, "{}", - self.stack - .iter() - .map(|frame| frame.to_string()) - .chain([format_panic_data(&self.last_retdata.0)]) - .join("\n") + // Truncate again as a failsafe. + final_string.chars().take(TRACE_LENGTH_CAP).collect::() ) } } diff --git a/crates/blockifier/src/execution/stack_trace_test.rs b/crates/blockifier/src/execution/stack_trace_test.rs index d3a20f94d0..3d2fb80bad 100644 --- a/crates/blockifier/src/execution/stack_trace_test.rs +++ b/crates/blockifier/src/execution/stack_trace_test.rs @@ -1,7 +1,13 @@ use pretty_assertions::assert_eq; use regex::Regex; use rstest::rstest; -use starknet_api::core::{calculate_contract_address, Nonce}; +use starknet_api::core::{ + calculate_contract_address, + ClassHash, + ContractAddress, + EntryPointSelector, + Nonce, +}; use starknet_api::transaction::{ ContractAddressSalt, Fee, @@ -14,6 +20,15 @@ use starknet_api::{calldata, felt, invoke_tx_args}; use crate::abi::abi_utils::selector_from_name; use crate::abi::constants::CONSTRUCTOR_ENTRY_POINT_NAME; use crate::context::{BlockContext, ChainInfo}; +use crate::execution::call_info::{CallExecution, CallInfo, Retdata}; +use crate::execution::entry_point::CallEntryPoint; +use crate::execution::stack_trace::{ + extract_trailing_cairo1_revert_trace, + Cairo1RevertStack, + MIN_CAIRO1_FRAME_LENGTH, + TRACE_LENGTH_CAP, +}; +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}; use crate::test_utils::{create_calldata, CairoVersion, BALANCE}; @@ -818,3 +833,109 @@ Error in contract (contract address: {expected_address:#064x}, class hash: {:#06 invoke_deploy_tx.execute(state, &block_context, true, true).unwrap().revert_error.unwrap(); assert_eq!(error.to_string(), expected_error); } + +#[test] +fn test_min_cairo1_frame_length() { + let failure_hex = "0xdeadbeef"; + let call_info = CallInfo { + call: CallEntryPoint { + class_hash: Some(ClassHash::default()), + storage_address: ContractAddress::default(), + entry_point_selector: EntryPointSelector::default(), + ..Default::default() + }, + execution: CallExecution { + retdata: Retdata(vec![felt!(failure_hex)]), + failed: true, + ..Default::default() + }, + ..Default::default() + }; + let error_stack = extract_trailing_cairo1_revert_trace(&call_info); + let error_string = error_stack.to_string(); + // Frame, newline, failure reason. + // Min frame length includes the newline. + assert_eq!(error_string.len(), *MIN_CAIRO1_FRAME_LENGTH + failure_hex.len()); +} + +#[rstest] +#[case::too_many_frames(TRACE_LENGTH_CAP / *MIN_CAIRO1_FRAME_LENGTH + 10, 1, "too_many_frames")] +// Each (large) felt should require at least 30 chars. +#[case::too_much_retdata(1, TRACE_LENGTH_CAP / 30, "too_much_retdata")] +#[case::both_too_much( + TRACE_LENGTH_CAP / (2 * *MIN_CAIRO1_FRAME_LENGTH), TRACE_LENGTH_CAP / 40, "both_too_much" +)] +fn test_cairo1_revert_error_truncation( + #[case] n_frames: usize, + #[case] n_retdata: usize, + #[case] scenario: &str, +) { + let failure_felt = "0xbeefbeefbeefbeefbeefbeefbeefbeef"; + let call = CallEntryPoint { + class_hash: Some(ClassHash::default()), + storage_address: ContractAddress::default(), + entry_point_selector: EntryPointSelector::default(), + ..Default::default() + }; + let mut retdata = Retdata(vec![felt!(failure_felt); n_retdata]); + let mut next_call_info = CallInfo { + call: call.clone(), + execution: CallExecution { retdata: retdata.clone(), failed: true, ..Default::default() }, + ..Default::default() + }; + for _ in 1..n_frames { + retdata.0.push(felt!(ENTRYPOINT_FAILED_ERROR)); + next_call_info = CallInfo { + call: call.clone(), + inner_calls: vec![next_call_info], + execution: CallExecution { + retdata: retdata.clone(), + failed: true, + ..Default::default() + }, + ..Default::default() + }; + } + + // Check that the error message is structured as expected. + let error_stack = extract_trailing_cairo1_revert_trace(&next_call_info); + let error_string = error_stack.to_string(); + let first_frame = error_stack.stack.first().unwrap().to_string(); + let last_frame = error_stack.stack.last().unwrap().to_string(); + match scenario { + // Frames truncated, entire failure reason (a single felt) is output. + "too_many_frames" => { + assert!(error_string.starts_with(&format!("{first_frame}\n"))); + assert!( + error_string.ends_with( + &[ + Cairo1RevertStack::TRUNCATION_SEPARATOR.into(), + last_frame, + // One failure felt. + failure_felt.to_string(), + ] + .join("\n") + ) + ); + } + // A single frame, but failure reason itself is too long. No frames printed. + "too_much_retdata" => { + assert!(error_string.starts_with(&format!("({failure_felt}"))); + assert!(error_string.ends_with(Cairo1RevertStack::TRUNCATION_SEPARATOR)); + } + // Too many frames and too much retdata - retdata takes precedence. + "both_too_much" => { + assert!(error_string.starts_with(&format!("{first_frame}\n"))); + let retdata_tail = + format!("({}{failure_felt})", format!("{failure_felt}, ").repeat(n_retdata - 1)); + assert!( + error_string.ends_with( + &[Cairo1RevertStack::TRUNCATION_SEPARATOR.into(), last_frame, retdata_tail] + .join("\n") + ) + ); + } + _ => panic!("Test not implemented for {n_frames} frames."), + } + assert!(error_string.len() <= TRACE_LENGTH_CAP); +}