Skip to content

Commit

Permalink
feat(blockifier): keep revert error structure in TransactionExecution…
Browse files Browse the repository at this point in the history
…Info
  • Loading branch information
dorimedini-starkware committed Oct 21, 2024
1 parent 9631fe8 commit 2d33fcc
Show file tree
Hide file tree
Showing 15 changed files with 171 additions and 61 deletions.
4 changes: 2 additions & 2 deletions crates/batcher/src/block_builder_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use blockifier::blockifier::transaction_executor::{
TransactionExecutorResult,
};
use blockifier::bouncer::BouncerWeights;
use blockifier::transaction::objects::TransactionExecutionInfo;
use blockifier::transaction::objects::{RevertError, TransactionExecutionInfo};
use blockifier::transaction::transaction_execution::Transaction as BlockifierTransaction;
use indexmap::IndexMap;
use rstest::{fixture, rstest};
Expand Down Expand Up @@ -191,5 +191,5 @@ fn block_builder_expected_output(
}

fn execution_info() -> TransactionExecutionInfo {
TransactionExecutionInfo { revert_error: Some("Test string".to_string()), ..Default::default() }
TransactionExecutionInfo { revert_error: Some(RevertError::default()), ..Default::default() }
}
2 changes: 1 addition & 1 deletion crates/blockifier/src/concurrency/worker_logic_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ fn test_deploy_before_declare(
let execution_output = worker_executor.execution_outputs[1].lock().unwrap();
let tx_execution_info = execution_output.as_ref().unwrap().result.as_ref().unwrap();
assert!(tx_execution_info.is_reverted());
assert!(tx_execution_info.revert_error.clone().unwrap().contains("not declared."));
assert!(tx_execution_info.revert_error.clone().unwrap().to_string().contains("not declared."));
drop(execution_output);

// Creates 2 active tasks.
Expand Down
70 changes: 58 additions & 12 deletions crates/blockifier/src/execution/stack_trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub const TRACE_LENGTH_CAP: usize = 15000;
pub const TRACE_EXTRA_CHARS_SLACK: usize = 100;

#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug, PartialEq)]
pub enum PreambleType {
CallContract,
LibraryCall,
Expand All @@ -39,7 +40,9 @@ impl PreambleType {
}
}

#[cfg_attr(any(test, feature = "testing"), derive(Clone))]
#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Debug, PartialEq)]
pub struct EntryPointErrorFrame {
pub depth: usize,
pub preamble_type: PreambleType,
Expand Down Expand Up @@ -71,7 +74,9 @@ impl From<&EntryPointErrorFrame> for String {
}
}

#[cfg_attr(any(test, feature = "testing"), derive(Clone))]
#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Debug, PartialEq)]
pub struct VmExceptionFrame {
pub pc: Relocatable,
pub error_attr_value: Option<String>,
Expand All @@ -94,7 +99,9 @@ impl From<&VmExceptionFrame> for String {
}
}

#[cfg_attr(any(test, feature = "testing"), derive(Clone))]
#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Debug, PartialEq)]
pub enum Frame {
EntryPoint(EntryPointErrorFrame),
Cairo1Tail(Cairo1RevertStack),
Expand Down Expand Up @@ -138,24 +145,52 @@ impl From<String> for Frame {
}

#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Default)]
#[derive(Clone, Debug, Default, PartialEq)]
pub enum ErrorStackHeader {
Constructor,
Execution,
Validation,
#[default]
None,
}

impl Display for ErrorStackHeader {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(
f,
"{}",
match self {
Self::Constructor => "Contract constructor execution has failed:\n",
Self::Execution => "Transaction execution has failed:\n",
Self::Validation => "Transaction validation has failed:\n",
Self::None => "",
}
)
}
}

#[cfg_attr(any(test, feature = "testing"), derive(Clone))]
#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Debug, Default, PartialEq)]
pub struct ErrorStack {
pub header: ErrorStackHeader,
pub stack: Vec<Frame>,
}

impl From<ErrorStack> for String {
fn from(value: ErrorStack) -> Self {
let error_stack_str = value.stack.iter().map(String::from).join("\n");
impl Display for ErrorStack {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
let error_stack_str = self.stack.iter().map(String::from).join("\n");

// When the trace string is too long, trim it in a way that keeps both the beginning and
// end.
if error_stack_str.len() > TRACE_LENGTH_CAP + TRACE_EXTRA_CHARS_SLACK {
let final_str = if error_stack_str.len() > TRACE_LENGTH_CAP + TRACE_EXTRA_CHARS_SLACK {
error_stack_str[..(TRACE_LENGTH_CAP / 2)].to_string()
+ "\n\n...\n\n"
+ &error_stack_str[(error_stack_str.len() - TRACE_LENGTH_CAP / 2)..]
} else {
error_stack_str
}
};
write!(f, "{}{}", self.header, final_str)
}
}

Expand All @@ -164,8 +199,9 @@ impl ErrorStack {
self.stack.push(frame);
}
}

#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq)]
pub struct Cairo1RevertFrame {
pub contract_address: ContractAddress,
pub class_hash: Option<ClassHash>,
Expand Down Expand Up @@ -198,7 +234,7 @@ impl Display for Cairo1RevertFrame {
}

#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, PartialEq)]
pub enum Cairo1RevertHeader {
Execution,
Validation,
Expand All @@ -218,7 +254,7 @@ impl Display for Cairo1RevertHeader {
}

#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq)]
pub struct Cairo1RevertStack {
pub header: Cairo1RevertHeader,
pub stack: Vec<Cairo1RevertFrame>,
Expand Down Expand Up @@ -293,13 +329,21 @@ pub fn gen_tx_execution_error_trace(error: &TransactionExecutionError) -> ErrorS
class_hash,
storage_address,
selector,
}
| TransactionExecutionError::ValidateTransactionError {
} => gen_error_trace_from_entry_point_error(
ErrorStackHeader::Execution,
error,
storage_address,
class_hash,
Some(selector),
PreambleType::CallContract,
),
TransactionExecutionError::ValidateTransactionError {
error,
class_hash,
storage_address,
selector,
} => gen_error_trace_from_entry_point_error(
ErrorStackHeader::Validation,
error,
storage_address,
class_hash,
Expand All @@ -314,6 +358,7 @@ pub fn gen_tx_execution_error_trace(error: &TransactionExecutionError) -> ErrorS
constructor_selector,
},
) => gen_error_trace_from_entry_point_error(
ErrorStackHeader::Constructor,
error,
storage_address,
class_hash,
Expand All @@ -336,13 +381,14 @@ pub fn gen_tx_execution_error_trace(error: &TransactionExecutionError) -> ErrorS

/// Generate error stack from top-level entry point execution error.
fn gen_error_trace_from_entry_point_error(
header: ErrorStackHeader,
error: &EntryPointExecutionError,
storage_address: &ContractAddress,
class_hash: &ClassHash,
entry_point_selector: Option<&EntryPointSelector>,
preamble_type: PreambleType,
) -> ErrorStack {
let mut error_stack: ErrorStack = ErrorStack::default();
let mut error_stack = ErrorStack { header, ..Default::default() };
let depth = 0;
error_stack.push(
EntryPointErrorFrame {
Expand Down
3 changes: 2 additions & 1 deletion crates/blockifier/src/fee/fee_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use crate::state::state_api::StateReader;
use crate::transaction::errors::TransactionExecutionError;
use crate::transaction::objects::{FeeType, TransactionExecutionResult, TransactionInfo};

#[derive(Clone, Copy, Debug, Error)]
#[cfg_attr(feature = "transaction_serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Copy, Debug, Error, PartialEq)]
pub enum FeeCheckError {
#[error(
"Insufficient max {resource}: max amount: {max_amount}, actual used: {actual_amount}."
Expand Down
15 changes: 10 additions & 5 deletions crates/blockifier/src/transaction/account_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ 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, Cairo1RevertHeader};
use crate::execution::stack_trace::{
extract_trailing_cairo1_revert_trace,
gen_tx_execution_error_trace,
Cairo1RevertHeader,
};
use crate::fee::fee_checks::{FeeCheckReportFields, PostExecutionReport};
use crate::fee::fee_utils::{
get_fee_by_gas_vector,
Expand All @@ -47,6 +51,7 @@ use crate::transaction::errors::{
use crate::transaction::objects::{
DeprecatedTransactionInfo,
HasRelatedFeeType,
RevertError,
TransactionExecutionInfo,
TransactionExecutionResult,
TransactionInfo,
Expand Down Expand Up @@ -720,7 +725,7 @@ impl AccountTransaction {
execution_state.abort();
Ok(ValidateExecuteCallInfo::new_reverted(
validate_call_info,
post_execution_error.to_string(),
post_execution_error.into(),
TransactionReceipt {
fee: post_execution_report.recommended_fee(),
..revert_cost
Expand All @@ -745,7 +750,7 @@ impl AccountTransaction {
PostExecutionReport::new(state, &tx_context, &revert_cost, charge_fee)?;
Ok(ValidateExecuteCallInfo::new_reverted(
validate_call_info,
execution_error.to_string(),
gen_tx_execution_error_trace(&execution_error).into(),
TransactionReceipt {
fee: post_execution_report.recommended_fee(),
..revert_cost
Expand Down Expand Up @@ -868,7 +873,7 @@ impl TransactionInfoCreator for AccountTransaction {
struct ValidateExecuteCallInfo {
validate_call_info: Option<CallInfo>,
execute_call_info: Option<CallInfo>,
revert_error: Option<String>,
revert_error: Option<RevertError>,
final_cost: TransactionReceipt,
}

Expand All @@ -883,7 +888,7 @@ impl ValidateExecuteCallInfo {

pub fn new_reverted(
validate_call_info: Option<CallInfo>,
revert_error: String,
revert_error: RevertError,
final_cost: TransactionReceipt,
) -> Self {
Self {
Expand Down
27 changes: 23 additions & 4 deletions crates/blockifier/src/transaction/account_transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ fn test_invoke_tx_from_non_deployed_account(
match tx_result {
Ok(info) => {
// Make sure the error is because the account wasn't deployed.
assert!(info.revert_error.is_some_and(|err_str| err_str.contains(expected_error)));
assert!(info.revert_error.unwrap().to_string().contains(expected_error));
}
Err(err) => {
// Make sure the error is because the account wasn't deployed.
Expand Down Expand Up @@ -339,6 +339,7 @@ fn test_infinite_recursion(
tx_execution_info
.revert_error
.unwrap()
.to_string()
.contains("RunResources has no remaining steps.")
);
}
Expand Down Expand Up @@ -546,7 +547,14 @@ fn test_recursion_depth_exceeded(
InvokeTxArgs { calldata, nonce: nonce_manager.next(account_address), ..invoke_args };
let tx_execution_info = run_invoke_tx(&mut state, &block_context, invoke_args);

assert!(tx_execution_info.unwrap().revert_error.unwrap().contains("recursion depth exceeded"));
assert!(
tx_execution_info
.unwrap()
.revert_error
.unwrap()
.to_string()
.contains("recursion depth exceeded")
);
}

#[rstest]
Expand Down Expand Up @@ -1128,6 +1136,7 @@ fn test_insufficient_max_fee_reverts(
tx_execution_info2
.revert_error
.unwrap()
.to_string()
.starts_with(&format!("Insufficient max {resource}", resource = Resource::L1Gas))
);

Expand All @@ -1148,7 +1157,11 @@ fn test_insufficient_max_fee_reverts(
assert!(tx_execution_info3.is_reverted());
assert!(tx_execution_info3.receipt.fee == actual_fee_depth1);
assert!(
tx_execution_info3.revert_error.unwrap().contains("RunResources has no remaining steps.")
tx_execution_info3
.revert_error
.unwrap()
.to_string()
.contains("RunResources has no remaining steps.")
);
}

Expand Down Expand Up @@ -1544,7 +1557,13 @@ fn test_revert_in_execute(
.unwrap();

assert!(tx_execution_info.is_reverted());
assert!(tx_execution_info.revert_error.unwrap().contains("Failed to deserialize param #1"));
assert!(
tx_execution_info
.revert_error
.unwrap()
.to_string()
.contains("Failed to deserialize param #1")
);
}

#[rstest]
Expand Down
15 changes: 3 additions & 12 deletions crates/blockifier/src/transaction/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,11 @@ pub enum TransactionExecutionError {
version {cairo_version:?}.", **declare_version
)]
ContractClassVersionMismatch { declare_version: TransactionVersion, cairo_version: u64 },
#[error(
"Contract constructor execution has failed:\n{}",
String::from(gen_tx_execution_error_trace(self))
)]
#[error("{}", gen_tx_execution_error_trace(self))]
ContractConstructorExecutionFailed(#[from] ConstructorEntryPointExecutionError),
#[error("Class with hash {:#064x} is already declared.", **class_hash)]
DeclareTransactionError { class_hash: ClassHash },
#[error(
"Transaction execution has failed:\n{}",
String::from(gen_tx_execution_error_trace(self))
)]
#[error("{}", gen_tx_execution_error_trace(self))]
ExecutionError {
error: EntryPointExecutionError,
class_hash: ClassHash,
Expand Down Expand Up @@ -127,10 +121,7 @@ pub enum TransactionExecutionError {
transaction size: {}.", *max_capacity, *tx_size
)]
TransactionTooLarge { max_capacity: Box<BouncerWeights>, tx_size: Box<BouncerWeights> },
#[error(
"Transaction validation has failed:\n{}",
String::from(gen_tx_execution_error_trace(self))
)]
#[error("{}", gen_tx_execution_error_trace(self))]
ValidateTransactionError {
error: EntryPointExecutionError,
class_hash: ClassHash,
Expand Down
Loading

0 comments on commit 2d33fcc

Please sign in to comment.