From 8c17f6379614a0d909884cd84938cf95d42b66ee Mon Sep 17 00:00:00 2001 From: fmoletta <99273364+fmoletta@users.noreply.github.com> Date: Thu, 12 Jan 2023 09:44:03 -0300 Subject: [PATCH] Implement `substitute_error_message_attribute_references` (#689) * Add flow_tracking_data to Attribute * Start substitute_error_message_references method * First draft * Finish first draft * Remove duplicated error_attr management * Fix string format * Fix invalid reference string + filter out ap-based references * Copy get_maybe_relocatable_from_reference from PR 687 * Filter out complex cairo types * Clippy * Add tests for ap-based references in error attribute messages * Add tests for complex references in error attribute messages * Start changelog entry * Fix eof * Add changelog entry * Fix test * Update changelog * Fix chanhgelog * Fix chanhgelog * Remove debug print * Simplify test Co-authored-by: Juan Rigada <62958725+Jrigada@users.noreply.github.com> --- CHANGELOG.md | 7 + .../bad_programs/error_msg_attr_struct.cairo | 16 ++ .../bad_programs/error_msg_attr_tempvar.cairo | 7 + src/cairo_run.rs | 2 +- .../hint_processor_definition.rs | 42 +++- src/serde/deserialize_program.rs | 15 ++ src/types/program.rs | 8 + src/utils.rs | 6 +- src/vm/errors/vm_exception.rs | 191 +++++++++++++++++- src/vm/vm_core.rs | 25 +-- tests/bitwise_test.rs | 2 +- tests/cairo_run_test.rs | 35 ++++ tests/pedersen_test.rs | 2 +- tests/struct_test.rs | 2 +- 14 files changed, 318 insertions(+), 42 deletions(-) create mode 100644 cairo_programs/bad_programs/error_msg_attr_struct.cairo create mode 100644 cairo_programs/bad_programs/error_msg_attr_tempvar.cairo diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f84e36958..d6396b439b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,13 @@ * `DictManager`, its dictionaries, and all dict module hints implemented in rust now use `MaybeRelocatable` for keys and values instead of `BigInt` * Add helper functions that allow extracting ids variables as `MaybeRelocatable`: `get_maybe_relocatable_from_var_name` & `get_maybe_relocatable_from_reference` * Change inner value type of dict-related `HintError` variants to `MaybeRelocatable` + +* Implement `substitute_error_message_attribute_references` [#689] (https://github.com/lambdaclass/cairo-rs/pull/689) + * Public Api changes: + * Remove `error_message_attributes` field from `VirtualMachine`, and `VirtualMachine::new` + * Add `flow_tracking_data` field to `Attribute` + * `get_error_attr_value` now replaces the references in the error message with the corresponding cairo values. + * Remove duplicated handling of error attribute messages leading to duplicated into in the final error display. * Fix multiplicative inverse bug [#697](https://github.com/lambdaclass/cairo-rs/pull/697) [#698](https://github.com/lambdaclass/cairo-rs/pull/698). The VM was using integer division rather than prime field inverse when deducing `op0` or `op1` for the multiplication opcode #### [0.1.0] - 2022-12-30 diff --git a/cairo_programs/bad_programs/error_msg_attr_struct.cairo b/cairo_programs/bad_programs/error_msg_attr_struct.cairo new file mode 100644 index 0000000000..5585a28a4e --- /dev/null +++ b/cairo_programs/bad_programs/error_msg_attr_struct.cairo @@ -0,0 +1,16 @@ +%builtins range_check +from starkware.cairo.common.math import assert_le + +struct Cat { + paws: felt, + lives: felt, +} + +func main{range_check_ptr}() { + alloc_locals; + local cat: Cat = Cat(2, 10); + with_attr error_message("Cats cannot have more than nine lives: {cat}") { + assert_le(cat.lives, 9); + } + return(); +} diff --git a/cairo_programs/bad_programs/error_msg_attr_tempvar.cairo b/cairo_programs/bad_programs/error_msg_attr_tempvar.cairo new file mode 100644 index 0000000000..59dc817df7 --- /dev/null +++ b/cairo_programs/bad_programs/error_msg_attr_tempvar.cairo @@ -0,0 +1,7 @@ +func main() { + tempvar x = 3; + with_attr error_message("SafeUint256: addition overflow: {x}") { + assert x = 2; + } + return(); +} diff --git a/src/cairo_run.rs b/src/cairo_run.rs index c8656f92f0..8014ebe761 100644 --- a/src/cairo_run.rs +++ b/src/cairo_run.rs @@ -32,7 +32,7 @@ pub fn cairo_run( }; let mut cairo_runner = CairoRunner::new(&program, layout, proof_mode)?; - let mut vm = VirtualMachine::new(trace_enabled, program.error_message_attributes); + let mut vm = VirtualMachine::new(trace_enabled); let end = cairo_runner.initialize(&mut vm)?; cairo_runner diff --git a/src/hint_processor/hint_processor_definition.rs b/src/hint_processor/hint_processor_definition.rs index 2889c7a025..56af6a02d3 100644 --- a/src/hint_processor/hint_processor_definition.rs +++ b/src/hint_processor/hint_processor_definition.rs @@ -1,13 +1,17 @@ +use crate::any_box; +use crate::serde::deserialize_program::ApTracking; +use crate::serde::deserialize_program::OffsetValue; +use crate::serde::deserialize_program::Reference; +use crate::types::exec_scope::ExecutionScopes; +use crate::types::instruction::Register; +use crate::vm::errors::hint_errors::HintError; +use crate::vm::errors::vm_errors::VirtualMachineError; +use crate::vm::vm_core::VirtualMachine; +use std::any::Any; +use std::collections::HashMap; + use super::builtin_hint_processor::builtin_hint_processor_definition::HintProcessorData; -use crate::{ - any_box, - serde::deserialize_program::{ApTracking, OffsetValue}, - types::{exec_scope::ExecutionScopes, instruction::Register}, - vm::errors::{hint_errors::HintError, vm_errors::VirtualMachineError}, - vm::vm_core::VirtualMachine, -}; use felt::Felt; -use std::{any::Any, collections::HashMap}; pub trait HintProcessor { //Executes the hint which's data is provided by a dynamic structure previously created by compile_hint @@ -97,3 +101,25 @@ impl HintReference { } } } + +impl From for HintReference { + fn from(reference: Reference) -> Self { + HintReference { + offset1: reference.value_address.offset1.clone(), + offset2: reference.value_address.offset2.clone(), + dereference: reference.value_address.dereference, + // only store `ap` tracking data if the reference is referred to it + ap_tracking_data: match ( + &reference.value_address.offset1, + &reference.value_address.offset2, + ) { + (OffsetValue::Reference(Register::AP, _, _), _) + | (_, OffsetValue::Reference(Register::AP, _, _)) => { + Some(reference.ap_tracking_data.clone()) + } + _ => None, + }, + cairo_type: Some(reference.value_address.value_type.clone()), + } + } +} diff --git a/src/serde/deserialize_program.rs b/src/serde/deserialize_program.rs index f6c31e4652..ce4ca395d5 100644 --- a/src/serde/deserialize_program.rs +++ b/src/serde/deserialize_program.rs @@ -83,6 +83,7 @@ pub struct Attribute { pub start_pc: usize, pub end_pc: usize, pub value: String, + pub flow_tracking_data: Option, } #[derive(Deserialize, Clone, Debug, PartialEq, Eq)] @@ -1044,12 +1045,26 @@ mod tests { start_pc: 379, end_pc: 381, value: String::from("SafeUint256: addition overflow"), + flow_tracking_data: Some(FlowTrackingData { + ap_tracking: ApTracking { + group: 14, + offset: 35, + }, + reference_ids: HashMap::new(), + }), }, Attribute { name: String::from("error_message"), start_pc: 402, end_pc: 404, value: String::from("SafeUint256: subtraction overflow"), + flow_tracking_data: Some(FlowTrackingData { + ap_tracking: ApTracking { + group: 15, + offset: 60, + }, + reference_ids: HashMap::new(), + }), }, ]; diff --git a/src/types/program.rs b/src/types/program.rs index 45295596b5..6bbd3c3674 100644 --- a/src/types/program.rs +++ b/src/types/program.rs @@ -109,6 +109,7 @@ impl Default for Program { #[cfg(test)] mod tests { use super::*; + use crate::serde::deserialize_program::{ApTracking, FlowTrackingData}; use crate::utils::test_utils::mayberelocatable; use felt::{felt_str, NewFelt}; use num_traits::Zero; @@ -366,6 +367,13 @@ mod tests { start_pc: 379, end_pc: 381, value: String::from("SafeUint256: addition overflow"), + flow_tracking_data: Some(FlowTrackingData { + ap_tracking: ApTracking { + group: 14, + offset: 35, + }, + reference_ids: HashMap::new(), + }), }]; let data: Vec = vec![ diff --git a/src/utils.rs b/src/utils.rs index 51c15a906d..ebb4d6f38e 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -201,7 +201,7 @@ pub mod test_utils { macro_rules! vm_with_range_check { () => {{ - let mut vm = VirtualMachine::new(false, Vec::new()); + let mut vm = VirtualMachine::new(false); vm.builtin_runners = vec![( "range_check".to_string(), RangeCheckBuiltinRunner::new(8, 8, true).into(), @@ -265,11 +265,11 @@ pub mod test_utils { macro_rules! vm { () => {{ - VirtualMachine::new(false, Vec::new()) + VirtualMachine::new(false) }}; ($use_trace:expr) => {{ - VirtualMachine::new($use_trace, Vec::new()) + VirtualMachine::new($use_trace) }}; } pub(crate) use vm; diff --git a/src/vm/errors/vm_exception.rs b/src/vm/errors/vm_exception.rs index a958106929..f0cb0f6520 100644 --- a/src/vm/errors/vm_exception.rs +++ b/src/vm/errors/vm_exception.rs @@ -8,7 +8,12 @@ use std::{ use thiserror::Error; use crate::{ - serde::deserialize_program::Location, + hint_processor::{ + hint_processor_definition::HintReference, + hint_processor_utils::get_maybe_relocatable_from_reference, + }, + serde::deserialize_program::{ApTracking, Attribute, Location, OffsetValue}, + types::{instruction::Register, relocatable::MaybeRelocatable}, vm::{runners::cairo_runner::CairoRunner, vm_core::VirtualMachine}, }; @@ -29,7 +34,7 @@ impl VmException { error: VirtualMachineError, ) -> Self { let pc = vm.run_context.pc.offset; - let error_attr_value = get_error_attr_value(pc, runner); + let error_attr_value = get_error_attr_value(pc, runner, vm); let hint_index = if let VirtualMachineError::Hint(hint_index, _) = error { Some(hint_index) } else { @@ -45,11 +50,18 @@ impl VmException { } } -pub fn get_error_attr_value(pc: usize, runner: &CairoRunner) -> Option { +pub fn get_error_attr_value( + pc: usize, + runner: &CairoRunner, + vm: &VirtualMachine, +) -> Option { let mut errors = String::new(); for attribute in &runner.program.error_message_attributes { if attribute.start_pc <= pc && attribute.end_pc > pc { - errors.push_str(&format!("Error message: {}\n", attribute.value)); + errors.push_str(&format!( + "Error message: {}\n", + substitute_error_message_references(attribute, runner, vm) + )); } } (!errors.is_empty()).then(|| errors) @@ -75,7 +87,7 @@ pub fn get_location( pub fn get_traceback(vm: &VirtualMachine, runner: &CairoRunner) -> Option { let mut traceback = String::new(); for (_fp, traceback_pc) in vm.get_traceback_entries() { - if let Some(ref attr) = get_error_attr_value(traceback_pc.offset, runner) { + if let Some(ref attr) = get_error_attr_value(traceback_pc.offset, runner, vm) { traceback.push_str(attr) } match get_location(traceback_pc.offset, runner, None) { @@ -90,6 +102,89 @@ pub fn get_traceback(vm: &VirtualMachine, runner: &CairoRunner) -> Option String { + let mut error_msg = error_message_attr.value.clone(); + if let Some(tracking_data) = &error_message_attr.flow_tracking_data { + let mut invalid_references = Vec::::new(); + // Iterate over the available references and check if one of them is addressed in the error message + for (cairo_variable_path, ref_id) in &tracking_data.reference_ids { + // Get the cairo variable name from its path ie: __main__.main.x -> x + let cairo_variable_name = match cairo_variable_path.rsplit('.').next() { + Some(string) => string, + None => continue, + }; + // Format the variable name to make it easier to search for and replace in the error message + // ie: x -> {x} + let formated_variable_name = format!("{{{}}}", cairo_variable_name); + // Look for the formated name inside the error message + if error_msg.contains(&formated_variable_name) { + // Get the value of the cairo variable from its reference id + match get_value_from_simple_reference( + *ref_id, + &tracking_data.ap_tracking, + runner, + vm, + ) { + Some(cairo_variable) => { + // Replace the value in the error message + error_msg = error_msg + .replace(&formated_variable_name, &format!("{}", cairo_variable)) + } + None => { + // If the reference is too complex or ap-based it might lead to a wrong value + // So we append the variable's name to the list of invalid reference + invalid_references.push(cairo_variable_name.to_string()); + } + } + } + } + if !invalid_references.is_empty() { + // Add the invalid references (if any) to the error_msg + error_msg.push_str(&format!( + " (Cannot evaluate ap-based or complex references: [{}])", + invalid_references + .iter() + .fold(String::new(), |acc, arg| acc + &format!("'{}'", arg)) + )); + } + } + error_msg +} + +fn get_value_from_simple_reference( + ref_id: usize, + ap_tracking: &ApTracking, + runner: &CairoRunner, + vm: &VirtualMachine, +) -> Option { + let reference: HintReference = runner + .program + .reference_manager + .references + .get(ref_id)? + .clone() + .into(); + // Filter ap-based references + match reference.offset1 { + OffsetValue::Reference(Register::AP, _, _) => None, + _ => { + // Filer complex types (only felt/felt pointers) + match reference.cairo_type { + Some(ref cairo_type) if cairo_type.contains("felt") => { + Some(get_maybe_relocatable_from_reference(vm, &reference, ap_tracking).ok()?) + } + _ => None, + } + } + } +} + impl Display for VmException { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { // Build initial message @@ -391,11 +486,13 @@ mod test { start_pc: 1, end_pc: 5, value: String::from("Invalid hash"), + flow_tracking_data: None, }]; let program = program!(error_message_attributes = attributes,); let runner = cairo_runner!(program); + let vm = vm!(); assert_eq!( - get_error_attr_value(2, &runner), + get_error_attr_value(2, &runner, &vm), Some(String::from("Error message: Invalid hash\n")) ); } @@ -407,10 +504,12 @@ mod test { start_pc: 1, end_pc: 5, value: String::from("Invalid hash"), + flow_tracking_data: None, }]; let program = program!(error_message_attributes = attributes,); let runner = cairo_runner!(program); - assert_eq!(get_error_attr_value(5, &runner), None); + let vm = vm!(); + assert_eq!(get_error_attr_value(5, &runner, &vm), None); } #[test] @@ -702,4 +801,82 @@ cairo_programs/bad_programs/bad_usort.cairo:64:5: (pc=0:60) let vm_excepction = VmException::from_vm_error(&cairo_runner, &vm, error); assert_eq!(vm_excepction.to_string(), expected_error_string); } + + #[test] + fn get_value_from_simple_reference_ap_based() { + let program = Program::from_file( + Path::new("cairo_programs/bad_programs/error_msg_attr_tempvar.json"), + Some("main"), + ) + .expect("Call to `Program::from_file()` failed."); + // This program uses a tempvar inside an error attribute + // This reference should be rejected when substituting the error attribute references + let runner = cairo_runner!(program); + let vm = vm!(); + // Ref id 0 corresponds to __main__.main.x, our tempvar + assert_eq!( + get_value_from_simple_reference(0, &ApTracking::default(), &runner, &vm), + None + ) + } + + #[test] + fn substitute_error_message_references_ap_based() { + let program = Program::from_file( + Path::new("cairo_programs/bad_programs/error_msg_attr_tempvar.json"), + Some("main"), + ) + .expect("Call to `Program::from_file()` failed."); + // This program uses a tempvar inside an error attribute + // This reference should be rejected when substituting the error attribute references + let runner = cairo_runner!(program); + let vm = vm!(); + let attribute = &program.error_message_attributes[0]; + assert_eq!( + substitute_error_message_references(attribute, &runner, &vm), + format!( + "{} (Cannot evaluate ap-based or complex references: ['x'])", + attribute.value + ) + ); + } + + #[test] + fn get_value_from_simple_reference_complex() { + let program = Program::from_file( + Path::new("cairo_programs/bad_programs/error_msg_attr_struct.json"), + Some("main"), + ) + .expect("Call to `Program::from_file()` failed."); + // This program uses a struct inside an error attribute + // This reference should be rejected when substituting the error attribute references + let runner = cairo_runner!(program); + let vm = vm!(); + // Ref id 0 corresponds to __main__.main.cat, our struct + assert_eq!( + get_value_from_simple_reference(0, &ApTracking::default(), &runner, &vm), + None + ) + } + + #[test] + fn substitute_error_message_references_complex() { + let program = Program::from_file( + Path::new("cairo_programs/bad_programs/error_msg_attr_struct.json"), + Some("main"), + ) + .expect("Call to `Program::from_file()` failed."); + // This program uses a struct inside an error attribute + // This reference should be rejected when substituting the error attribute references + let runner = cairo_runner!(program); + let vm = vm!(); + let attribute = &program.error_message_attributes[0]; + assert_eq!( + substitute_error_message_references(attribute, &runner, &vm), + format!( + "{} (Cannot evaluate ap-based or complex references: ['cat'])", + attribute.value + ) + ); + } } diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 32f55884a7..719a74d1af 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -1,6 +1,6 @@ use crate::{ hint_processor::hint_processor_definition::HintProcessor, - serde::deserialize_program::{ApTracking, Attribute}, + serde::deserialize_program::ApTracking, types::{ exec_scope::ExecutionScopes, instruction::{ @@ -83,7 +83,6 @@ pub struct VirtualMachine { pub(crate) accessed_addresses: Option>, pub(crate) trace: Option>, pub(crate) current_step: usize, - pub(crate) error_message_attributes: Vec, skip_instruction_execution: bool, run_finished: bool, } @@ -103,7 +102,7 @@ impl HintData { } impl VirtualMachine { - pub fn new(trace_enabled: bool, error_message_attributes: Vec) -> VirtualMachine { + pub fn new(trace_enabled: bool) -> VirtualMachine { let run_context = RunContext { pc: Relocatable::from((0, 0)), ap: 0, @@ -128,7 +127,6 @@ impl VirtualMachine { current_step: 0, skip_instruction_execution: false, segments: MemorySegmentManager::new(), - error_message_attributes, run_finished: false, } } @@ -507,20 +505,7 @@ impl VirtualMachine { pub fn step_instruction(&mut self) -> Result<(), VirtualMachineError> { let instruction = self.decode_current_instruction()?; - self.run_instruction(instruction).map_err(|err| { - let pc = &self.get_pc().offset; - let attr_error_msg = &self - .error_message_attributes - .iter() - .find(|attr| attr.start_pc <= *pc && attr.end_pc >= *pc); - match attr_error_msg { - Some(attr) => VirtualMachineError::ErrorMessageAttribute( - attr.value.to_string(), - Box::new(err), - ), - _ => err, - } - })?; + self.run_instruction(instruction)?; self.skip_instruction_execution = false; Ok(()) } @@ -1194,7 +1179,7 @@ mod tests { op1: MaybeRelocatable::Int(Felt::new(10)), }; - let mut vm = VirtualMachine::new(false, Vec::new()); + let mut vm = VirtualMachine::new(false); vm.run_context.pc = Relocatable::from((0, 4)); vm.run_context.ap = 5; vm.run_context.fp = 6; @@ -3503,7 +3488,7 @@ mod tests { #[test] fn disable_trace() { - let mut vm = VirtualMachine::new(true, Vec::new()); + let mut vm = VirtualMachine::new(true); assert!(vm.trace.is_some()); vm.disable_trace(); assert!(vm.trace.is_none()); diff --git a/tests/bitwise_test.rs b/tests/bitwise_test.rs index adba4f6597..082696ef3a 100644 --- a/tests/bitwise_test.rs +++ b/tests/bitwise_test.rs @@ -16,7 +16,7 @@ fn bitwise_integration_test() { .expect("Failed to deserialize program"); let mut hint_processor = BuiltinHintProcessor::new_empty(); let mut cairo_runner = CairoRunner::new(&program, "all", false).unwrap(); - let mut vm = VirtualMachine::new(true, Vec::new()); + let mut vm = VirtualMachine::new(true); let end = cairo_runner.initialize(&mut vm).unwrap(); assert!( cairo_runner.run_until_pc(end, &mut vm, &mut hint_processor) == Ok(()), diff --git a/tests/cairo_run_test.rs b/tests/cairo_run_test.rs index 126c40eb41..d400559daf 100644 --- a/tests/cairo_run_test.rs +++ b/tests/cairo_run_test.rs @@ -1252,6 +1252,41 @@ fn cairo_run_error_msg_attr() { assert!(err.to_string().contains("SafeUint256: addition overflow")); } +#[test] +fn cairo_run_error_msg_attr_ap_based_reference() { + let mut hint_executor = BuiltinHintProcessor::new_empty(); + let err = cairo_run::cairo_run( + Path::new("cairo_programs/bad_programs/error_msg_attr_tempvar.json"), + "main", + false, + false, + "all", + false, + &mut hint_executor, + ) + .err() + .unwrap(); + + assert_eq!(err.to_string(), String::from("Error message: SafeUint256: addition overflow: {x} (Cannot evaluate ap-based or complex references: ['x'])\ncairo_programs/bad_programs/error_msg_attr_tempvar.cairo:4:9: Error at pc=0:2:\nAn ASSERT_EQ instruction failed: 3 != 2.\n assert x = 2;\n ^***********^\n")); +} + +#[test] +fn cairo_run_error_msg_attr_complex_reference() { + let mut hint_executor = BuiltinHintProcessor::new_empty(); + let err = cairo_run::cairo_run( + Path::new("cairo_programs/bad_programs/error_msg_attr_struct.json"), + "main", + false, + false, + "all", + false, + &mut hint_executor, + ) + .err() + .unwrap(); + assert!(err.to_string().contains("Error message: Cats cannot have more than nine lives: {cat} (Cannot evaluate ap-based or complex references: ['cat'])")) +} + #[test] fn cairo_run_dict_store_cast_pointer() { let mut hint_executor = BuiltinHintProcessor::new_empty(); diff --git a/tests/pedersen_test.rs b/tests/pedersen_test.rs index 4836a1f263..060ac078e8 100644 --- a/tests/pedersen_test.rs +++ b/tests/pedersen_test.rs @@ -13,7 +13,7 @@ fn pedersen_integration_test() { .expect("Failed to deserialize program"); let mut hint_processor = BuiltinHintProcessor::new_empty(); let mut cairo_runner = CairoRunner::new(&program, "all", false).unwrap(); - let mut vm = VirtualMachine::new(true, Vec::new()); + let mut vm = VirtualMachine::new(true); let end = cairo_runner.initialize(&mut vm).unwrap(); assert_eq!( cairo_runner.run_until_pc(end, &mut vm, &mut hint_processor), diff --git a/tests/struct_test.rs b/tests/struct_test.rs index 1915ed3734..59529845c5 100644 --- a/tests/struct_test.rs +++ b/tests/struct_test.rs @@ -15,7 +15,7 @@ fn struct_integration_test() { .expect("Failed to deserialize program"); let mut hint_processor = BuiltinHintProcessor::new_empty(); let mut cairo_runner = CairoRunner::new(&program, "all", false).unwrap(); - let mut vm = VirtualMachine::new(true, Vec::new()); + let mut vm = VirtualMachine::new(true); let end = cairo_runner.initialize(&mut vm).unwrap(); assert!(