diff --git a/CHANGELOG.md b/CHANGELOG.md index ef4d845371..76000d5d86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## 0.20.2 (TBD) +- Fix issue where decorator access was not bypassed properly in release mode ([#2529](https://github.com/0xMiden/miden-vm/pull/2529)). + ## 0.20.1 (2025-12-14) - Fix issue where calling procedures from statically linked libraries did not import their decorators ([#2459](https://github.com/0xMiden/miden-vm/pull/2459)). diff --git a/core/src/mast/debuginfo/decorator_storage.rs b/core/src/mast/debuginfo/decorator_storage.rs index 681c541d61..f63b410de9 100644 --- a/core/src/mast/debuginfo/decorator_storage.rs +++ b/core/src/mast/debuginfo/decorator_storage.rs @@ -140,12 +140,36 @@ impl OpToDecoratorIds { /// - Last value in `node_indptr_for_op_idx` must be <= `op_indptr_for_dec_ids.len() - 1` /// - Both `op_indptr_for_dec_ids` and `node_indptr_for_op_idx` must be strictly monotonic (each /// successive value must be >= the previous one) - #[cfg(test)] - pub fn from_components( + pub(crate) fn from_components( decorator_ids: Vec, op_indptr_for_dec_ids: Vec, node_indptr_for_op_idx: IndexVec, ) -> Result { + // Empty structures are valid (no nodes, no decorators) + if decorator_ids.is_empty() + && op_indptr_for_dec_ids.is_empty() + && node_indptr_for_op_idx.is_empty() + { + return Ok(Self { + decorator_ids, + op_indptr_for_dec_ids, + node_indptr_for_op_idx, + }); + } + + // Nodes but no decorators: empty decorator_ids/op_indptr, node_indptr with all zeros + if decorator_ids.is_empty() && op_indptr_for_dec_ids.is_empty() { + if node_indptr_for_op_idx.iter().all(|&ptr| ptr == 0) { + return Ok(Self { + decorator_ids, + op_indptr_for_dec_ids, + node_indptr_for_op_idx, + }); + } else { + return Err(DecoratorIndexError::InternalStructure); + } + } + // Validate the structure if op_indptr_for_dec_ids.is_empty() { return Err(DecoratorIndexError::InternalStructure); @@ -729,9 +753,9 @@ mod tests { #[test] fn test_from_components_invalid_structure() { - // Test with empty operation pointers + // Empty structures are now valid let result = OpToDecoratorIds::from_components(vec![], vec![], IndexVec::new()); - assert_eq!(result, Err(DecoratorIndexError::InternalStructure)); + assert!(result.is_ok()); // Test with operation pointer exceeding decorator indices let result = OpToDecoratorIds::from_components( diff --git a/core/src/mast/debuginfo/mod.rs b/core/src/mast/debuginfo/mod.rs index 7690904ddf..c67b0e031d 100644 --- a/core/src/mast/debuginfo/mod.rs +++ b/core/src/mast/debuginfo/mod.rs @@ -109,6 +109,25 @@ impl DebugInfo { } } + /// Creates an empty [DebugInfo] with valid CSR structures for N nodes. + pub fn empty_for_nodes(num_nodes: usize) -> Self { + let mut node_indptr_for_op_idx = IndexVec::new(); + for _ in 0..=num_nodes { + let _ = node_indptr_for_op_idx.push(0); + } + + let op_decorator_storage = + OpToDecoratorIds::from_components(Vec::new(), Vec::new(), node_indptr_for_op_idx) + .expect("Empty CSR structure should be valid"); + + Self { + decorators: IndexVec::new(), + op_decorator_storage, + node_decorator_storage: NodeToDecoratorIds::new(), + error_codes: BTreeMap::new(), + } + } + // PUBLIC ACCESSORS // -------------------------------------------------------------------------------------------- diff --git a/core/src/mast/mod.rs b/core/src/mast/mod.rs index a50678a708..4d932bd066 100644 --- a/core/src/mast/mod.rs +++ b/core/src/mast/mod.rs @@ -139,7 +139,7 @@ impl MastForest { /// /// This method modifies the forest in-place, removing all decorator information /// including operation-indexed decorators, before-enter decorators, after-exit - /// decorators, and error codes. + /// decorators, and error codes while keeping the CSR structure valid. /// /// # Examples /// @@ -151,8 +151,7 @@ impl MastForest { /// forest.strip_decorators(); // forest is now stripped /// ``` pub fn strip_decorators(&mut self) { - // Clear all debug info (decorators and error codes) - self.debug_info.clear(); + self.debug_info = DebugInfo::empty_for_nodes(self.nodes.len()); } /// Compacts the forest by merging duplicate nodes. diff --git a/core/src/mast/tests.rs b/core/src/mast/tests.rs index 1b17c4277a..e37e1e574c 100644 --- a/core/src/mast/tests.rs +++ b/core/src/mast/tests.rs @@ -9,8 +9,8 @@ use crate::{ DebugOptions, Decorator, Felt, Kernel, Operation, ProgramInfo, Word, chiplets::hasher, mast::{ - BasicBlockNodeBuilder, DynNode, DynNodeBuilder, MastForest, MastForestContributor, - MastNodeExt, + BasicBlockNodeBuilder, DynNode, DynNodeBuilder, JoinNodeBuilder, MastForest, + MastForestContributor, MastNodeExt, SplitNodeBuilder, }, utils::{Deserializable, Serializable}, }; @@ -242,40 +242,61 @@ fn test_decorator_storage_consistency_with_multiple_blocks() { fn test_decorator_storage_after_strip_decorators() { let mut forest = MastForest::new(); - // Create decorators let deco1 = forest.add_decorator(Decorator::Trace(1)).unwrap(); let deco2 = forest.add_decorator(Decorator::Trace(2)).unwrap(); - - // Create operations and decorators let operations = vec![Operation::Push(Felt::new(1)), Operation::Add]; - let decorators = vec![(0, deco1), (1, deco2)]; - - // Add block to forest - let block_id = BasicBlockNodeBuilder::new(operations, decorators) + let block_id = BasicBlockNodeBuilder::new(operations, vec![(0, deco1), (1, deco2)]) .add_to_forest(&mut forest) .unwrap(); - // Verify decorators exist initially - assert!(!forest.debug_info.op_decorator_storage().is_empty()); - assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 1); + assert_eq!(forest.debug_info.num_decorators(), 2); assert_eq!(forest.debug_info.op_decorator_storage().num_decorator_ids(), 2); - // Strip decorators forest.strip_decorators(); - // Verify decorators are cleared from storage - assert!(forest.debug_info.op_decorator_storage().is_empty()); + assert_eq!(forest.debug_info.num_decorators(), 0); + assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 1); + assert!(forest.decorator_links_for_node(block_id).unwrap().into_iter().next().is_none()); +} + +#[test] +fn test_strip_decorators_edge_cases() { + // Empty forest + let mut forest = MastForest::new(); + forest.strip_decorators(); + assert_eq!(forest.debug_info.num_decorators(), 0); assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 0); - assert_eq!(forest.debug_info.op_decorator_storage().num_decorator_ids(), 0); - // Verify block also has no decorators after stripping - let block = if let crate::mast::MastNode::Block(block) = &forest[block_id] { - block - } else { - panic!("Expected a block node"); - }; - let block_decorators: Vec<_> = block.indexed_decorator_iter(&forest).collect(); - assert_eq!(block_decorators, []); + // Idempotent: stripping twice should be safe + let operations = vec![Operation::Push(Felt::new(1)), Operation::Add]; + let block_id = BasicBlockNodeBuilder::new(operations, vec![]) + .add_to_forest(&mut forest) + .unwrap(); + forest.strip_decorators(); + forest.strip_decorators(); + assert_eq!(forest.debug_info.num_decorators(), 0); + assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 1); + assert!(forest.decorator_links_for_node(block_id).unwrap().into_iter().next().is_none()); +} + +#[test] +fn test_strip_decorators_multiple_node_types() { + let mut forest = MastForest::new(); + let deco = forest.add_decorator(Decorator::Trace(1)).unwrap(); + let block_id = BasicBlockNodeBuilder::new( + vec![Operation::Push(Felt::new(1)), Operation::Add], + vec![(0, deco)], + ) + .add_to_forest(&mut forest) + .unwrap(); + + JoinNodeBuilder::new([block_id, block_id]).add_to_forest(&mut forest).unwrap(); + SplitNodeBuilder::new([block_id, block_id]).add_to_forest(&mut forest).unwrap(); + + forest.strip_decorators(); + + assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 3); + assert!(forest.decorator_links_for_node(block_id).unwrap().into_iter().next().is_none()); } #[test] diff --git a/crates/test-utils/src/lib.rs b/crates/test-utils/src/lib.rs index 574ff37e11..e0099ac799 100644 --- a/crates/test-utils/src/lib.rs +++ b/crates/test-utils/src/lib.rs @@ -575,7 +575,11 @@ impl Test { let (program, mut host) = self.get_program_and_host(); let stack_inputs: Vec = self.stack_inputs.clone().into_iter().rev().collect(); let advice_inputs = self.advice_inputs.clone(); - let fast_process = FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs); + let fast_process = if self.in_debug_mode { + FastProcessor::new_debug(&stack_inputs, advice_inputs) + } else { + FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs) + }; let fast_stack_outputs = fast_process.execute_sync(&program, &mut host).unwrap(); assert_eq!( @@ -593,7 +597,11 @@ impl Test { let stack_inputs: Vec = self.stack_inputs.clone().into_iter().rev().collect(); let advice_inputs: AdviceInputs = self.advice_inputs.clone(); - let fast_process = FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs); + let fast_process = if self.in_debug_mode { + FastProcessor::new_debug(&stack_inputs, advice_inputs) + } else { + FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs) + }; let fast_result = fast_process.execute_sync(&program, &mut host); match (fast_result, slow_result) { diff --git a/processor/src/errors.rs b/processor/src/errors.rs index ca684456ef..798df479e8 100644 --- a/processor/src/errors.rs +++ b/processor/src/errors.rs @@ -520,11 +520,17 @@ pub enum AceError { #[cfg(not(feature = "no_err_ctx"))] #[macro_export] macro_rules! err_ctx { - ($mast_forest:expr, $node:expr, $host:expr) => { - $crate::errors::ErrorContextImpl::new($mast_forest, $node, $host) + ($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr) => { + $crate::errors::ErrorContextImpl::new($mast_forest, $node, $host, $in_debug_mode) }; - ($mast_forest:expr, $node:expr, $host:expr, $op_idx:expr) => { - $crate::errors::ErrorContextImpl::new_with_op_idx($mast_forest, $node, $host, $op_idx) + ($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr, $op_idx:expr) => { + $crate::errors::ErrorContextImpl::new_with_op_idx( + $mast_forest, + $node, + $host, + $in_debug_mode, + $op_idx, + ) }; } @@ -541,8 +547,8 @@ macro_rules! err_ctx { #[cfg(feature = "no_err_ctx")] #[macro_export] macro_rules! err_ctx { - ($mast_forest:expr, $node:expr, $host:expr) => {{ () }}; - ($mast_forest:expr, $node:expr, $host:expr, $op_idx:expr) => {{ () }}; + ($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr) => {{ () }}; + ($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr, $op_idx:expr) => {{ () }}; } /// Trait defining the interface for error context providers. @@ -567,9 +573,10 @@ impl ErrorContextImpl { mast_forest: &MastForest, node: &impl MastNodeErrorContext, host: &impl BaseHost, + in_debug_mode: bool, ) -> Self { let (label, source_file) = - Self::precalc_label_and_source_file(None, mast_forest, node, host); + Self::precalc_label_and_source_file(None, mast_forest, node, host, in_debug_mode); Self { label, source_file } } @@ -577,11 +584,12 @@ impl ErrorContextImpl { mast_forest: &MastForest, node: &impl MastNodeErrorContext, host: &impl BaseHost, + in_debug_mode: bool, op_idx: usize, ) -> Self { let op_idx = op_idx.into(); let (label, source_file) = - Self::precalc_label_and_source_file(op_idx, mast_forest, node, host); + Self::precalc_label_and_source_file(op_idx, mast_forest, node, host, in_debug_mode); Self { label, source_file } } @@ -590,7 +598,15 @@ impl ErrorContextImpl { mast_forest: &MastForest, node: &impl MastNodeErrorContext, host: &impl BaseHost, + in_debug_mode: bool, ) -> (SourceSpan, Option>) { + // When not in debug mode, skip the expensive decorator traversal entirely. + // Decorators (including AsmOp decorators used for error context) should only + // be accessed when debugging is enabled. + if !in_debug_mode { + return (SourceSpan::UNKNOWN, None); + } + node.get_assembly_op(mast_forest, op_idx) .and_then(|assembly_op| assembly_op.location()) .map_or_else( diff --git a/processor/src/fast/basic_block.rs b/processor/src/fast/basic_block.rs index 15428c4a11..6486a95a7f 100644 --- a/processor/src/fast/basic_block.rs +++ b/processor/src/fast/basic_block.rs @@ -107,14 +107,18 @@ impl FastProcessor { // Corresponds to the row inserted for the END operation added to the trace. self.increment_clk(tracer); - // execute any decorators which have not been executed during span ops execution; this can - // happen for decorators appearing after all operations in a block. these decorators are + // Execute any decorators which have not been executed during span ops execution; this can + // happen for decorators appearing after all operations in a block. These decorators are // executed after BASIC BLOCK is closed to make sure the VM clock cycle advances beyond the // last clock cycle of the BASIC BLOCK ops. - // For the linked case, check for decorators at an operation index beyond the last operation - let num_ops = basic_block_node.num_operations() as usize; - for decorator in current_forest.decorators_for_op(node_id, num_ops) { - self.execute_decorator(decorator, host)?; + if self.in_debug_mode { + #[cfg(test)] + self.record_decorator_retrieval(); + + let num_ops = basic_block_node.num_operations() as usize; + for decorator in current_forest.decorators_for_op(node_id, num_ops) { + self.execute_decorator(decorator, host)?; + } } self.execute_after_exit_decorators(node_id, current_forest, host) @@ -138,16 +142,22 @@ impl FastProcessor { let mut group_idx = 0; let mut next_group_idx = 1; + // Get the node ID once since it doesn't change within the loop + let node_id = basic_block + .linked_id() + .expect("basic block node should be linked when executing operations"); + // execute operations in the batch one by one for (op_idx_in_batch, op) in batch.ops().iter().enumerate() { let op_idx_in_block = batch_offset_in_block + op_idx_in_batch; - // Use the forest's decorator storage to get decorators for this operation - let node_id = basic_block - .linked_id() - .expect("basic block node should be linked when executing operations"); - for decorator in current_forest.decorators_for_op(node_id, op_idx_in_block) { - self.execute_decorator(decorator, host)?; + if self.in_debug_mode { + #[cfg(test)] + self.record_decorator_retrieval(); + + for decorator in current_forest.decorators_for_op(node_id, op_idx_in_block) { + self.execute_decorator(decorator, host)?; + } } // if in trace mode, check if we need to record a trace state before executing the @@ -165,7 +175,8 @@ impl FastProcessor { // whereas all the other operations are synchronous (resulting in a significant // performance improvement). { - let err_ctx = err_ctx!(program, basic_block, host, op_idx_in_block); + let err_ctx = + err_ctx!(program, basic_block, host, self.in_debug_mode, op_idx_in_block); match op { Operation::Emit => self.op_emit(host, &err_ctx).await?, _ => { diff --git a/processor/src/fast/call_and_dyn.rs b/processor/src/fast/call_and_dyn.rs index 654fc48de6..22764d8f1e 100644 --- a/processor/src/fast/call_and_dyn.rs +++ b/processor/src/fast/call_and_dyn.rs @@ -41,7 +41,7 @@ impl FastProcessor { // Execute decorators that should be executed before entering the node self.execute_before_enter_decorators(current_node_id, current_forest, host)?; - let err_ctx = err_ctx!(current_forest, call_node, host); + let err_ctx = err_ctx!(current_forest, call_node, host, self.in_debug_mode); let callee_hash = current_forest .get_node_by_id(call_node.callee()) @@ -106,7 +106,7 @@ impl FastProcessor { // so we prefix it with underscore to indicate this intentional unused state // and suppress warnings in feature combinations that include `no_err_ctx`. let _call_node = current_forest[node_id].unwrap_call(); - let err_ctx = err_ctx!(current_forest, _call_node, host); + let err_ctx = err_ctx!(current_forest, _call_node, host, self.in_debug_mode); // when returning from a function call or a syscall, restore the // context of the // system registers and the operand stack to what it was prior @@ -142,7 +142,7 @@ impl FastProcessor { // added to the trace. let dyn_node = current_forest[current_node_id].unwrap_dyn(); - let err_ctx = err_ctx!(¤t_forest, dyn_node, host); + let err_ctx = err_ctx!(¤t_forest, dyn_node, host, self.in_debug_mode); // Retrieve callee hash from memory, using stack top as the memory // address. @@ -237,7 +237,7 @@ impl FastProcessor { ); let dyn_node = current_forest[node_id].unwrap_dyn(); - let err_ctx = err_ctx!(current_forest, dyn_node, host); + let err_ctx = err_ctx!(current_forest, dyn_node, host, self.in_debug_mode); // For dyncall, restore the context. if dyn_node.is_dyncall() { self.restore_context(tracer, &err_ctx)?; diff --git a/processor/src/fast/loop.rs b/processor/src/fast/loop.rs index 8252ab4ac9..0ddc7d4089 100644 --- a/processor/src/fast/loop.rs +++ b/processor/src/fast/loop.rs @@ -69,7 +69,7 @@ impl FastProcessor { // Execute decorators that should be executed after exiting the node self.execute_after_exit_decorators(current_node_id, current_forest, host)?; } else { - let err_ctx = err_ctx!(current_forest, loop_node, host); + let err_ctx = err_ctx!(current_forest, loop_node, host, self.in_debug_mode); return Err(ExecutionError::not_binary_value_loop(condition, &err_ctx)); } Ok(()) @@ -121,7 +121,7 @@ impl FastProcessor { self.increment_clk(tracer); self.execute_after_exit_decorators(current_node_id, current_forest, host)?; } else { - let err_ctx = err_ctx!(current_forest, loop_node, host); + let err_ctx = err_ctx!(current_forest, loop_node, host, self.in_debug_mode); return Err(ExecutionError::not_binary_value_loop(condition, &err_ctx)); } Ok(()) diff --git a/processor/src/fast/mod.rs b/processor/src/fast/mod.rs index a9e8f7cd92..e1c2d506c3 100644 --- a/processor/src/fast/mod.rs +++ b/processor/src/fast/mod.rs @@ -1,4 +1,8 @@ +#[cfg(test)] +use alloc::rc::Rc; use alloc::{boxed::Box, sync::Arc, vec::Vec}; +#[cfg(test)] +use core::cell::Cell; use core::cmp::min; use memory::Memory; @@ -124,6 +128,10 @@ pub struct FastProcessor { /// Transcript used to record commitments via `log_precompile` instruction (implemented via RPO /// sponge). pc_transcript: PrecompileTranscript, + + /// Tracks decorator retrieval calls for testing. + #[cfg(test)] + pub decorator_retrieval_count: Rc>, } impl FastProcessor { @@ -189,12 +197,20 @@ impl FastProcessor { ace: Ace::default(), in_debug_mode, pc_transcript: PrecompileTranscript::new(), + #[cfg(test)] + decorator_retrieval_count: Rc::new(Cell::new(0)), } } // ACCESSORS // ------------------------------------------------------------------------------------------- + #[cfg(test)] + #[inline(always)] + fn record_decorator_retrieval(&self) { + self.decorator_retrieval_count.set(self.decorator_retrieval_count.get() + 1); + } + /// Returns the size of the stack. #[inline(always)] fn stack_size(&self) -> usize { @@ -506,6 +522,13 @@ impl FastProcessor { current_forest: &MastForest, host: &mut impl AsyncHost, ) -> Result<(), ExecutionError> { + if !self.in_debug_mode { + return Ok(()); + } + + #[cfg(test)] + self.record_decorator_retrieval(); + let node = current_forest .get_node_by_id(node_id) .expect("internal error: node id {node_id} not found in current forest"); @@ -524,6 +547,13 @@ impl FastProcessor { current_forest: &MastForest, host: &mut impl AsyncHost, ) -> Result<(), ExecutionError> { + if !self.in_debug_mode { + return Ok(()); + } + + #[cfg(test)] + self.record_decorator_retrieval(); + let node = current_forest .get_node_by_id(node_id) .expect("internal error: node id {node_id} not found in current forest"); diff --git a/processor/src/fast/split.rs b/processor/src/fast/split.rs index 30006ef1ed..66991972ba 100644 --- a/processor/src/fast/split.rs +++ b/processor/src/fast/split.rs @@ -46,7 +46,7 @@ impl FastProcessor { } else if condition == ZERO { continuation_stack.push_start_node(split_node.on_false()); } else { - let err_ctx = err_ctx!(current_forest, split_node, host); + let err_ctx = err_ctx!(current_forest, split_node, host, self.in_debug_mode); return Err(ExecutionError::not_binary_value_if(condition, &err_ctx)); }; diff --git a/processor/src/fast/tests/fast_decorator_execution_tests.rs b/processor/src/fast/tests/fast_decorator_execution_tests.rs index 8ad5287b57..acfb0932be 100644 --- a/processor/src/fast/tests/fast_decorator_execution_tests.rs +++ b/processor/src/fast/tests/fast_decorator_execution_tests.rs @@ -51,7 +51,7 @@ fn test_before_enter_decorator_executed_once_fast() { let mut host = TestConsistencyHost::new(); let stack_slice = Vec::new(); - let processor = FastProcessor::new(&stack_slice); + let processor = FastProcessor::new_debug(&stack_slice, AdviceInputs::default()); // Execute the program let result = processor.execute_sync(&program, &mut host); @@ -79,7 +79,7 @@ fn test_multiple_before_enter_decorators_each_once_fast() { let mut host = TestConsistencyHost::new(); let stack_slice = Vec::new(); - let processor = FastProcessor::new(&stack_slice); + let processor = FastProcessor::new_debug(&stack_slice, AdviceInputs::default()); // Execute the program let result = processor.execute_sync(&program, &mut host); @@ -123,7 +123,7 @@ fn test_multiple_after_exit_decorators_each_once_fast() { let mut host = TestConsistencyHost::new(); let stack_slice = Vec::new(); - let processor = FastProcessor::new(&stack_slice); + let processor = FastProcessor::new_debug(&stack_slice, AdviceInputs::default()); // Execute the program let result = processor.execute_sync(&program, &mut host); @@ -173,7 +173,7 @@ fn test_decorator_execution_order_fast() { let mut host = TestConsistencyHost::new(); let stack_slice = Vec::new(); - let processor = FastProcessor::new(&stack_slice); + let processor = FastProcessor::new_debug(&stack_slice, AdviceInputs::default()); // Execute the program let result = processor.execute_sync(&program, &mut host); @@ -236,7 +236,7 @@ fn test_fast_processor_decorator_execution_vs_standard() { // Test fast processor let mut host_fast = TestConsistencyHost::new(); let stack_slice = Vec::new(); - let processor = FastProcessor::new(&stack_slice); + let processor = FastProcessor::new_debug(&stack_slice, AdviceInputs::default()); let result_fast = processor.execute_sync(&program, &mut host_fast); assert!(result_fast.is_ok(), "Fast execution failed: {:?}", result_fast); @@ -301,7 +301,7 @@ fn test_no_duplication_between_inner_and_before_exit_decorators_fast() { let mut host = TestConsistencyHost::new(); let stack_slice = Vec::new(); - let processor = FastProcessor::new(&stack_slice); + let processor = FastProcessor::new_debug(&stack_slice, AdviceInputs::default()); // Execute the program let result = processor.execute_sync(&program, &mut host); @@ -359,3 +359,29 @@ fn create_test_program_with_inner_decorators( Program::new(mast_forest.into(), basic_block_id) } +// DECORATOR BYPASS SPY TESTS +// ================================================================================================ + +#[test] +fn test_decorator_bypass_in_release_mode() { + let program = + create_test_program(&[Decorator::Trace(1)], &[Decorator::Trace(2)], &[Operation::Noop]); + let processor = FastProcessor::new(&[]); + let counter = processor.decorator_retrieval_count.clone(); + let mut host = TestConsistencyHost::new(); + + processor.execute_sync(&program, &mut host).unwrap(); + assert_eq!(counter.get(), 0, "decorators should not be retrieved in release mode"); +} + +#[test] +fn test_decorator_bypass_in_debug_mode() { + let program = + create_test_program(&[Decorator::Trace(1)], &[Decorator::Trace(2)], &[Operation::Noop]); + let processor = FastProcessor::new_debug(&[], AdviceInputs::default()); + let counter = processor.decorator_retrieval_count.clone(); + let mut host = TestConsistencyHost::new(); + + processor.execute_sync(&program, &mut host).unwrap(); + assert!(counter.get() > 0, "decorators should be retrieved in debug mode"); +} diff --git a/processor/src/fast/tests/mod.rs b/processor/src/fast/tests/mod.rs index bba43771b2..abb1f79245 100644 --- a/processor/src/fast/tests/mod.rs +++ b/processor/src/fast/tests/mod.rs @@ -13,7 +13,7 @@ use miden_utils_testing::build_test; use rstest::rstest; use super::*; -use crate::{DefaultHost, Process}; +use crate::{AdviceInputs, DefaultHost, Process}; mod advice_provider; mod all_ops; @@ -384,7 +384,7 @@ fn test_external_node_decorator_sequencing() { crate::test_utils::test_consistency_host::TestConsistencyHost::with_kernel_forest( Arc::new(lib_forest), ); - let processor = FastProcessor::new(&alloc::vec::Vec::new()); + let processor = FastProcessor::new_debug(&alloc::vec::Vec::new(), AdviceInputs::default()); let result = processor.execute_sync(&program, &mut host); assert!(result.is_ok(), "Execution failed: {:?}", result); diff --git a/processor/src/lib.rs b/processor/src/lib.rs index 0d8a94c1ab..f47b8c27e7 100644 --- a/processor/src/lib.rs +++ b/processor/src/lib.rs @@ -258,6 +258,9 @@ pub struct Process { pub enable_tracing: bool, /// Precompile transcript state (sponge capacity) used by `log_precompile`. pub pc_transcript_state: PrecompileTranscriptState, + /// Tracks decorator retrieval calls for testing. + #[cfg(test)] + pub decorator_retrieval_count: core::cell::Cell, } impl Process { @@ -305,9 +308,21 @@ impl Process { max_cycles: execution_options.max_cycles(), enable_tracing: execution_options.enable_tracing(), pc_transcript_state: PrecompileTranscriptState::default(), + #[cfg(test)] + decorator_retrieval_count: core::cell::Cell::new(0), } } + #[cfg(test)] + #[inline(always)] + fn record_decorator_retrieval(&self) { + self.decorator_retrieval_count.set(self.decorator_retrieval_count.get() + 1); + } + + #[cfg(not(test))] + #[inline(always)] + fn record_decorator_retrieval(&self) {} + // PROGRAM EXECUTOR // -------------------------------------------------------------------------------------------- @@ -343,8 +358,11 @@ impl Process { .get_node_by_id(node_id) .ok_or(ExecutionError::MastNodeNotFoundInForest { node_id })?; - for &decorator_id in node.before_enter(program) { - self.execute_decorator(&program[decorator_id], host)?; + if self.decoder.in_debug_mode() { + self.record_decorator_retrieval(); + for &decorator_id in node.before_enter(program) { + self.execute_decorator(&program[decorator_id], host)?; + } } match node { @@ -353,14 +371,14 @@ impl Process { MastNode::Split(node) => self.execute_split_node(node, program, host)?, MastNode::Loop(node) => self.execute_loop_node(node, program, host)?, MastNode::Call(node) => { - let err_ctx = err_ctx!(program, node, host); + let err_ctx = err_ctx!(program, node, host, self.decoder.in_debug_mode()); add_error_ctx_to_external_error( self.execute_call_node(node, program, host), err_ctx, )? }, MastNode::Dyn(node) => { - let err_ctx = err_ctx!(program, node, host); + let err_ctx = err_ctx!(program, node, host, self.decoder.in_debug_mode()); add_error_ctx_to_external_error( self.execute_dyn_node(node, program, host), err_ctx, @@ -373,8 +391,11 @@ impl Process { }, } - for &decorator_id in node.after_exit(program) { - self.execute_decorator(&program[decorator_id], host)?; + if self.decoder.in_debug_mode() { + self.record_decorator_retrieval(); + for &decorator_id in node.after_exit(program) { + self.execute_decorator(&program[decorator_id], host)?; + } } Ok(()) @@ -414,7 +435,7 @@ impl Process { } else if condition == ZERO { self.execute_mast_node(node.on_false(), program, host)?; } else { - let err_ctx = err_ctx!(program, node, host); + let err_ctx = err_ctx!(program, node, host, self.decoder.in_debug_mode()); return Err(ExecutionError::not_binary_value_if(condition, &err_ctx)); } @@ -447,7 +468,7 @@ impl Process { } if self.stack.peek() != ZERO { - let err_ctx = err_ctx!(program, node, host); + let err_ctx = err_ctx!(program, node, host, self.decoder.in_debug_mode()); return Err(ExecutionError::not_binary_value_loop(self.stack.peek(), &err_ctx)); } @@ -458,7 +479,7 @@ impl Process { // already dropped when we started the LOOP block self.end_loop_node(node, false, program, host) } else { - let err_ctx = err_ctx!(program, node, host); + let err_ctx = err_ctx!(program, node, host, self.decoder.in_debug_mode()); Err(ExecutionError::not_binary_value_loop(condition, &err_ctx)) } } @@ -476,10 +497,10 @@ impl Process { let callee = program.get_node_by_id(call_node.callee()).ok_or_else(|| { ExecutionError::MastNodeNotFoundInForest { node_id: call_node.callee() } })?; - let err_ctx = err_ctx!(program, call_node, host); + let err_ctx = err_ctx!(program, call_node, host, self.decoder.in_debug_mode()); self.chiplets.kernel_rom.access_proc(callee.digest(), &err_ctx)?; } - let err_ctx = err_ctx!(program, call_node, host); + let err_ctx = err_ctx!(program, call_node, host, self.decoder.in_debug_mode()); self.start_call_node(call_node, program, host, &err_ctx)?; self.execute_mast_node(call_node.callee(), program, host)?; @@ -497,7 +518,7 @@ impl Process { program: &MastForest, host: &mut impl SyncHost, ) -> Result<(), ExecutionError> { - let err_ctx = err_ctx!(program, node, host); + let err_ctx = err_ctx!(program, node, host, self.decoder.in_debug_mode()); let callee_hash = if node.is_dyncall() { self.start_dyncall_node(node, &err_ctx)? @@ -574,14 +595,16 @@ impl Process { self.end_basic_block_node(basic_block, program, host)?; - // execute any decorators which have not been executed during span ops execution; this - // can happen for decorators appearing after all operations in a block. these decorators + // Execute any decorators which have not been executed during span ops execution; this + // can happen for decorators appearing after all operations in a block. These decorators // are executed after BASIC BLOCK is closed to make sure the VM clock cycle advances beyond // the last clock cycle of the BASIC BLOCK ops. - // For the linked case, check for decorators at an operation index beyond the last operation - let num_ops = basic_block.num_operations() as usize; - for decorator in program.decorators_for_op(node_id, num_ops) { - self.execute_decorator(decorator, host)?; + if self.decoder.in_debug_mode() { + let num_ops = basic_block.num_operations() as usize; + self.record_decorator_retrieval(); + for decorator in program.decorators_for_op(node_id, num_ops) { + self.execute_decorator(decorator, host)?; + } } Ok(()) @@ -619,14 +642,17 @@ impl Process { // execute operations in the batch one by one for (i, &op) in batch.ops().iter().enumerate() { - // Use the forest's decorator storage to get decorators for this operation - let current_op_idx = i + op_offset; - for decorator in program.decorators_for_op(node_id, current_op_idx) { - self.execute_decorator(decorator, host)?; + if self.decoder.in_debug_mode() { + let current_op_idx = i + op_offset; + self.record_decorator_retrieval(); + for decorator in program.decorators_for_op(node_id, current_op_idx) { + self.execute_decorator(decorator, host)?; + } } // decode and execute the operation - let err_ctx = err_ctx!(program, basic_block, host, i + op_offset); + let err_ctx = + err_ctx!(program, basic_block, host, self.decoder.in_debug_mode(), i + op_offset); self.decoder.execute_user_op(op, op_idx); self.execute_op_with_error_ctx(op, program, host, &err_ctx)?; diff --git a/processor/src/tests/decorator_bypass_spy_test.rs b/processor/src/tests/decorator_bypass_spy_test.rs new file mode 100644 index 0000000000..630ceced8b --- /dev/null +++ b/processor/src/tests/decorator_bypass_spy_test.rs @@ -0,0 +1,88 @@ +//! Tests that decorator retrieval is bypassed when `in_debug_mode` is false. + +use alloc::vec::Vec; + +use miden_core::{ + Decorator, Kernel, Operation, + mast::{BasicBlockNodeBuilder, DecoratorId, MastForest, MastForestContributor}, +}; + +use crate::{AdviceInputs, ExecutionOptions, Process, Program, StackInputs}; + +fn create_program_with_decorators() -> Program { + let mut mast_forest = MastForest::new(); + + let trace1 = Decorator::Trace(100); + let trace2 = Decorator::Trace(200); + let trace3 = Decorator::Trace(300); + + let id1 = mast_forest.add_decorator(trace1).unwrap(); + let id2 = mast_forest.add_decorator(trace2).unwrap(); + let id3 = mast_forest.add_decorator(trace3).unwrap(); + + let operations = vec![ + Operation::Push(1_u32.into()), + Operation::Push(2_u32.into()), + Operation::Add, + Operation::Drop, + ]; + + let op_decorators: Vec<(usize, DecoratorId)> = vec![(0, id3)]; + + let basic_block_id = BasicBlockNodeBuilder::new(operations, op_decorators) + .with_before_enter(vec![id1]) + .with_after_exit(vec![id2]) + .add_to_forest(&mut mast_forest) + .unwrap(); + + mast_forest.make_root(basic_block_id); + Program::new(mast_forest.into(), basic_block_id) +} + +#[test] +fn test_decorator_retrieval_bypassed_in_release_mode() { + let program = create_program_with_decorators(); + let mut host = crate::test_utils::test_consistency_host::TestConsistencyHost::new(); + + let mut process = Process::new( + Kernel::default(), + StackInputs::default(), + AdviceInputs::default(), + ExecutionOptions::default(), + ); + + assert!(!process.decoder.in_debug_mode()); + + let result = process.execute(&program, &mut host); + assert!(result.is_ok(), "Execution failed: {:?}", result); + + assert_eq!( + process.decorator_retrieval_count.get(), + 0, + "decorator retrieval should be bypassed in release mode, got {} calls", + process.decorator_retrieval_count.get() + ); +} + +#[test] +fn test_decorator_retrieval_happens_in_debug_mode() { + let program = create_program_with_decorators(); + let mut host = crate::test_utils::test_consistency_host::TestConsistencyHost::new(); + + let mut process = Process::new( + Kernel::default(), + StackInputs::default(), + AdviceInputs::default(), + ExecutionOptions::default().with_tracing(), + ); + + assert!(process.decoder.in_debug_mode()); + + let result = process.execute(&program, &mut host); + assert!(result.is_ok(), "Execution failed: {:?}", result); + + assert!( + process.decorator_retrieval_count.get() > 0, + "decorator retrieval should occur in debug mode" + ); +} diff --git a/processor/src/tests/mod.rs b/processor/src/tests/mod.rs index b117ab7ed3..e478339d3c 100644 --- a/processor/src/tests/mod.rs +++ b/processor/src/tests/mod.rs @@ -20,6 +20,7 @@ use miden_utils_testing::{ use super::*; mod debug; +mod decorator_bypass_spy_test; mod decorator_execution_tests; // AdviceMap inlined in the script