From bff7607a3a4bd8d00cd1f8e4d5f5730cce231182 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Wed, 7 Jan 2026 13:36:24 -0500 Subject: [PATCH 1/7] chore: allow bincode exception in deny.toml --- deny.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/deny.toml b/deny.toml index 098d1b9043..e5ce39146d 100644 --- a/deny.toml +++ b/deny.toml @@ -16,6 +16,7 @@ db-urls = ["https://github.com/rustsec/advisory-db"] yanked = "warn" ignore = [ "RUSTSEC-2024-0436", # paste is unmaintained but no alternative available + "RUSTSEC-2025-0141", # bincode is unmaintained, replace with wincode (#2550) ] # License configuration From eab67657c821da1a62ee6879457aee2662b1c014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Fri, 12 Dec 2025 00:29:09 -0500 Subject: [PATCH 2/7] feat(core): serialize operations in padded form with batch metadata Serialize BasicBlockNode operations in padded form with batch metadata to enable exact OpBatch reconstruction during deserialization. Changes: - Add batch metadata to serialization format (indptr, padding, groups) - Add OperationData enum to bundle operations with matching decorator indices - Add from_op_batches constructor to BasicBlockNodeBuilder - Serialize decorators with padded indices to match padded operations - Bump serialization version from [0,0,0] to [0,0,1] - Add comprehensive tests including 7 unit tests and 3 proptests This preserves the exact OpBatch structure across serialization boundaries, eliminating the need for re-batching during deserialization. --- core/src/mast/node/basic_block_node/mod.rs | 193 ++++-- .../mast/node/basic_block_node/op_batch.rs | 21 + core/src/mast/serialization/basic_blocks.rs | 126 +++- core/src/mast/serialization/info.rs | 9 +- core/src/mast/serialization/mod.rs | 13 +- core/src/mast/serialization/tests.rs | 621 ++++++++++++++++++ 6 files changed, 920 insertions(+), 63 deletions(-) diff --git a/core/src/mast/node/basic_block_node/mod.rs b/core/src/mast/node/basic_block_node/mod.rs index 67b30c93b9..3bb5bb1144 100644 --- a/core/src/mast/node/basic_block_node/mod.rs +++ b/core/src/mast/node/basic_block_node/mod.rs @@ -1253,11 +1253,30 @@ fn batch_ops(ops: Vec) -> Vec { } // ------------------------------------------------------------------------------------------------ +/// Represents the operation data for a [`BasicBlockNodeBuilder`]. +/// +/// The decorators are bundled with the operation data to maintain the invariant that +/// decorator indices match the format of the operations: +/// - `Raw`: decorators have raw (unpadded) indices +/// - `Batched`: decorators have padded indices +#[derive(Debug)] +enum OperationData { + /// Raw operations with raw decorator indices + Raw { + operations: Vec, + decorators: DecoratorList, + }, + /// Pre-batched operations with padded decorator indices + Batched { + op_batches: Vec, + decorators: DecoratorList, + }, +} + /// Builder for creating [`BasicBlockNode`] instances with decorators. #[derive(Debug)] pub struct BasicBlockNodeBuilder { - operations: Vec, - decorators: DecoratorList, + operation_data: OperationData, before_enter: Vec, after_exit: Vec, digest: Option, @@ -1265,45 +1284,84 @@ pub struct BasicBlockNodeBuilder { impl BasicBlockNodeBuilder { /// Creates a new builder for a BasicBlockNode with the specified operations and decorators. + /// + /// The decorators must use raw (unpadded) operation indices. pub fn new(operations: Vec, decorators: DecoratorList) -> Self { Self { - operations, - decorators, + operation_data: OperationData::Raw { operations, decorators }, before_enter: Vec::new(), after_exit: Vec::new(), digest: None, } } + /// Creates a builder from pre-existing OpBatches with padded decorator indices. + /// + /// This constructor is used during deserialization where operations are already batched + /// and decorators already use padded indices. The digest must also be provided. + /// + /// The decorators must use padded operation indices that match the batched operations. + pub(crate) fn from_op_batches( + op_batches: Vec, + decorators: DecoratorList, + digest: Word, + ) -> Self { + Self { + operation_data: OperationData::Batched { op_batches, decorators }, + before_enter: Vec::new(), + after_exit: Vec::new(), + digest: Some(digest), + } + } + /// Used to initialize decorators for the [`BasicBlockNodeBuilder`]. Replaces the existing /// decorators with the given ['DecoratorList']. pub(crate) fn set_decorators(&mut self, decorators: DecoratorList) { - self.decorators = decorators; + match &mut self.operation_data { + OperationData::Raw { decorators: dec, .. } => *dec = decorators, + OperationData::Batched { decorators: dec, .. } => *dec = decorators, + } } /// Builds the BasicBlockNode with the specified decorators. pub fn build(self) -> Result { - if self.operations.is_empty() { - return Err(MastForestError::EmptyBasicBlock); - } + let (op_batches, digest, padded_decorators) = match self.operation_data { + OperationData::Raw { operations, decorators } => { + if operations.is_empty() { + return Err(MastForestError::EmptyBasicBlock); + } - // Validate decorators list (only in debug mode). - #[cfg(debug_assertions)] - validate_decorators(self.operations.len(), &self.decorators); + // Validate decorators list (only in debug mode). + #[cfg(debug_assertions)] + validate_decorators(operations.len(), &decorators); - let (op_batches, computed_digest) = batch_and_hash_ops(self.operations); - // the prior line may have inserted some padding Noops in the op_batches - // the decorator mapping should still point to the correct operation when that happens - let reflowed_decorators = BasicBlockNode::adjust_decorators(self.decorators, &op_batches); + let (op_batches, computed_digest) = batch_and_hash_ops(operations); + // Batch operations (adds padding NOOPs) + // Adjust decorators from raw to padded indices + let padded_decorators = BasicBlockNode::adjust_decorators(decorators, &op_batches); + + // Use the forced digest if provided, otherwise use the computed digest + let digest = self.digest.unwrap_or(computed_digest); - // Use the forced digest if provided, otherwise use the computed digest - let digest = self.digest.unwrap_or(computed_digest); + (op_batches, digest, padded_decorators) + }, + OperationData::Batched { op_batches, decorators } => { + if op_batches.is_empty() { + return Err(MastForestError::EmptyBasicBlock); + } + + // Decorators are already padded - no adjustment needed! + let digest = self.digest.expect("digest must be set for batched operations"); + + (op_batches, digest, decorators) + }, + }; Ok(BasicBlockNode { op_batches, digest, decorators: DecoratorStore::Owned { - decorators: reflowed_decorators, + decorators: padded_decorators, before_enter: self.before_enter.clone(), after_exit: self.after_exit.clone(), }, @@ -1334,31 +1392,47 @@ impl BasicBlockNodeBuilder { impl MastForestContributor for BasicBlockNodeBuilder { fn add_to_forest(self, forest: &mut MastForest) -> Result { - if self.operations.is_empty() { - return Err(MastForestError::EmptyBasicBlock); - } - - // Validate decorators list (only in debug mode). - #[cfg(debug_assertions)] - validate_decorators(self.operations.len(), &self.decorators); - // Determine the node ID that will be assigned let future_node_id = MastNodeId::new_unchecked(forest.nodes.len() as u32); - // Process operations and decorators directly without creating an intermediate Owned node - let (op_batches, computed_digest) = batch_and_hash_ops(self.operations); + // Process based on operation data type + let (op_batches, digest, padded_decorators) = match self.operation_data { + OperationData::Raw { operations, decorators } => { + if operations.is_empty() { + return Err(MastForestError::EmptyBasicBlock); + } + + // Validate decorators list (only in debug mode). + #[cfg(debug_assertions)] + validate_decorators(operations.len(), &decorators); + + // Batch operations (adds padding NOOPs) + let (op_batches, computed_digest) = batch_and_hash_ops(operations); + + // Use the forced digest if provided, otherwise use the computed digest + let digest = self.digest.unwrap_or(computed_digest); - // Use the forced digest if provided, otherwise use the computed digest - let digest = self.digest.unwrap_or(computed_digest); + // Adjust decorator indices from raw to padded + let padded_decorators = BasicBlockNode::adjust_decorators(decorators, &op_batches); - // Adjust decorator indices from raw to padded using the same logic as - // BasicBlockNode::adjust_decorators - let reflowed_decorators = BasicBlockNode::adjust_decorators(self.decorators, &op_batches); + (op_batches, digest, padded_decorators) + }, + OperationData::Batched { op_batches, decorators } => { + if op_batches.is_empty() { + return Err(MastForestError::EmptyBasicBlock); + } + + // Decorators are already padded - no adjustment needed! + let digest = self.digest.expect("digest must be set for batched operations"); + + (op_batches, digest, decorators) + }, + }; // Add decorator info to the forest storage forest .debug_info - .register_op_indexed_decorators(future_node_id, reflowed_decorators) + .register_op_indexed_decorators(future_node_id, padded_decorators) .map_err(MastForestError::DecoratorError)?; // Add node-level decorators to the centralized NodeToDecoratorIds for efficient access @@ -1385,9 +1459,37 @@ impl MastForestContributor for BasicBlockNodeBuilder { // For BasicBlockNode, we need to implement custom logic because BasicBlock has special // decorator handling with operation indices that other nodes don't have - // Compute digest - use forced digest if available, otherwise compute normally - let (op_batches, computed_digest) = batch_and_hash_ops(self.operations.clone()); - let digest = self.digest.unwrap_or(computed_digest); + // Process based on operation data type + let (op_batches, digest, raw_decorators) = match &self.operation_data { + OperationData::Raw { operations, decorators } => { + // Compute digest - use forced digest if available, otherwise compute normally + let (op_batches, computed_digest) = batch_and_hash_ops(operations.clone()); + let digest = self.digest.unwrap_or(computed_digest); + + // Decorators are already in raw form - no conversion needed + #[cfg(debug_assertions)] + { + validate_decorators(operations.len(), decorators); + } + + (op_batches, digest, decorators.clone()) + }, + OperationData::Batched { op_batches, decorators } => { + let digest = self.digest.expect("digest must be set for batched operations"); + + // Convert from padded to raw indices for fingerprinting + let pad2raw = PaddedToRawPrefix::new(op_batches); + let raw_decorators: Vec<(usize, DecoratorId)> = decorators + .iter() + .map(|(padded_idx, decorator_id)| { + let raw_idx = padded_idx - pad2raw[*padded_idx]; + (raw_idx, *decorator_id) + }) + .collect(); + + (op_batches.clone(), digest, raw_decorators) + }, + }; // Collect before_enter decorator fingerprints let before_enter_bytes: Vec<[u8; 32]> = self @@ -1396,21 +1498,8 @@ impl MastForestContributor for BasicBlockNodeBuilder { .map(|&id| forest[id].fingerprint().as_bytes()) .collect(); - // Hash op-indexed decorators using the same logic as node.indexed_decorator_iter() - #[cfg(debug_assertions)] - { - validate_decorators(self.operations.len(), &self.decorators); - } - // For BasicBlockNodeBuilder, convert from padded to raw indices - let pad2raw = PaddedToRawPrefix::new(&op_batches); - let adjusted_decorators: Vec<(usize, DecoratorId)> = self - .decorators - .iter() - .map(|(padded_idx, decorator_id)| { - let raw_idx = padded_idx - pad2raw[*padded_idx]; - (raw_idx, *decorator_id) - }) - .collect(); + // Collect op-indexed decorator data (using raw indices) + let adjusted_decorators = raw_decorators; // Collect op-indexed decorator data let mut op_decorator_data = Vec::with_capacity(adjusted_decorators.len() * 33); diff --git a/core/src/mast/node/basic_block_node/op_batch.rs b/core/src/mast/node/basic_block_node/op_batch.rs index 057bb1812a..26f684765d 100644 --- a/core/src/mast/node/basic_block_node/op_batch.rs +++ b/core/src/mast/node/basic_block_node/op_batch.rs @@ -116,6 +116,27 @@ impl OpBatch { self.num_groups } + /// Creates a new OpBatch from its constituent parts. + /// + /// This constructor is used during deserialization to reconstruct OpBatches with the exact + /// structure they had when serialized. + /// + /// # Arguments + /// * `ops` - The operations in this batch (including padding NOOPs) + /// * `indptr` - Array of group boundary indices + /// * `padding` - Array of padding flags for each group + /// * `groups` - Array of group hashes and immediate values + /// * `num_groups` - Number of groups in this batch + pub(crate) fn new_from_parts( + ops: Vec, + indptr: [usize; Self::BATCH_SIZE_PLUS_ONE], + padding: [bool; BATCH_SIZE], + groups: [Felt; BATCH_SIZE], + num_groups: usize, + ) -> Self { + Self { ops, indptr, padding, groups, num_groups } + } + /// Returns the (op_group_idx, op_idx_in_group) given an operation index in the batch. Returns /// `None` if the index is out of bounds. /// diff --git a/core/src/mast/serialization/basic_blocks.rs b/core/src/mast/serialization/basic_blocks.rs index 57f1fcc6d2..995337f5ae 100644 --- a/core/src/mast/serialization/basic_blocks.rs +++ b/core/src/mast/serialization/basic_blocks.rs @@ -26,13 +26,44 @@ impl BasicBlockDataBuilder { /// Mutators impl BasicBlockDataBuilder { /// Encodes a [`BasicBlockNode`]'s operations into the serialized [`crate::mast::MastForest`] - /// data field. The decorators are not encoded because are stored separately + /// data field. Decorators are stored separately. + /// + /// Operations are written in padded form with batch metadata for exact reconstruction. pub fn encode_basic_block(&mut self, basic_block: &BasicBlockNode) -> NodeDataOffset { let ops_offset = self.node_data.len() as NodeDataOffset; - let operations: Vec = basic_block.raw_operations().copied().collect(); + // Write padded operations + let operations: Vec = basic_block.operations().copied().collect(); operations.write_into(&mut self.node_data); + // Write batch metadata + let op_batches = basic_block.op_batches(); + let num_batches = op_batches.len(); + + // Write number of batches + (num_batches as u32).write_into(&mut self.node_data); + + // Write indptr arrays for each batch (9 u8s per batch, since max index is 72) + for batch in op_batches { + let indptr = batch.indptr(); + for &idx in indptr { + debug_assert!(idx <= 72, "batch index {} exceeds maximum of 72", idx); + (idx as u8).write_into(&mut self.node_data); + } + } + + // Write padding metadata (1 byte per batch, bit-packed) + for batch in op_batches { + let padding = batch.padding(); + let mut packed: u8 = 0; + for (i, &is_padded) in padding.iter().enumerate().take(8) { + if is_padded { + packed |= 1 << i; + } + } + packed.write_into(&mut self.node_data); + } + ops_offset } @@ -58,10 +89,13 @@ impl<'a> BasicBlockDataDecoder<'a> { /// Decoding methods impl BasicBlockDataDecoder<'_> { + /// Reconstructs OpBatches from serialized data, preserving padding and batch structure. pub fn decode_operations( &self, ops_offset: NodeDataOffset, - ) -> Result, DeserializationError> { + ) -> Result, DeserializationError> { + use crate::Felt; + let offset = ops_offset as usize; // Bounds check before slicing to prevent panic @@ -73,10 +107,92 @@ impl BasicBlockDataDecoder<'_> { ))); } - // Read ops let mut ops_data_reader = SliceReader::new(&self.node_data[offset..]); + + // Read padded operations let operations: Vec = ops_data_reader.read()?; - Ok(operations) + // Read batch count + let num_batches: u32 = ops_data_reader.read()?; + let num_batches = num_batches as usize; + + // Read indptr arrays (9 u8s per batch) + let mut batch_indptrs: Vec<[usize; 9]> = Vec::with_capacity(num_batches); + for _ in 0..num_batches { + let mut indptr = [0usize; 9]; + for idx in indptr.iter_mut() { + let val: u8 = ops_data_reader.read()?; + *idx = val as usize; + } + batch_indptrs.push(indptr); + } + + // Read padding metadata (1 byte per batch) + let mut batch_padding: Vec<[bool; 8]> = Vec::with_capacity(num_batches); + for _ in 0..num_batches { + let packed: u8 = ops_data_reader.read()?; + let mut padding = [false; 8]; + for (i, p) in padding.iter_mut().enumerate() { + *p = (packed & (1 << i)) != 0; + } + batch_padding.push(padding); + } + + // Reconstruct OpBatch structures + let mut op_batches: Vec = Vec::with_capacity(num_batches); + let mut global_op_offset = 0; + + for (indptr, padding) in batch_indptrs.iter().zip(batch_padding) { + // Find the highest operation group index + let highest_op_group = (1..=8).rev().find(|&i| indptr[i] > indptr[i - 1]).unwrap_or(1); + + // Extract operations for this batch + let batch_num_ops = indptr[highest_op_group]; + let batch_ops_end = global_op_offset + batch_num_ops; + + let batch_ops: Vec = operations[global_op_offset..batch_ops_end].to_vec(); + + // Reconstruct the groups array and calculate num_groups + // num_groups is the next available slot after all operation groups and immediate values + let mut groups = [Felt::new(0); 8]; + let mut next_group_idx = 0; + + for array_idx in 0..highest_op_group { + let start = indptr[array_idx]; + let end = indptr[array_idx + 1]; + + if start < end { + // This index contains an operation group - compute its hash + let mut group_value: u64 = 0; + for (local_op_idx, op) in batch_ops[start..end].iter().enumerate() { + let opcode = op.op_code() as u64; + group_value |= opcode << (Operation::OP_BITS * local_op_idx); + } + groups[array_idx] = Felt::new(group_value); + next_group_idx = array_idx + 1; + + // Store immediate values from this operation group + for op in &batch_ops[start..end] { + if let Some(imm) = op.imm_value() + && next_group_idx < 8 + { + groups[next_group_idx] = imm; + next_group_idx += 1; + } + } + } + } + + // num_groups is the next available index after all groups and immediates + let num_groups = next_group_idx; + + op_batches.push(crate::mast::OpBatch::new_from_parts( + batch_ops, *indptr, padding, groups, num_groups, + )); + + global_op_offset = batch_ops_end; + } + + Ok(op_batches) } } diff --git a/core/src/mast/serialization/info.rs b/core/src/mast/serialization/info.rs index fd94e5d7f7..4b6b4a5f5c 100644 --- a/core/src/mast/serialization/info.rs +++ b/core/src/mast/serialization/info.rs @@ -48,9 +48,12 @@ impl MastNodeInfo { ) -> Result { match self.ty { MastNodeType::Block { ops_offset } => { - let operations = basic_block_data_decoder.decode_operations(ops_offset)?; - let builder = crate::mast::node::BasicBlockNodeBuilder::new(operations, Vec::new()) - .with_digest(self.digest); + let op_batches = basic_block_data_decoder.decode_operations(ops_offset)?; + let builder = crate::mast::node::BasicBlockNodeBuilder::from_op_batches( + op_batches, + Vec::new(), // decorators set later + self.digest, + ); Ok(MastNodeBuilder::BasicBlock(builder)) }, MastNodeType::Join { left_child_id, right_child_id } => { diff --git a/core/src/mast/serialization/mod.rs b/core/src/mast/serialization/mod.rs index 813dca400d..57b16bcad8 100644 --- a/core/src/mast/serialization/mod.rs +++ b/core/src/mast/serialization/mod.rs @@ -99,7 +99,12 @@ const MAGIC: &[u8; 5] = b"MAST\0"; /// If future modifications are made to this format, the version should be incremented by 1. A /// version of `[255, 255, 255]` is reserved for future extensions that require extending the /// version field itself, but should be considered invalid for now. -const VERSION: [u8; 3] = [0, 0, 0]; +/// +/// Version history: +/// - [0, 0, 0]: Initial format +/// - [0, 0, 1]: Added batch metadata to basic blocks (operations serialized in padded form with +/// indptr, padding, and group metadata for exact OpBatch reconstruction) +const VERSION: [u8; 3] = [0, 0, 1]; // MAST FOREST SERIALIZATION/DESERIALIZATION // ================================================================================================ @@ -149,8 +154,10 @@ impl Serializable for MastForest { let ops_offset = if let MastNode::Block(basic_block) = mast_node { let ops_offset = basic_block_data_builder.encode_basic_block(basic_block); - basic_block_decorators - .push((mast_node_id, basic_block.raw_op_indexed_decorators(self))); + // Serialize decorators with padded indices + let padded_decorators: Vec<(usize, crate::mast::DecoratorId)> = + basic_block.indexed_decorator_iter(self).collect(); + basic_block_decorators.push((mast_node_id, padded_decorators)); ops_offset } else { diff --git a/core/src/mast/serialization/tests.rs b/core/src/mast/serialization/tests.rs index 518514d08d..0c4b60f9bb 100644 --- a/core/src/mast/serialization/tests.rs +++ b/core/src/mast/serialization/tests.rs @@ -651,3 +651,624 @@ fn mast_forest_serialize_deserialize_multiple_procedure_names() { assert_eq!(forest, deserialized); } + +// OPBATCH PRESERVATION TESTS +// ================================================================================================ + +/// Tests that OpBatch structure is exactly preserved during round-trip serialization. +/// +/// Verifies that all OpBatch fields match after deserialization: +/// - ops (including padding NOOPs) +/// - indptr arrays +/// - padding flags +/// - groups arrays +/// - num_groups +#[test] +fn test_opbatch_roundtrip_exact_preservation() { + let mut forest = MastForest::new(); + + // Create operations that will result in specific batching + let operations = vec![ + Operation::Add, + Operation::Mul, + Operation::Push(Felt::new(100)), + Operation::Push(Felt::new(200)), + Operation::Drop, + Operation::Dup0, + ]; + + let basic_block_id = BasicBlockNodeBuilder::new(operations, Vec::new()) + .add_to_forest(&mut forest) + .unwrap(); + + // Get original OpBatch structure + let original_node = forest[basic_block_id].unwrap_basic_block(); + let original_batches = original_node.op_batches(); + + // Serialize and deserialize + let serialized = forest.to_bytes(); + let deserialized_forest = MastForest::read_from_bytes(&serialized).unwrap(); + + // Get deserialized OpBatch structure + let deserialized_node = deserialized_forest[basic_block_id].unwrap_basic_block(); + let deserialized_batches = deserialized_node.op_batches(); + + // Verify exact OpBatch structure match + assert_eq!( + original_batches.len(), + deserialized_batches.len(), + "Number of batches should match" + ); + + for (i, (orig_batch, deser_batch)) in + original_batches.iter().zip(deserialized_batches).enumerate() + { + // Check operations match (including padding NOOPs) + assert_eq!(orig_batch.ops(), deser_batch.ops(), "Batch {i}: Operations don't match"); + + // Check indptr arrays match + assert_eq!( + orig_batch.indptr(), + deser_batch.indptr(), + "Batch {i}: Indptr arrays don't match" + ); + + // Check padding metadata matches + assert_eq!( + orig_batch.padding(), + deser_batch.padding(), + "Batch {i}: Padding metadata doesn't match" + ); + + // Check groups array matches + assert_eq!( + orig_batch.groups(), + deser_batch.groups(), + "Batch {i}: Groups arrays don't match" + ); + + // Check num_groups matches + assert_eq!( + orig_batch.num_groups(), + deser_batch.num_groups(), + "Batch {i}: num_groups doesn't match" + ); + } +} + +/// Tests round-trip with multiple PUSH operations in the same group. +/// +/// Verifies that immediate values are correctly stored and retrieved. +#[test] +fn test_multiple_push_immediates_roundtrip() { + let mut forest = MastForest::new(); + + let operations = vec![ + Operation::Push(Felt::new(100)), + Operation::Push(Felt::new(200)), + Operation::Push(Felt::new(300)), + Operation::Mul, + Operation::Add, + ]; + + let block_id = BasicBlockNodeBuilder::new(operations.clone(), Vec::new()) + .add_to_forest(&mut forest) + .unwrap(); + + let serialized = forest.to_bytes(); + let deserialized = MastForest::read_from_bytes(&serialized).unwrap(); + + let original_node = forest[block_id].unwrap_basic_block(); + let deserialized_node = deserialized[block_id].unwrap_basic_block(); + + // Verify operations match + let original_ops: Vec<_> = original_node.operations().collect(); + let deserialized_ops: Vec<_> = deserialized_node.operations().collect(); + assert_eq!(original_ops, deserialized_ops); + + // Verify OpBatch structure is preserved + assert_eq!( + original_node.op_batches(), + deserialized_node.op_batches(), + "OpBatch structures should be identical" + ); +} + +/// Tests round-trip with operations spanning multiple batches (>72 operations). +#[test] +fn test_multi_batch_roundtrip() { + let mut forest = MastForest::new(); + + // Create 80 operations to span multiple batches + let mut operations = Vec::new(); + for i in 0..80 { + operations.push(Operation::Push(Felt::new(i as u64))); + } + + let block_id = BasicBlockNodeBuilder::new(operations, Vec::new()) + .add_to_forest(&mut forest) + .unwrap(); + + let serialized = forest.to_bytes(); + let deserialized = MastForest::read_from_bytes(&serialized).unwrap(); + + let original_node = forest[block_id].unwrap_basic_block(); + let deserialized_node = deserialized[block_id].unwrap_basic_block(); + + // Verify multiple batches exist + assert!( + original_node.op_batches().len() > 1, + "Should have multiple batches for 80 operations" + ); + + // Verify OpBatch structure is preserved + assert_eq!( + original_node.op_batches().len(), + deserialized_node.op_batches().len(), + "Number of batches should match" + ); + + for (orig, deser) in original_node.op_batches().iter().zip(deserialized_node.op_batches()) { + assert_eq!(orig, deser, "Each OpBatch should be identical"); + } +} + +/// Tests that decorator indices remain correct after round-trip with padded operations. +#[test] +fn test_decorator_indices_preserved_with_padding() { + let mut forest = MastForest::new(); + + let decorator_id = forest.add_decorator(Decorator::Trace(42)).unwrap(); + + let operations = vec![ + Operation::Add, + Operation::Mul, + Operation::Push(Felt::new(100)), // Will cause padding + Operation::Drop, + ]; + + // Add decorator at operation index 2 (the PUSH) + let decorators = vec![(2, decorator_id)]; + + let block_id = BasicBlockNodeBuilder::new(operations.clone(), decorators) + .add_to_forest(&mut forest) + .unwrap(); + + // Serialize and deserialize + let serialized = forest.to_bytes(); + let deserialized_forest = MastForest::read_from_bytes(&serialized).unwrap(); + + // Verify decorator still points to correct operation + let original_node = forest[block_id].unwrap_basic_block(); + let deserialized_node = deserialized_forest[block_id].unwrap_basic_block(); + + let original_decorators: Vec<_> = original_node.indexed_decorator_iter(&forest).collect(); + let deserialized_decorators: Vec<_> = + deserialized_node.indexed_decorator_iter(&deserialized_forest).collect(); + + assert_eq!( + original_decorators, deserialized_decorators, + "Decorator indices should be preserved" + ); + + // Verify the decorator points to the PUSH operation + assert_eq!(deserialized_decorators.len(), 1, "Should have one decorator"); + let (padded_idx, _) = deserialized_decorators[0]; + + // Get the operation at the decorator's index + let op_at_decorator = deserialized_node.operations().nth(padded_idx).unwrap(); + assert!( + matches!(op_at_decorator, Operation::Push(_)), + "Decorator should point to PUSH operation" + ); +} + +// RAW VS BATCHED CONSTRUCTION EQUIVALENCE TESTS +// ================================================================================================ + +/// Tests that Raw and Batched construction paths produce semantically equivalent nodes. +/// +/// This test verifies that a node constructed from raw operations and then deserialized +/// (which uses the Batched path) produces the same semantic result. +#[test] +fn test_raw_vs_batched_construction_equivalence() { + let mut forest1 = MastForest::new(); + let mut forest2 = MastForest::new(); + + let decorator_id1 = forest1.add_decorator(Decorator::Trace(1)).unwrap(); + let _decorator_id2 = forest2.add_decorator(Decorator::Trace(1)).unwrap(); + + let operations = + vec![Operation::Add, Operation::Mul, Operation::Push(Felt::new(100)), Operation::Drop]; + + // Path 1: Raw construction + let block_id1 = BasicBlockNodeBuilder::new(operations.clone(), vec![(2, decorator_id1)]) + .add_to_forest(&mut forest1) + .unwrap(); + + // Path 2: Serialize and deserialize (uses Batched construction) + let serialized = forest1.to_bytes(); + let _deserialized_forest = MastForest::read_from_bytes(&serialized).unwrap(); + + // Manually construct using Batched path to test directly + let original_node = forest1[block_id1].unwrap_basic_block(); + let op_batches = original_node.op_batches().to_vec(); + let digest = original_node.digest(); + let decorators: Vec<_> = original_node.indexed_decorator_iter(&forest1).collect(); + + let block_id2 = BasicBlockNodeBuilder::from_op_batches(op_batches, decorators, digest) + .add_to_forest(&mut forest2) + .unwrap(); + + // Verify nodes are semantically equivalent + let node1 = forest1[block_id1].unwrap_basic_block(); + let node2 = forest2[block_id2].unwrap_basic_block(); + + // Check operations match + let ops1: Vec<_> = node1.operations().collect(); + let ops2: Vec<_> = node2.operations().collect(); + assert_eq!(ops1, ops2, "Operations should match"); + + // Check OpBatch structure matches + assert_eq!(node1.op_batches(), node2.op_batches(), "OpBatch structures should match"); + + // Check digest matches + assert_eq!(node1.digest(), node2.digest(), "Digests should match"); + + // Check decorators match + let decorators1: Vec<_> = node1.indexed_decorator_iter(&forest1).collect(); + let decorators2: Vec<_> = node2.indexed_decorator_iter(&forest2).collect(); + assert_eq!(decorators1, decorators2, "Decorators should match"); +} + +/// Tests that Raw and Batched construction produce the same digest. +#[test] +fn test_raw_batched_digest_equivalence() { + let operations = vec![ + Operation::Add, + Operation::Mul, + Operation::Push(Felt::new(42)), + Operation::Drop, + Operation::Dup0, + ]; + + // Construct via Raw path + let mut forest1 = MastForest::new(); + let block_id1 = BasicBlockNodeBuilder::new(operations.clone(), Vec::new()) + .add_to_forest(&mut forest1) + .unwrap(); + let digest1 = forest1[block_id1].unwrap_basic_block().digest(); + + // Construct via Batched path (via serialization round-trip) + let serialized = forest1.to_bytes(); + let deserialized = MastForest::read_from_bytes(&serialized).unwrap(); + let digest2 = deserialized[block_id1].unwrap_basic_block().digest(); + + assert_eq!(digest1, digest2, "Digests from Raw and Batched paths should match"); +} + +/// Tests that Batched construction preserves the exact OpBatch structure. +/// +/// This verifies that the Batched path doesn't inadvertently re-batch operations. +#[test] +fn test_batched_construction_preserves_structure() { + let mut forest = MastForest::new(); + + let operations = vec![ + Operation::Add, + Operation::Mul, + Operation::Push(Felt::new(100)), + Operation::Push(Felt::new(200)), + ]; + + let block_id = BasicBlockNodeBuilder::new(operations, Vec::new()) + .add_to_forest(&mut forest) + .unwrap(); + + // Get the OpBatches from the original node + let original_node = forest[block_id].unwrap_basic_block(); + let original_batches = original_node.op_batches().to_vec(); + let original_digest = original_node.digest(); + + // Construct a new node using the Batched path + let mut forest2 = MastForest::new(); + let block_id2 = BasicBlockNodeBuilder::from_op_batches( + original_batches.clone(), + Vec::new(), + original_digest, + ) + .add_to_forest(&mut forest2) + .unwrap(); + + // Verify the OpBatch structure is exactly preserved + let new_node = forest2[block_id2].unwrap_basic_block(); + assert_eq!( + original_batches, + new_node.op_batches(), + "OpBatch structure should be exactly preserved" + ); +} + +// PROPTEST-BASED ROUND-TRIP SERIALIZATION TESTS +// ================================================================================================ + +mod proptests { + use proptest::{prelude::*, strategy::Just}; + + use super::*; + use crate::{ + Decorator, + mast::{BasicBlockNodeBuilder, MastForest, MastNode, arbitrary::MastForestParams}, + }; + + proptest! { + /// Property test: any MastForest should round-trip through serialization + #[test] + fn proptest_mast_forest_roundtrip( + forest in any_with::(MastForestParams { + decorators: 5, + blocks: 1..=5, + max_joins: 3, + max_splits: 2, + max_loops: 2, + max_calls: 2, + max_syscalls: 0, // Avoid syscalls in roundtrip tests + max_externals: 1, + max_dyns: 1, + }) + ) { + // Serialize + let serialized = forest.to_bytes(); + + // Deserialize + let deserialized = MastForest::read_from_bytes(&serialized) + .expect("Deserialization should succeed"); + + // Verify node count + prop_assert_eq!( + forest.num_nodes(), + deserialized.num_nodes(), + "Node count should match" + ); + + // Verify all nodes match + for (idx, original) in forest.nodes().iter().enumerate() { + let node_id = crate::mast::MastNodeId::new_unchecked(idx as u32); + let deserialized_node = &deserialized[node_id]; + + // Check digests match + prop_assert_eq!( + original.digest(), + deserialized_node.digest(), + "Node {:?} digest mismatch", node_id + ); + + // For basic blocks, verify OpBatch structure is preserved + if let MastNode::Block(original_block) = original && let MastNode::Block(deserialized_block) = deserialized_node { + let orig_batches = original_block.op_batches(); + let deser_batches = deserialized_block.op_batches(); + + prop_assert_eq!( + orig_batches.len(), + deser_batches.len(), + "Node {:?}: Batch count mismatch", node_id + ); + + for (batch_idx, (orig_batch, deser_batch)) in + orig_batches.iter().zip(deser_batches).enumerate() + { + prop_assert_eq!( + orig_batch.ops(), + deser_batch.ops(), + "Node {:?} Batch {}: Operations mismatch", node_id, batch_idx + ); + prop_assert_eq!( + orig_batch.indptr(), + deser_batch.indptr(), + "Node {:?} Batch {}: Indptr mismatch", node_id, batch_idx + ); + prop_assert_eq!( + orig_batch.padding(), + deser_batch.padding(), + "Node {:?} Batch {}: Padding mismatch", node_id, batch_idx + ); + prop_assert_eq!( + orig_batch.groups(), + deser_batch.groups(), + "Node {:?} Batch {}: Groups mismatch", node_id, batch_idx + ); + prop_assert_eq!( + orig_batch.num_groups(), + deser_batch.num_groups(), + "Node {:?} Batch {}: num_groups mismatch", node_id, batch_idx + ); + } + + // Verify decorators are preserved + let orig_decorators: Vec<_> = + original_block.indexed_decorator_iter(&forest).collect(); + let deser_decorators: Vec<_> = + deserialized_block.indexed_decorator_iter(&deserialized).collect(); + + prop_assert_eq!( + orig_decorators.len(), + deser_decorators.len(), + "Node {:?}: Decorator count mismatch", node_id + ); + + for ((orig_idx, orig_dec_id), (deser_idx, deser_dec_id)) in + orig_decorators.iter().zip(&deser_decorators) + { + prop_assert_eq!( + orig_idx, + deser_idx, + "Node {:?}: Decorator index mismatch", node_id + ); + prop_assert_eq!( + forest.decorator_by_id(*orig_dec_id), + deserialized.decorator_by_id(*deser_dec_id), + "Node {:?}: Decorator content mismatch", node_id + ); + } + } + + } + } + + /// Property test: multi-batch basic blocks should preserve exact structure + #[test] + fn proptest_multi_batch_roundtrip( + ops in prop::collection::vec( + prop::sample::select(vec![ + crate::Operation::Add, + crate::Operation::Mul, + crate::Operation::Push(crate::Felt::new(42)), + crate::Operation::Drop, + crate::Operation::Dup0, + crate::Operation::Swap, + ]), + 73..=150 // Generate 73-150 operations for multi-batch testing + ) + ) { + // Create a forest and add the block + let mut forest = MastForest::new(); + + let block_id = BasicBlockNodeBuilder::new(ops, Vec::new()) + .add_to_forest(&mut forest) + .unwrap(); + + let original_block = forest[block_id].unwrap_basic_block(); + let original_batches = original_block.op_batches(); + + // Verify we have multiple batches + prop_assume!(original_batches.len() > 1, "Need multiple batches for this test"); + + // Serialize and deserialize + let serialized = forest.to_bytes(); + let deserialized_forest = MastForest::read_from_bytes(&serialized) + .expect("Deserialization should succeed"); + + let deserialized_block = deserialized_forest[block_id].unwrap_basic_block(); + let deserialized_batches = deserialized_block.op_batches(); + + // Verify batch count + prop_assert_eq!( + original_batches.len(), + deserialized_batches.len(), + "Batch count should match" + ); + + // Verify every batch field matches exactly + for (i, (orig_batch, deser_batch)) in + original_batches.iter().zip(deserialized_batches).enumerate() + { + prop_assert_eq!( + orig_batch.ops(), + deser_batch.ops(), + "Batch {}: Operations should match exactly", i + ); + prop_assert_eq!( + orig_batch.indptr(), + deser_batch.indptr(), + "Batch {}: Indptr arrays should match exactly", i + ); + prop_assert_eq!( + orig_batch.padding(), + deser_batch.padding(), + "Batch {}: Padding metadata should match exactly", i + ); + prop_assert_eq!( + orig_batch.groups(), + deser_batch.groups(), + "Batch {}: Groups arrays should match exactly", i + ); + prop_assert_eq!( + orig_batch.num_groups(), + deser_batch.num_groups(), + "Batch {}: num_groups should match exactly", i + ); + } + } + + /// Property test: basic blocks with decorators should preserve decorator indices + #[test] + fn proptest_decorator_indices_roundtrip( + (ops, decorator_indices) in ( + prop::collection::vec( + prop::sample::select(vec![ + crate::Operation::Add, + crate::Operation::Mul, + crate::Operation::Push(crate::Felt::new(99)), + crate::Operation::Drop, + crate::Operation::Dup0, + ]), + 10..=50 + ) + ).prop_flat_map(|ops| { + let ops_len = ops.len(); + ( + Just(ops), + prop::collection::vec((0..ops_len, 0..5_u32), 1..=10) + ) + }) + ) { + // Create a forest and add decorators + let mut forest = MastForest::new(); + let decorator_id1 = forest.add_decorator(Decorator::Trace(1)).unwrap(); + let decorator_id2 = forest.add_decorator(Decorator::Trace(2)).unwrap(); + let decorator_id3 = forest.add_decorator(Decorator::Trace(3)).unwrap(); + let decorator_id4 = forest.add_decorator(Decorator::Trace(4)).unwrap(); + let decorator_id5 = forest.add_decorator(Decorator::Trace(5)).unwrap(); + let decorator_ids = [decorator_id1, decorator_id2, decorator_id3, decorator_id4, decorator_id5]; + + // Map indices to actual decorator IDs and sort by index + let mut decorators: Vec<(usize, _)> = decorator_indices + .into_iter() + .map(|(idx, dec_id_idx)| (idx, decorator_ids[dec_id_idx as usize])) + .collect(); + decorators.sort_by_key(|(idx, _)| *idx); + decorators.dedup_by_key(|(idx, _)| *idx); // Remove duplicates + + let block_id = BasicBlockNodeBuilder::new(ops, decorators) + .add_to_forest(&mut forest) + .unwrap(); + + let original_block = forest[block_id].unwrap_basic_block(); + + // Serialize and deserialize + let serialized = forest.to_bytes(); + let deserialized_forest = MastForest::read_from_bytes(&serialized) + .expect("Deserialization should succeed"); + + let deserialized_block = deserialized_forest[block_id].unwrap_basic_block(); + + // Verify decorator indices and content match + let orig_decorators: Vec<_> = + original_block.indexed_decorator_iter(&forest).collect(); + let deser_decorators: Vec<_> = + deserialized_block.indexed_decorator_iter(&deserialized_forest).collect(); + + prop_assert_eq!( + orig_decorators.len(), + deser_decorators.len(), + "Decorator count should match" + ); + + for ((orig_idx, orig_dec_id), (deser_idx, deser_dec_id)) in + orig_decorators.iter().zip(&deser_decorators) + { + prop_assert_eq!( + orig_idx, + deser_idx, + "Decorator indices should match (padded form)" + ); + + prop_assert_eq!( + forest.decorator_by_id(*orig_dec_id), + deserialized_forest.decorator_by_id(*deser_dec_id), + "Decorator content should match" + ); + } + } + } +} From 144d2eb534f83c575bebf8664fa1cd1cb97e81d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Fri, 12 Dec 2025 00:32:58 -0500 Subject: [PATCH 3/7] docs(core): add wire format schematic for basic block serialization Add module-level documentation showing the over-the-wire format for basic blocks with byte consumption formula. --- core/src/mast/serialization/basic_blocks.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/core/src/mast/serialization/basic_blocks.rs b/core/src/mast/serialization/basic_blocks.rs index 995337f5ae..0737ecb8b2 100644 --- a/core/src/mast/serialization/basic_blocks.rs +++ b/core/src/mast/serialization/basic_blocks.rs @@ -1,3 +1,14 @@ +//! Basic block serialization format. +//! +//! ## Wire Format +//! +//! - Padded operations (variable size) +//! - Batch count (4 bytes) +//! - Indptr array per batch (9 bytes each) +//! - Padding flags per batch (1 byte each, bit-packed) +//! +//! **Total**: `ops_size + 4 + (10 * num_batches)` bytes + use alloc::vec::Vec; use super::NodeDataOffset; From d7b9b402e0f658dbddde5f28e8a9760dab53a792 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Fri, 12 Dec 2025 13:52:47 -0500 Subject: [PATCH 4/7] refactor(core): optimize BasicBlockNode::to_builder to avoid re-batching Modified BasicBlockNode::to_builder to directly use pre-batched operations and padded decorators already stored in the node, eliminating redundant re-batching and decorator adjustment. The implementation now: - Uses from_op_batches constructor to preserve existing op_batches - Extracts padded decorators directly from Owned or Linked storage - Avoids wasteful extraction of unpadded operations followed by re-batching --- core/src/mast/node/basic_block_node/mod.rs | 25 ++- core/src/mast/node/basic_block_node/tests.rs | 221 +++++++++++++++---- 2 files changed, 191 insertions(+), 55 deletions(-) diff --git a/core/src/mast/node/basic_block_node/mod.rs b/core/src/mast/node/basic_block_node/mod.rs index 3bb5bb1144..ad3ffd9932 100644 --- a/core/src/mast/node/basic_block_node/mod.rs +++ b/core/src/mast/node/basic_block_node/mod.rs @@ -648,20 +648,29 @@ impl MastNodeExt for BasicBlockNode { type Builder = BasicBlockNodeBuilder; fn to_builder(self, forest: &MastForest) -> Self::Builder { - let operations: Vec = self.raw_operations().cloned().collect(); - let un_adjusted_decorators = self.raw_op_indexed_decorators(forest); - - let (before_enter, after_exit) = match self.decorators { - DecoratorStore::Owned { before_enter, after_exit, .. } => (before_enter, after_exit), + // Extract padded decorators and before_enter/after_exit based on storage type + let (padded_decorators, before_enter, after_exit) = match self.decorators { + DecoratorStore::Owned { decorators, before_enter, after_exit } => { + // Decorators are already padded in Owned storage + (decorators, before_enter, after_exit) + }, DecoratorStore::Linked { id } => { - // For linked nodes, get the decorators from the forest's NodeToDecoratorIds + // For linked nodes, get decorators from forest's centralized storage + // The decorators are already padded in the centralized storage + let padded_decorators: DecoratorList = forest + .debug_info + .decorator_links_for_node(id) + .expect("node must exist in forest") + .into_iter() + .collect(); let before_enter = forest.before_enter_decorators(id).to_vec(); let after_exit = forest.after_exit_decorators(id).to_vec(); - (before_enter, after_exit) + (padded_decorators, before_enter, after_exit) }, }; - BasicBlockNodeBuilder::new(operations, un_adjusted_decorators) + // Use from_op_batches to avoid re-batching and re-adjusting decorators + BasicBlockNodeBuilder::from_op_batches(self.op_batches, padded_decorators, self.digest) .with_before_enter(before_enter) .with_after_exit(after_exit) } diff --git a/core/src/mast/node/basic_block_node/tests.rs b/core/src/mast/node/basic_block_node/tests.rs index 3eccbfb0c4..cc1ebbaf44 100644 --- a/core/src/mast/node/basic_block_node/tests.rs +++ b/core/src/mast/node/basic_block_node/tests.rs @@ -1,9 +1,7 @@ use proptest::prelude::*; // Import strategy functions from arbitrary.rs -pub(super) use super::arbitrary::{ - BasicBlockNodeParams, MastForestParams, op_non_control_sequence_strategy, -}; +pub(super) use super::arbitrary::op_non_control_sequence_strategy; use super::*; use crate::{ Decorator, Felt, ONE, Word, @@ -803,53 +801,182 @@ fn test_basic_block_fingerprint_uses_forced_digest() { ); } -// PROPTERY TESTS WITH ARBITRARY -// ================================================================================================ +/// Test that BasicBlockNode -> to_builder -> build is an identity transformation for Owned storage +#[test] +fn test_to_builder_identity_owned() { + let ops = vec![ + Operation::Push(Felt::new(1)), + Operation::Push(Felt::new(2)), + Operation::Add, + Operation::Push(Felt::new(3)), + Operation::Mul, + ]; -proptest! { - /// Property test: All randomly generated BasicBlockNodes should pass validation - #[test] - fn prop_validate_arbitrary_basic_block( - // Generate BasicBlockNode with custom parameters to increase test coverage - block in any_with::(BasicBlockNodeParams { - max_ops_len: 72, // Allow larger blocks for more complex batching - max_pairs: 5, // Allow more decorators - max_decorator_id_u32: 10, - }) - ) { - // All BasicBlockNodes created through the official constructor should pass validation - // This property test ensures our validation correctly handles all edge cases - // that the official BasicBlockNode::new() function produces - prop_assert!( - block.validate_batch_invariants().is_ok(), - "BasicBlockNode validation failed: {:?}", - block.validate_batch_invariants().err() - ); + let mut forest = MastForest::new(); + let deco1 = forest.add_decorator(Decorator::Trace(1)).unwrap(); + let deco2 = forest.add_decorator(Decorator::Trace(2)).unwrap(); + let deco3 = forest.add_decorator(Decorator::Trace(3)).unwrap(); + + let decorators = vec![(0, deco1), (2, deco2), (4, deco3)]; + + let original = BasicBlockNodeBuilder::new(ops.clone(), decorators.clone()) + .with_before_enter(vec![deco1]) + .with_after_exit(vec![deco2]) + .build() + .unwrap(); + + // Clone the original node to preserve it + let node_for_roundtrip = original.clone(); + + // Perform the identity transformation: node -> builder -> node + let builder = node_for_roundtrip.to_builder(&forest); + let roundtrip = builder.build().unwrap(); + + // Verify identity: all fields should be identical + assert_eq!(original.op_batches, roundtrip.op_batches, "OpBatches should be identical"); + assert_eq!(original.digest, roundtrip.digest, "Digest should be identical"); + + // Extract decorators for comparison + match (&original.decorators, &roundtrip.decorators) { + ( + DecoratorStore::Owned { + decorators: orig_decs, + before_enter: orig_before, + after_exit: orig_after, + }, + DecoratorStore::Owned { + decorators: rt_decs, + before_enter: rt_before, + after_exit: rt_after, + }, + ) => { + assert_eq!(orig_decs, rt_decs, "Operation decorators should be identical"); + assert_eq!(orig_before, rt_before, "Before-enter decorators should be identical"); + assert_eq!(orig_after, rt_after, "After-exit decorators should be identical"); + }, + _ => panic!("Expected Owned decorator storage"), + } +} + +/// Test that BasicBlockNode -> to_builder -> build is an identity transformation for Linked storage +#[test] +fn test_to_builder_identity_linked() { + let ops = vec![ + Operation::Push(Felt::new(10)), + Operation::Push(Felt::new(20)), + Operation::Add, + Operation::Mul, + ]; + + let mut forest = MastForest::new(); + let deco1 = forest.add_decorator(Decorator::Trace(10)).unwrap(); + let deco2 = forest.add_decorator(Decorator::Trace(20)).unwrap(); + + let decorators = vec![(1, deco1), (3, deco2)]; + + // Add node to forest so it gets Linked storage + let node_id = BasicBlockNodeBuilder::new(ops.clone(), decorators.clone()) + .with_before_enter(vec![deco1]) + .with_after_exit(vec![deco2]) + .add_to_forest(&mut forest) + .unwrap(); + + // Get the original node (with Linked storage) + let original = match &forest[node_id] { + MastNode::Block(block) => block.clone(), + _ => panic!("Expected BasicBlockNode"), + }; + + // Verify it has Linked storage + assert!(matches!(original.decorators, DecoratorStore::Linked { .. })); + + // Perform the identity transformation: node -> builder -> node + let builder = original.clone().to_builder(&forest); + let roundtrip = builder.build().unwrap(); + + // Verify identity: all fields should be identical + assert_eq!(original.op_batches, roundtrip.op_batches, "OpBatches should be identical"); + assert_eq!(original.digest, roundtrip.digest, "Digest should be identical"); + + // For Linked -> Owned transition, verify the decorators are equivalent + match &roundtrip.decorators { + DecoratorStore::Owned { + decorators: rt_decs, + before_enter: rt_before, + after_exit: rt_after, + } => { + // Get expected decorators from forest + let expected_decs: DecoratorList = forest + .debug_info + .decorator_links_for_node(node_id) + .unwrap() + .into_iter() + .collect(); + let expected_before = forest.before_enter_decorators(node_id); + let expected_after = forest.after_exit_decorators(node_id); + + assert_eq!(rt_decs, &expected_decs, "Operation decorators should match forest"); + assert_eq!( + rt_before.as_slice(), + expected_before, + "Before-enter decorators should match forest" + ); + assert_eq!( + rt_after.as_slice(), + expected_after, + "After-exit decorators should match forest" + ); + }, + _ => panic!("Expected Owned decorator storage after to_builder"), } +} - /// Property test: All randomly generated MastForests should pass validation +proptest! { + /// Proptest: verify to_builder identity for arbitrary BasicBlockNodes #[test] - fn prop_validate_arbitrary_mast_forest( - // Generate MastForest with BasicBlockNodes that should all be valid - forest in any_with::(MastForestParams { - decorators: 5, - blocks: 1..=5, // Test with multiple blocks - max_joins: 2, - max_splits: 2, - max_loops: 1, - max_calls: 2, - max_syscalls: 0, // Avoid syscalls that require kernel setup - max_externals: 0, // Avoid externals with random digests - max_dyns: 0, // Avoid dyn nodes that leave junk on stack - }) - ) { - // All MastForests generated through the official Arbitrary implementation - // should pass validation since they use BasicBlockNode::new() which enforces - // the same invariants we're checking - prop_assert!( - forest.validate().is_ok(), - "MastForest validation failed: {:?}", - forest.validate().err() - ); + fn proptest_to_builder_identity(ops in op_non_control_sequence_strategy(20)) { + let mut forest = MastForest::new(); + + // Create some decorators + let num_decorators = (ops.len() / 3).max(1); + let decorator_ids: Vec<_> = (0..num_decorators) + .map(|i| forest.add_decorator(Decorator::Trace(i as u32)).unwrap()) + .collect(); + + // Create decorator list with random positions + let decorators: Vec<_> = ops + .iter() + .enumerate() + .filter(|(i, _)| i % 3 == 0) + .zip(decorator_ids.iter().cycle()) + .map(|((idx, _), &dec_id)| (idx, dec_id)) + .collect(); + + // Test with Owned storage + let original = BasicBlockNodeBuilder::new(ops.clone(), decorators.clone()) + .build() + .unwrap(); + + let builder = original.clone().to_builder(&forest); + let roundtrip = builder.build().unwrap(); + + prop_assert_eq!(original.op_batches, roundtrip.op_batches); + prop_assert_eq!(original.digest, roundtrip.digest); + + // Test with Linked storage + let node_id = BasicBlockNodeBuilder::new(ops, decorators) + .add_to_forest(&mut forest) + .unwrap(); + + let linked_node = match &forest[node_id] { + MastNode::Block(block) => block.clone(), + _ => panic!("Expected BasicBlockNode"), + }; + + let builder2 = linked_node.clone().to_builder(&forest); + let roundtrip2 = builder2.build().unwrap(); + + prop_assert_eq!(linked_node.op_batches, roundtrip2.op_batches); + prop_assert_eq!(linked_node.digest, roundtrip2.digest); } } From 62361a449e2aa805928cab0565c7e0a029301c92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Fri, 12 Dec 2025 15:04:43 -0500 Subject: [PATCH 5/7] chore: Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac4063cfaa..25a0433038 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ - [BREAKING] Removed `miden debug`, `miden analyze` and `miden repl` ([#2483](https://github.com/0xMiden/miden-vm/pull/2483)). - [BREAKING] Change backend from winterfell to Plonky3 ([#2472](https://github.com/0xMiden/miden-vm/pull/2472)). - Added validation of `core_trace_fragment_size` in `ExecutionOptions` ([#2528](https://github.com/0xMiden/miden-vm/pull/2528)). +- [BREAKING] Change serialization of `BasicBlockNode`s to use padded indices ([#2466](https://github.com/0xMiden/miden-vm/pull/2466/)). ## 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)). From c1d389eee62499219a04dfba1523a5108b183197 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Fri, 12 Dec 2025 15:09:59 -0500 Subject: [PATCH 6/7] refactor(core): simplify unit tests - Merged test_to_builder_identity_{owned,linked} into single test covering both storage types - Simplified OpBatch roundtrip tests by using PartialEq instead of checking each field individually - Simplified proptest assertions to compare OpBatch directly instead of checking ops, indptr, padding, groups, num_groups separately --- core/src/mast/node/basic_block_node/tests.rs | 118 ++------- core/src/mast/serialization/tests.rs | 238 ++++--------------- 2 files changed, 57 insertions(+), 299 deletions(-) diff --git a/core/src/mast/node/basic_block_node/tests.rs b/core/src/mast/node/basic_block_node/tests.rs index cc1ebbaf44..92220a0b15 100644 --- a/core/src/mast/node/basic_block_node/tests.rs +++ b/core/src/mast/node/basic_block_node/tests.rs @@ -801,134 +801,44 @@ fn test_basic_block_fingerprint_uses_forced_digest() { ); } -/// Test that BasicBlockNode -> to_builder -> build is an identity transformation for Owned storage +/// Test that BasicBlockNode -> to_builder -> build preserves structure and decorators #[test] -fn test_to_builder_identity_owned() { +fn test_to_builder_identity() { let ops = vec![ Operation::Push(Felt::new(1)), Operation::Push(Felt::new(2)), Operation::Add, - Operation::Push(Felt::new(3)), Operation::Mul, ]; let mut forest = MastForest::new(); let deco1 = forest.add_decorator(Decorator::Trace(1)).unwrap(); let deco2 = forest.add_decorator(Decorator::Trace(2)).unwrap(); - let deco3 = forest.add_decorator(Decorator::Trace(3)).unwrap(); - - let decorators = vec![(0, deco1), (2, deco2), (4, deco3)]; + let decorators = vec![(0, deco1), (2, deco2)]; - let original = BasicBlockNodeBuilder::new(ops.clone(), decorators.clone()) + // Test Owned storage: node -> builder -> node + let owned = BasicBlockNodeBuilder::new(ops.clone(), decorators.clone()) .with_before_enter(vec![deco1]) .with_after_exit(vec![deco2]) .build() .unwrap(); + let owned_rt = owned.clone().to_builder(&forest).build().unwrap(); + assert_eq!(owned.op_batches, owned_rt.op_batches); + assert_eq!(owned.digest, owned_rt.digest); - // Clone the original node to preserve it - let node_for_roundtrip = original.clone(); - - // Perform the identity transformation: node -> builder -> node - let builder = node_for_roundtrip.to_builder(&forest); - let roundtrip = builder.build().unwrap(); - - // Verify identity: all fields should be identical - assert_eq!(original.op_batches, roundtrip.op_batches, "OpBatches should be identical"); - assert_eq!(original.digest, roundtrip.digest, "Digest should be identical"); - - // Extract decorators for comparison - match (&original.decorators, &roundtrip.decorators) { - ( - DecoratorStore::Owned { - decorators: orig_decs, - before_enter: orig_before, - after_exit: orig_after, - }, - DecoratorStore::Owned { - decorators: rt_decs, - before_enter: rt_before, - after_exit: rt_after, - }, - ) => { - assert_eq!(orig_decs, rt_decs, "Operation decorators should be identical"); - assert_eq!(orig_before, rt_before, "Before-enter decorators should be identical"); - assert_eq!(orig_after, rt_after, "After-exit decorators should be identical"); - }, - _ => panic!("Expected Owned decorator storage"), - } -} - -/// Test that BasicBlockNode -> to_builder -> build is an identity transformation for Linked storage -#[test] -fn test_to_builder_identity_linked() { - let ops = vec![ - Operation::Push(Felt::new(10)), - Operation::Push(Felt::new(20)), - Operation::Add, - Operation::Mul, - ]; - - let mut forest = MastForest::new(); - let deco1 = forest.add_decorator(Decorator::Trace(10)).unwrap(); - let deco2 = forest.add_decorator(Decorator::Trace(20)).unwrap(); - - let decorators = vec![(1, deco1), (3, deco2)]; - - // Add node to forest so it gets Linked storage - let node_id = BasicBlockNodeBuilder::new(ops.clone(), decorators.clone()) + // Test Linked storage: node in forest -> builder -> node + let node_id = BasicBlockNodeBuilder::new(ops, decorators) .with_before_enter(vec![deco1]) .with_after_exit(vec![deco2]) .add_to_forest(&mut forest) .unwrap(); - - // Get the original node (with Linked storage) - let original = match &forest[node_id] { + let linked = match &forest[node_id] { MastNode::Block(block) => block.clone(), _ => panic!("Expected BasicBlockNode"), }; - - // Verify it has Linked storage - assert!(matches!(original.decorators, DecoratorStore::Linked { .. })); - - // Perform the identity transformation: node -> builder -> node - let builder = original.clone().to_builder(&forest); - let roundtrip = builder.build().unwrap(); - - // Verify identity: all fields should be identical - assert_eq!(original.op_batches, roundtrip.op_batches, "OpBatches should be identical"); - assert_eq!(original.digest, roundtrip.digest, "Digest should be identical"); - - // For Linked -> Owned transition, verify the decorators are equivalent - match &roundtrip.decorators { - DecoratorStore::Owned { - decorators: rt_decs, - before_enter: rt_before, - after_exit: rt_after, - } => { - // Get expected decorators from forest - let expected_decs: DecoratorList = forest - .debug_info - .decorator_links_for_node(node_id) - .unwrap() - .into_iter() - .collect(); - let expected_before = forest.before_enter_decorators(node_id); - let expected_after = forest.after_exit_decorators(node_id); - - assert_eq!(rt_decs, &expected_decs, "Operation decorators should match forest"); - assert_eq!( - rt_before.as_slice(), - expected_before, - "Before-enter decorators should match forest" - ); - assert_eq!( - rt_after.as_slice(), - expected_after, - "After-exit decorators should match forest" - ); - }, - _ => panic!("Expected Owned decorator storage after to_builder"), - } + let linked_rt = linked.clone().to_builder(&forest).build().unwrap(); + assert_eq!(linked.op_batches, linked_rt.op_batches); + assert_eq!(linked.digest, linked_rt.digest); } proptest! { diff --git a/core/src/mast/serialization/tests.rs b/core/src/mast/serialization/tests.rs index 0c4b60f9bb..44588b6ebe 100644 --- a/core/src/mast/serialization/tests.rs +++ b/core/src/mast/serialization/tests.rs @@ -655,162 +655,46 @@ fn mast_forest_serialize_deserialize_multiple_procedure_names() { // OPBATCH PRESERVATION TESTS // ================================================================================================ -/// Tests that OpBatch structure is exactly preserved during round-trip serialization. -/// -/// Verifies that all OpBatch fields match after deserialization: -/// - ops (including padding NOOPs) -/// - indptr arrays -/// - padding flags -/// - groups arrays -/// - num_groups +/// Tests that OpBatch structure is preserved during round-trip serialization #[test] -fn test_opbatch_roundtrip_exact_preservation() { +fn test_opbatch_roundtrip_preservation() { let mut forest = MastForest::new(); - // Create operations that will result in specific batching let operations = vec![ Operation::Add, - Operation::Mul, - Operation::Push(Felt::new(100)), - Operation::Push(Felt::new(200)), - Operation::Drop, - Operation::Dup0, - ]; - - let basic_block_id = BasicBlockNodeBuilder::new(operations, Vec::new()) - .add_to_forest(&mut forest) - .unwrap(); - - // Get original OpBatch structure - let original_node = forest[basic_block_id].unwrap_basic_block(); - let original_batches = original_node.op_batches(); - - // Serialize and deserialize - let serialized = forest.to_bytes(); - let deserialized_forest = MastForest::read_from_bytes(&serialized).unwrap(); - - // Get deserialized OpBatch structure - let deserialized_node = deserialized_forest[basic_block_id].unwrap_basic_block(); - let deserialized_batches = deserialized_node.op_batches(); - - // Verify exact OpBatch structure match - assert_eq!( - original_batches.len(), - deserialized_batches.len(), - "Number of batches should match" - ); - - for (i, (orig_batch, deser_batch)) in - original_batches.iter().zip(deserialized_batches).enumerate() - { - // Check operations match (including padding NOOPs) - assert_eq!(orig_batch.ops(), deser_batch.ops(), "Batch {i}: Operations don't match"); - - // Check indptr arrays match - assert_eq!( - orig_batch.indptr(), - deser_batch.indptr(), - "Batch {i}: Indptr arrays don't match" - ); - - // Check padding metadata matches - assert_eq!( - orig_batch.padding(), - deser_batch.padding(), - "Batch {i}: Padding metadata doesn't match" - ); - - // Check groups array matches - assert_eq!( - orig_batch.groups(), - deser_batch.groups(), - "Batch {i}: Groups arrays don't match" - ); - - // Check num_groups matches - assert_eq!( - orig_batch.num_groups(), - deser_batch.num_groups(), - "Batch {i}: num_groups doesn't match" - ); - } -} - -/// Tests round-trip with multiple PUSH operations in the same group. -/// -/// Verifies that immediate values are correctly stored and retrieved. -#[test] -fn test_multiple_push_immediates_roundtrip() { - let mut forest = MastForest::new(); - - let operations = vec![ Operation::Push(Felt::new(100)), Operation::Push(Felt::new(200)), - Operation::Push(Felt::new(300)), Operation::Mul, - Operation::Add, ]; - let block_id = BasicBlockNodeBuilder::new(operations.clone(), Vec::new()) + let block_id = BasicBlockNodeBuilder::new(operations, Vec::new()) .add_to_forest(&mut forest) .unwrap(); - let serialized = forest.to_bytes(); - let deserialized = MastForest::read_from_bytes(&serialized).unwrap(); - - let original_node = forest[block_id].unwrap_basic_block(); - let deserialized_node = deserialized[block_id].unwrap_basic_block(); - - // Verify operations match - let original_ops: Vec<_> = original_node.operations().collect(); - let deserialized_ops: Vec<_> = deserialized_node.operations().collect(); - assert_eq!(original_ops, deserialized_ops); + let original = forest[block_id].unwrap_basic_block(); + let deserialized_forest = MastForest::read_from_bytes(&forest.to_bytes()).unwrap(); + let deserialized = deserialized_forest[block_id].unwrap_basic_block(); - // Verify OpBatch structure is preserved - assert_eq!( - original_node.op_batches(), - deserialized_node.op_batches(), - "OpBatch structures should be identical" - ); + assert_eq!(original.op_batches(), deserialized.op_batches()); } -/// Tests round-trip with operations spanning multiple batches (>72 operations). +/// Tests OpBatch preservation with multiple batches (>72 operations) #[test] fn test_multi_batch_roundtrip() { let mut forest = MastForest::new(); - - // Create 80 operations to span multiple batches - let mut operations = Vec::new(); - for i in 0..80 { - operations.push(Operation::Push(Felt::new(i as u64))); - } + let operations: Vec<_> = (0..80).map(|i| Operation::Push(Felt::new(i))).collect(); let block_id = BasicBlockNodeBuilder::new(operations, Vec::new()) .add_to_forest(&mut forest) .unwrap(); - let serialized = forest.to_bytes(); - let deserialized = MastForest::read_from_bytes(&serialized).unwrap(); + let original = forest[block_id].unwrap_basic_block(); + assert!(original.op_batches().len() > 1, "Should have multiple batches"); - let original_node = forest[block_id].unwrap_basic_block(); - let deserialized_node = deserialized[block_id].unwrap_basic_block(); + let deserialized_forest = MastForest::read_from_bytes(&forest.to_bytes()).unwrap(); + let deserialized = deserialized_forest[block_id].unwrap_basic_block(); - // Verify multiple batches exist - assert!( - original_node.op_batches().len() > 1, - "Should have multiple batches for 80 operations" - ); - - // Verify OpBatch structure is preserved - assert_eq!( - original_node.op_batches().len(), - deserialized_node.op_batches().len(), - "Number of batches should match" - ); - - for (orig, deser) in original_node.op_batches().iter().zip(deserialized_node.op_batches()) { - assert_eq!(orig, deser, "Each OpBatch should be identical"); - } + assert_eq!(original.op_batches(), deserialized.op_batches()); } /// Tests that decorator indices remain correct after round-trip with padded operations. @@ -1043,74 +927,38 @@ mod proptests { "Node {:?} digest mismatch", node_id ); - // For basic blocks, verify OpBatch structure is preserved - if let MastNode::Block(original_block) = original && let MastNode::Block(deserialized_block) = deserialized_node { - let orig_batches = original_block.op_batches(); - let deser_batches = deserialized_block.op_batches(); - - prop_assert_eq!( - orig_batches.len(), - deser_batches.len(), - "Node {:?}: Batch count mismatch", node_id - ); - - for (batch_idx, (orig_batch, deser_batch)) in - orig_batches.iter().zip(deser_batches).enumerate() - { - prop_assert_eq!( - orig_batch.ops(), - deser_batch.ops(), - "Node {:?} Batch {}: Operations mismatch", node_id, batch_idx - ); - prop_assert_eq!( - orig_batch.indptr(), - deser_batch.indptr(), - "Node {:?} Batch {}: Indptr mismatch", node_id, batch_idx - ); - prop_assert_eq!( - orig_batch.padding(), - deser_batch.padding(), - "Node {:?} Batch {}: Padding mismatch", node_id, batch_idx - ); - prop_assert_eq!( - orig_batch.groups(), - deser_batch.groups(), - "Node {:?} Batch {}: Groups mismatch", node_id, batch_idx - ); - prop_assert_eq!( - orig_batch.num_groups(), - deser_batch.num_groups(), - "Node {:?} Batch {}: num_groups mismatch", node_id, batch_idx - ); - } - - // Verify decorators are preserved - let orig_decorators: Vec<_> = - original_block.indexed_decorator_iter(&forest).collect(); - let deser_decorators: Vec<_> = - deserialized_block.indexed_decorator_iter(&deserialized).collect(); - + // For basic blocks, verify OpBatch structure and decorators are preserved + if let MastNode::Block(original_block) = original + && let MastNode::Block(deserialized_block) = deserialized_node + { + prop_assert_eq!( + original_block.op_batches(), + deserialized_block.op_batches(), + "Node {:?}: OpBatch mismatch", node_id + ); + + let orig_decorators: Vec<_> = + original_block.indexed_decorator_iter(&forest).collect(); + let deser_decorators: Vec<_> = + deserialized_block.indexed_decorator_iter(&deserialized).collect(); + + prop_assert_eq!( + orig_decorators.len(), + deser_decorators.len(), + "Node {:?}: Decorator count mismatch", node_id + ); + + for ((orig_idx, orig_dec_id), (deser_idx, deser_dec_id)) in + orig_decorators.iter().zip(&deser_decorators) + { + prop_assert_eq!(orig_idx, deser_idx, "Node {:?}: Decorator index mismatch", node_id); prop_assert_eq!( - orig_decorators.len(), - deser_decorators.len(), - "Node {:?}: Decorator count mismatch", node_id + forest.decorator_by_id(*orig_dec_id), + deserialized.decorator_by_id(*deser_dec_id), + "Node {:?}: Decorator content mismatch", node_id ); - - for ((orig_idx, orig_dec_id), (deser_idx, deser_dec_id)) in - orig_decorators.iter().zip(&deser_decorators) - { - prop_assert_eq!( - orig_idx, - deser_idx, - "Node {:?}: Decorator index mismatch", node_id - ); - prop_assert_eq!( - forest.decorator_by_id(*orig_dec_id), - deserialized.decorator_by_id(*deser_dec_id), - "Node {:?}: Decorator content mismatch", node_id - ); - } } + } } } From b383cbfb40613b0f9b60eeb659c4776c5de3479c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= Date: Fri, 12 Dec 2025 15:37:55 -0500 Subject: [PATCH 7/7] test(core): add stdlib integration test for multi-batch serialization Add comprehensive serialization round-trip test using the standard library to verify multi-batch basic block serialization. --- crates/lib/core/tests/mast_forest_merge.rs | 74 +++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/crates/lib/core/tests/mast_forest_merge.rs b/crates/lib/core/tests/mast_forest_merge.rs index 642cc0e839..005364a53a 100644 --- a/crates/lib/core/tests/mast_forest_merge.rs +++ b/crates/lib/core/tests/mast_forest_merge.rs @@ -1,4 +1,7 @@ -use miden_processor::MastForest; +use miden_processor::{ + MastForest, MastNode, MastNodeExt, + utils::{Deserializable, Serializable}, +}; /// Tests that the core library merged with itself produces a forest that has the same procedure /// roots. @@ -17,3 +20,72 @@ fn mast_forest_merge_core_lib() { assert!(merged_digests.contains(&digest)); } } + +/// Tests that the core library with its multi-batch basic blocks round-trips through serialization. +/// +/// The standard library contains many procedures with >72 operations, ensuring multi-batch +/// serialization is tested comprehensively. +#[test] +fn test_core_lib_serialization_roundtrip() { + let std_lib = miden_core_lib::CoreLibrary::default(); + let original_forest = std_lib.mast_forest().as_ref(); + + // Count multi-batch blocks in the stdlib + let multi_batch_count = original_forest + .nodes() + .iter() + .filter_map(|node| { + if let MastNode::Block(block) = node { + if block.op_batches().len() > 1 { Some(()) } else { None } + } else { + None + } + }) + .count(); + + assert!( + multi_batch_count > 0, + "Standard library should contain basic blocks with multiple batches" + ); + + // Round-trip through serialization + let serialized = original_forest.to_bytes(); + let deserialized_forest = + MastForest::read_from_bytes(&serialized).expect("Failed to deserialize standard library"); + + // Verify forest structure is preserved + assert_eq!( + original_forest.num_nodes(), + deserialized_forest.num_nodes(), + "Node count mismatch after serialization" + ); + + assert_eq!( + original_forest.num_procedures(), + deserialized_forest.num_procedures(), + "Procedure count mismatch after serialization" + ); + + // Verify all procedure roots match + let original_roots: Vec<_> = original_forest.procedure_roots().iter().collect(); + let deserialized_roots: Vec<_> = deserialized_forest.procedure_roots().iter().collect(); + assert_eq!(original_roots, deserialized_roots, "Procedure roots mismatch"); + + // Verify all nodes have matching digests + for (idx, (orig_node, deser_node)) in + original_forest.nodes().iter().zip(deserialized_forest.nodes()).enumerate() + { + assert_eq!(orig_node.digest(), deser_node.digest(), "Node {} digest mismatch", idx); + + // For basic blocks, verify OpBatch structure is preserved + if let (MastNode::Block(orig_block), MastNode::Block(deser_block)) = (orig_node, deser_node) + { + assert_eq!( + orig_block.op_batches(), + deser_block.op_batches(), + "Node {} OpBatch structure mismatch", + idx + ); + } + } +}