From d58fbff92a2db01652fdb765456850a4bddaa913 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 10 Apr 2025 13:26:19 +0000 Subject: [PATCH 1/4] Update ACVM doc --- acvm-repo/acvm/src/compiler/mod.rs | 4 ++- .../compiler/optimizers/merge_expressions.rs | 24 +++++++++++-- .../src/compiler/optimizers/unused_memory.rs | 3 ++ .../acvm/src/compiler/transformers/csat.rs | 8 +++-- .../acvm/src/compiler/transformers/mod.rs | 21 +++++++++--- acvm-repo/acvm/src/pwg/arithmetic.rs | 34 ++++++++++++++++--- acvm-repo/acvm/src/pwg/blackbox/mod.rs | 13 +++++++ acvm-repo/acvm/src/pwg/brillig.rs | 17 +++++++--- acvm-repo/acvm/src/pwg/memory_op.rs | 29 +++++++++++++++- acvm-repo/acvm/src/pwg/mod.rs | 24 +++++++++++-- acvm-repo/brillig_vm/src/lib.rs | 24 ++++++++++++- 11 files changed, 177 insertions(+), 24 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 301f6088826..1e4e571ea03 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -26,7 +26,7 @@ pub struct AcirTransformationMap { impl AcirTransformationMap { /// Builds a map from a vector of pointers to the old acir opcodes. - /// The index of the vector is the new opcode index. + /// The index in the vector is the new opcode index. /// The value of the vector is the old opcode index pointed. fn new(acir_opcode_positions: &[usize]) -> Self { let mut old_indices_to_new_indices = HashMap::with_capacity(acir_opcode_positions.len()); @@ -36,6 +36,7 @@ impl AcirTransformationMap { AcirTransformationMap { old_indices_to_new_indices } } + /// Returns the new opcode location(s) corresponding to the old opcode. pub fn new_locations( &self, old_location: OpcodeLocation, @@ -58,6 +59,7 @@ impl AcirTransformationMap { } } +/// Update the assert messages to point to the new opcode locations. fn transform_assert_messages( assert_messages: Vec<(OpcodeLocation, AssertionPayload)>, map: &AcirTransformationMap, diff --git a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index 0e0d22edcd8..4ececb3114d 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -27,9 +27,25 @@ impl MergeExpressionsOptimizer { } } /// This pass analyzes the circuit and identifies intermediate variables that are - /// only used in two gates. It then merges the gate that produces the + /// only used in two arithmetic opcodes. It then merges the opcode which produces the /// intermediate variable into the second one that uses it /// Note: This pass is only relevant for backends that can handle unlimited width + /// + /// The first pass maps witness with the opcodes using it. + /// Public inputs are not considered because they cannot be simplified. + /// Witnesses used by MemoryInit opcodes are put in a separate map and marked as used by a Brillig call + /// if the memory block is an input to the call. + /// + /// The second pass looks for arithmetic opcodes having a witness which is only used by another arithmetic opcode. + /// In that case, the opcode with the smallest index is merged into the other one via Gaussian elimination. + /// For instance, if we have '_1' used only by these two opcodes: + /// [(1, _2,_3), (2, _2), (2, _1), (1, _3)] + /// [(2, _3, _4), (2,_1), (1, _4)] + /// We will remove the first one and modify the second one like this: + /// [(2, _3, _4), (1, _4), (-1, _2), (-1/2, _3), (-1/2, _2, _3)] + /// + /// This transformation is relevant for Plonk-ish backends although they have a limited width because + /// they can potentially handle expressions with large linear combinations using 'big-add' gates. pub(crate) fn eliminate_intermediate_variable( &mut self, circuit: &Circuit, @@ -183,7 +199,7 @@ impl MergeExpressionsOptimizer { witnesses } Opcode::MemoryOp { block_id: _, op, predicate } => { - //index et value, et predicate + //index, value, and predicate let mut witnesses = CircuitSimulator::expr_wit(&op.index); witnesses.extend(CircuitSimulator::expr_wit(&op.value)); if let Some(p) = predicate { @@ -248,6 +264,10 @@ impl MergeExpressionsOptimizer { None } + /// Returns the 'updated' opcode at index 'g' in the circuit + /// The modifications to the circuits are stored with 'deleted_gates' and 'modified_gates' + /// These structures are used to give the 'updated' opcode. + /// For instance, if the opcode has been deleted inside 'deleted_gates', then it returns None. fn get_opcode(&self, g: usize, circuit: &Circuit) -> Option> { if self.deleted_gates.contains(&g) { return None; diff --git a/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs b/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs index 3a256aafe63..7155931720b 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs @@ -5,6 +5,9 @@ use acir::{ use std::collections::HashSet; /// `UnusedMemoryOptimizer` will remove initializations of memory blocks which are unused. +/// A first pass collect all memory blocks which are initialized but discard the ones +/// which are used in a MemoryOp or as input to a BrilligCall. +/// The second pass removes the opcodes tagged as unused by the first pass. pub(crate) struct UnusedMemoryOptimizer { unused_memory_initializations: HashSet, circuit: Circuit, diff --git a/acvm-repo/acvm/src/compiler/transformers/csat.rs b/acvm-repo/acvm/src/compiler/transformers/csat.rs index 222cad74e22..f79c4365323 100644 --- a/acvm-repo/acvm/src/compiler/transformers/csat.rs +++ b/acvm-repo/acvm/src/compiler/transformers/csat.rs @@ -66,9 +66,11 @@ impl CSatTransformer { self.solvable_witness.insert(witness); } - // Still missing dead witness optimization. - // To do this, we will need the whole set of assert-zero opcodes - // I think it can also be done before the local optimization seen here, as dead variables will come from the user + /// Transform the input arithmetic expression into a new one having the correct 'width' + /// by creating intermediate variables as needed. + /// Having the correct width means: + /// - it has at most one multiplicative term + /// - it has at most 'width-1' linear combinations terms. pub(crate) fn transform( &mut self, opcode: Expression, diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index 72d070d80a8..7dd85bdac99 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -91,7 +91,16 @@ pub(super) fn transform_internal( /// /// Accepts an injected `acir_opcode_positions` to allow transformations to be applied directly after optimizations. /// -/// Does a single optimization pass. +/// If the width is unbounded, it does nothing. +/// If it is bounded, it first performs the 'CSAT transformation' in one pass, by creating intermediate variables when necessary. +/// This results in arithmetic opcodes having the required width. +/// Then the 'eliminate intermediate variables' pass will remove any intermediate variables (for instance created by the previous transformation) +/// that are used in exactly two arithmetic opcodes. +/// This results in arithmetic opcodes having linear combinations of potentially large width. +/// Finally, the 'range optimization' pass will remove any redundant range opcodes. +/// It is expected that the backend will be able to handle linear combinations of 'unbounded width' in a more efficient way +/// than the 'CSAT transformation'. +/// If it is not the case, the 'eliminate intermediate variables' pass should not be applied. #[tracing::instrument( level = "trace", name = "transform_acir_once", @@ -102,6 +111,7 @@ fn transform_internal_once( expression_width: ExpressionWidth, acir_opcode_positions: Vec, ) -> (Circuit, Vec) { + // If the expression width is unbounded, we don't need to do anything. let mut transformer = match &expression_width { ExpressionWidth::Unbounded => { return (acir, acir_opcode_positions); @@ -115,9 +125,10 @@ fn transform_internal_once( } }; - // TODO: the code below is only for CSAT transformer - // TODO it may be possible to refactor it in a way that we do not need to return early from the r1cs - // TODO or at the very least, we could put all of it inside of CSatOptimizer pass + // 1. CSAT transformation + // Process each opcode in the circuit by marking the solvable witnesses and transforming the AssertZero opcodes + // to the required width by creating intermediate variables. + // Knowing if a witness is solvable avoids creating un-solvable intermediate variables. let mut new_acir_opcode_positions: Vec = Vec::with_capacity(acir_opcode_positions.len()); // Optimize the assert-zero gates by reducing them into the correct width and @@ -216,6 +227,7 @@ fn transform_internal_once( ..acir }; + // 2. Eliminate intermediate variables, when they are used in exactly two arithmetic opcodes. let mut merge_optimizer = MergeExpressionsOptimizer::new(); let (opcodes, new_acir_opcode_positions) = @@ -228,6 +240,7 @@ fn transform_internal_once( ..acir }; + // 3. Remove redundant range constraints. // The `MergeOptimizer` can merge two witnesses which have range opcodes applied to them // so we run the `RangeOptimizer` afterwards to clear these up. let range_optimizer = RangeOptimizer::new(acir); diff --git a/acvm-repo/acvm/src/pwg/arithmetic.rs b/acvm-repo/acvm/src/pwg/arithmetic.rs index a2921bcbc9b..636e0658f96 100644 --- a/acvm-repo/acvm/src/pwg/arithmetic.rs +++ b/acvm-repo/acvm/src/pwg/arithmetic.rs @@ -23,7 +23,17 @@ pub(crate) enum MulTerm { } impl ExpressionSolver { - /// Derives the rest of the witness based on the initial low level variables + /// Derives the rest of the witness in the provided expression based on the known witness values + /// 1. Fist we simplify the expression based on the known values and try to reduce the multiplication and linear terms + /// 2. If we end up with only the constant term; + /// - if it is 0 then the opcode is solved, if not, + /// - the assert_zero opcode is not satisfied and we return an error + /// 3. If we end up with only linear terms on the same witness 'w', + /// we can regroup them and solve 'a*w+c = 0': + /// If 'a' is zero in the above expression; + /// - if c is also 0 then the opcode is solved + /// - if not that means the assert_zero opcode is not satisfied and we return an error + /// If 'a' is not zero, we can solve it by setting the value of w: 'w = -c/a' pub(crate) fn solve( initial_witness: &mut WitnessMap, opcode: &Expression, @@ -129,10 +139,11 @@ impl ExpressionSolver { } } - /// Returns the evaluation of the multiplication term in the expression - /// If the witness values are not known, then the function returns a None - /// XXX: Do we need to account for the case where 5xy + 6x = 0 ? We do not know y, but it can be solved given x . But I believe x can be solved with another opcode - /// XXX: What about making a mul opcode = a constant 5xy + 7 = 0 ? This is the same as the above. + /// Try to reduce the multiplication terms of the given expression to a known value or to a linear term, + /// using the provided witness mapping. + /// If there are 2 or more multiplication terms it returns the OpcodeUnsolvable error. + /// If no witnesses value is in the provided 'witness_assignments' map, + /// it returns MulTerm::TooManyUnknowns fn solve_mul_term( arith_opcode: &Expression, witness_assignments: &WitnessMap, @@ -149,6 +160,11 @@ impl ExpressionSolver { } } + /// Try to solve a multiplication term of the form q*a*b, where + /// q is a constant and a,b are witnesses + /// If both a and b have known values (in the provided map), it returns the value q*a*b + /// If only one of a or b has a known value, it returns the linear term c*w where c is a constant and w is the unknown witness + /// If both a and b are unknown, it returns MulTerm::TooManyUnknowns fn solve_mul_term_helper( term: &(F, Witness, Witness), witness_assignments: &WitnessMap, @@ -166,6 +182,8 @@ impl ExpressionSolver { } } + /// Reduce a linear term to its value if the witness assignment is known + /// If the witness value is not known in the provided map, it returns None. fn solve_fan_in_term_helper( term: &(F, Witness), witness_assignments: &WitnessMap, @@ -215,6 +233,12 @@ impl ExpressionSolver { } // Partially evaluate the opcode using the known witnesses + // For instance if values of witness 'a' and 'b' are known, then + // the multiplication 'a*b' is removed and their multiplied values are added to the constant term + // If only witness 'a' is known, then the multiplication 'a*b' is replaced by the linear term '(value of b)*a' + // etc ... + // If all values are known, the partial evaluation gives a constant expression + // If no value is known, the partial evaluation returns the original expression pub(crate) fn evaluate( expr: &Expression, initial_witness: &WitnessMap, diff --git a/acvm-repo/acvm/src/pwg/blackbox/mod.rs b/acvm-repo/acvm/src/pwg/blackbox/mod.rs index ad064fefd7b..51d07be252c 100644 --- a/acvm-repo/acvm/src/pwg/blackbox/mod.rs +++ b/acvm-repo/acvm/src/pwg/blackbox/mod.rs @@ -53,6 +53,19 @@ fn contains_all_inputs( first_missing_assignment(witness_assignments, inputs).is_none() } +/// Solve a black box function call +/// 1. Returns an error if not all the inputs are already resolved to a value +/// 2. Compute the output from the inputs, using the dedicated solvers +/// +/// A blackbox is a fully specified function (e.g sha256, ecdsa signature,...) +/// which the backend can prove execution in a more efficient way than using a generic +/// arithmetic circuit. +/// Solving a black box function simply means to compute the output from the inputs for +/// the specific function. +/// Our black box solver uses the standard rust implementation for the function if it is available. +/// However, some function depends on the backend, such as embedded curve operations, which depend on the +/// elliptic curves used by the proving system. This is why the 'solve' functions takes a blackbox solver trait. +/// The 'AcvmBigIntSolver' is also a blackbox solver, but dedicated to the BigInteger blackbox functions. pub(crate) fn solve( backend: &impl BlackBoxFunctionSolver, initial_witness: &mut WitnessMap, diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index 889c7512b6a..9abb5b306b0 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -30,6 +30,8 @@ pub enum BrilligSolverStatus { ForeignCallWait(ForeignCallWaitInfo), } +/// Specific solver for Brillig opcodes +/// It maintains a Brillig VM that can execute the bytecode of the called brillig function pub struct BrilligSolver<'b, F, B: BlackBoxFunctionSolver> { vm: VM<'b, F, B>, acir_index: usize, @@ -86,6 +88,9 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { Ok(Self { vm, acir_index, function_id: brillig_function_id }) } + /// Get a BrilligVM for executing the provided bytecode + /// 1. Reduce the input expressions into a known value, or error if they do not reduce to a value. + /// 2. Instantiate the Brillig VM with the bytecode and the reduced inputs. fn setup_brillig_vm( initial_witness: &WitnessMap, memory: &HashMap>, @@ -181,14 +186,14 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { self.vm.program_counter() } + /// Returns the status of the Brillig VM as a 'BrilligSolverStatus' resolution. + /// It may be finished, in-progress, failed, or may be waiting for results of a foreign call. + /// Return the "resolution" to the caller who may choose to make subsequent calls + /// (when it gets foreign call results for example). fn handle_vm_status( &self, vm_status: VMStatus, ) -> Result, OpcodeResolutionError> { - // Check the status of the Brillig VM and return a resolution. - // It may be finished, in-progress, failed, or may be waiting for results of a foreign call. - // Return the "resolution" to the caller who may choose to make subsequent calls - // (when it gets foreign call results for example). match vm_status { VMStatus::Finished { .. } => Ok(BrilligSolverStatus::Finished), VMStatus::InProgress => Ok(BrilligSolverStatus::InProgress), @@ -234,6 +239,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { self.finalize_inner(witness, outputs) } + /// Finalize the VM and return the profiling samples. pub(crate) fn finalize_with_profiling( mut self, witness: &mut WitnessMap, @@ -244,6 +250,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { Ok(self.vm.take_profiling_samples()) } + /// Finalize the VM execution and write the outputs to the provided witness map. fn finalize_inner( &self, witness: &mut WitnessMap, @@ -260,6 +267,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { } } + /// Write VM execution results into the witness map fn write_brillig_outputs( &self, witness_map: &mut WitnessMap, @@ -267,7 +275,6 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { return_data_size: usize, outputs: &[BrilligOutputs], ) -> Result<(), OpcodeResolutionError> { - // Write VM execution results into the witness map let memory = self.vm.get_memory(); let mut current_ret_data_idx = return_data_offset; for output in outputs.iter() { diff --git a/acvm-repo/acvm/src/pwg/memory_op.rs b/acvm-repo/acvm/src/pwg/memory_op.rs index aef18d3957e..c215f863562 100644 --- a/acvm-repo/acvm/src/pwg/memory_op.rs +++ b/acvm-repo/acvm/src/pwg/memory_op.rs @@ -16,11 +16,16 @@ type MemoryIndex = u32; /// Maintains the state for solving [`MemoryInit`][`acir::circuit::Opcode::MemoryInit`] and [`MemoryOp`][`acir::circuit::Opcode::MemoryOp`] opcodes. #[derive(Default)] pub(crate) struct MemoryOpSolver { + /// Known values of the memory block, based on the index + /// This map evolves as we process the opcodes pub(super) block_value: HashMap, + /// Length of the block, i.e the number of elements stored into the memory block. pub(super) block_len: u32, } impl MemoryOpSolver { + /// Convert a field element into a memory index + /// Only 32 bits values are valid memory index fn index_from_field(&self, index: F) -> Result> { if index.num_bits() <= 32 { let memory_index = index.try_to_u64().unwrap() as MemoryIndex; @@ -34,6 +39,8 @@ impl MemoryOpSolver { } } + /// Update the 'block_value' map with the provided index/value + /// Returns an 'IndexOutOfBounds' error if the index is outside the block range. fn write_memory_index( &mut self, index: MemoryIndex, @@ -50,6 +57,8 @@ impl MemoryOpSolver { Ok(()) } + /// Returns the value stored in the 'block_value' map for the provided index + /// Returns an 'IndexOutOfBounds' error if the index is not in the map. fn read_memory_index(&self, index: MemoryIndex) -> Result> { self.block_value.get(&index).copied().ok_or(OpcodeResolutionError::IndexOutOfBounds { opcode_location: ErrorLocation::Unresolved, @@ -74,11 +83,29 @@ impl MemoryOpSolver { Ok(()) } + /// Update the 'block_values' by processing the provided Memory opcode + /// The opcode 'op' contains the index and value of the operation and the type + /// of the operation. + /// They are all stored as 'Expressions' + /// The type of 'operation' is '0' for a read and '1' for a write. It must be a constant + /// expression. + /// Index is not required to be constant but it must reduce to a known value + /// for processing the opcode. This is done by doing the (partial) evaluation of its expression, + /// using the provided witness map. + /// + /// READ: read the block at index op.index and update op.value with the read value + /// - 'op.value' must reduce to a witness (after the evaluation of its expression) + /// - the value is updated in the provided witness map, for the 'op.value' witness + /// + /// WRITE: update the block at index 'op.index' with 'op.value' + /// - 'op.value' must reduce to a known value + /// + /// If a requirement is not met, it returns an error. pub(crate) fn solve_memory_op( &mut self, op: &MemOp, initial_witness: &mut WitnessMap, - predicate: &Option>, + predicate: &Option>, //TODO il faut enlver le predicat !? pedantic_solving: bool, ) -> Result<(), OpcodeResolutionError> { let operation = get_value(&op.operation, initial_witness)?; diff --git a/acvm-repo/acvm/src/pwg/mod.rs b/acvm-repo/acvm/src/pwg/mod.rs index 7d462ad6244..ad16db6d3dd 100644 --- a/acvm-repo/acvm/src/pwg/mod.rs +++ b/acvm-repo/acvm/src/pwg/mod.rs @@ -214,6 +214,8 @@ pub struct ACVM<'a, F: AcirField, B: BlackBoxFunctionSolver> { /// Index of the next opcode to be executed. instruction_pointer: usize, + /// A mapping of witnesses to their solved values + /// The map is updated as the ACVM executes. witness_map: WitnessMap, brillig_solver: Option>, @@ -408,6 +410,12 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { self.status.clone() } + /// Executes a single opcode using the dedicated solver. + /// + /// Foreign or Acir Calls are deferred to the caller, which will + /// either instantiate a new ACVM to execute the called ACIR function + /// or a custom implementation to execute the foreign call. + /// Then it will resume execution of the current ACVM with the results of the call. pub fn solve_opcode(&mut self) -> ACVMStatus { let opcode = &self.opcodes[self.instruction_pointer]; let resolution = match opcode { @@ -443,6 +451,8 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { self.handle_opcode_resolution(resolution) } + /// Returns the status of the ACVM + /// If the status is in error, it converts the error into 'OpcodeResolutionError' fn handle_opcode_resolution( &mut self, resolution: Result<(), OpcodeResolutionError>, @@ -524,6 +534,9 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { })) } + /// Solves a Brillig Call opcode, which represents a call to an unconstrained function. + /// It first handle the predicate and returns zero values if the predicate is false. + /// Then it executes (or resumes execution) the Brillig function using a Brillig VM. fn solve_brillig_call_opcode( &mut self, ) -> Result>, OpcodeResolutionError> { @@ -610,6 +623,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { } } + // This function is used by the debugger pub fn step_into_brillig(&mut self) -> StepResult<'a, F, B> { let Opcode::BrilligCall { id, inputs, outputs, predicate } = &self.opcodes[self.instruction_pointer] @@ -651,6 +665,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { } } + // This function is used by the debugger pub fn finish_brillig_with_solver(&mut self, solver: BrilligSolver<'a, F, B>) -> ACVMStatus { if !matches!(self.opcodes[self.instruction_pointer], Opcode::BrilligCall { .. }) { unreachable!("Not executing a Brillig/BrilligCall opcode"); @@ -659,6 +674,11 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { self.solve_opcode() } + /// Defer execution of the ACIR call opcode to the caller, or finalize the execution. + /// 1. It first handle the predicate and return zero values if the predicate is false. + /// 2. If the results of the execution are not available, it issues a 'AcirCallWaitInfo' + /// to notify the caller that it (the caller) needs to execute the ACIR function. + /// 3. If the results are available, it updates the witness map and indicates that the opcode is solved. pub fn solve_call_opcode( &mut self, ) -> Result>, OpcodeResolutionError> { @@ -756,8 +776,8 @@ pub fn input_to_value( } } -// TODO: There is an issue open to decide on whether we need to get values from Expressions -// TODO versus just getting values from Witness +/// Returns the concrete value for a particular expression +/// If the value cannot be computed, it returns an 'OpcodeNotSolvable' error. pub fn get_value( expr: &Expression, initial_witness: &WitnessMap, diff --git a/acvm-repo/brillig_vm/src/lib.rs b/acvm-repo/brillig_vm/src/lib.rs index afbd31416cb..ebb778d0209 100644 --- a/acvm-repo/brillig_vm/src/lib.rs +++ b/acvm-repo/brillig_vm/src/lib.rs @@ -122,7 +122,7 @@ pub struct VM<'a, F, B: BlackBoxFunctionSolver> { call_stack: Vec, /// The solver for blackbox functions black_box_solver: &'a B, - // The solver for big integers + // The solver for big integers, it is used by the blackbox solver bigint_solver: BrilligBigIntSolver, // Flag that determines whether we want to profile VM. profiling_active: bool, @@ -213,6 +213,8 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { self.status(VMStatus::ForeignCallWait { function, inputs }) } + /// Provide the results of a Foreign Call to the VM + /// and resume execution of the VM. pub fn resolve_foreign_call(&mut self, foreign_call_result: ForeignCallResult) { if self.foreign_call_counter < self.foreign_call_results.len() { panic!("No unresolved foreign calls"); @@ -402,6 +404,18 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { pub fn get_fuzzing_trace(&self) -> Vec { self.fuzzer_trace.clone() } + + /// Execute a single opcode: + /// 1. Retrieve the current opcode using the program counter + /// 2. Execute the opcode. + /// For instance a binary 'result = lhs+rhs' opcode will read the VM memory at the lhs and rhs addresses, + /// compute the sum and write it to the 'result' memory address. + /// 3. Update the program counter, usually by incrementing it. + /// + /// - Control flow opcodes will will jump around the bytecode by setting the program counter. + /// - Foreign call opcode pause the VM until the foreign call results are available + /// - Function call opcode backup the current program counter into the call stack and jump to the function entry point. + /// stack frame for function calls are handled during codegen. fn process_opcode_internal(&mut self) -> VMStatus { let opcode = &self.bytecode[self.program_counter]; match opcode { @@ -709,6 +723,14 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { } } + /// Write a foreign call results to the VM memory. + /// + /// We match the expected types with the actual results. + /// However foreign call results does not support nested structures: + /// They are either a single integer value or a vector of integer values (field elements). + /// Therefore, nested arrays returned from foreign call results are flattened. + /// If the expected array sizes do not match the actual size, we reconstruct the nested + /// structure from the flatten output array. fn write_foreign_call_result( &mut self, destinations: &[ValueOrArray], From 9e846323c8047c32fec63a12bee29ea5ed6a46c0 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 15 Apr 2025 13:25:22 +0000 Subject: [PATCH 2/4] Code review --- .../acvm/src/compiler/optimizers/merge_expressions.rs | 5 +++-- acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs | 2 +- acvm-repo/acvm/src/compiler/transformers/csat.rs | 2 +- acvm-repo/acvm/src/pwg/blackbox/mod.rs | 4 ++-- acvm-repo/acvm/src/pwg/memory_op.rs | 6 +++--- acvm-repo/acvm/src/pwg/mod.rs | 8 ++++---- 6 files changed, 14 insertions(+), 13 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index 4ececb3114d..92082b3b73d 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -31,14 +31,15 @@ impl MergeExpressionsOptimizer { /// intermediate variable into the second one that uses it /// Note: This pass is only relevant for backends that can handle unlimited width /// - /// The first pass maps witness with the opcodes using it. + /// The first pass maps witnesses to the index of the opcodes using them. /// Public inputs are not considered because they cannot be simplified. /// Witnesses used by MemoryInit opcodes are put in a separate map and marked as used by a Brillig call /// if the memory block is an input to the call. /// /// The second pass looks for arithmetic opcodes having a witness which is only used by another arithmetic opcode. /// In that case, the opcode with the smallest index is merged into the other one via Gaussian elimination. - /// For instance, if we have '_1' used only by these two opcodes: + /// For instance, if we have '_1' used only by these two opcodes, + /// where `_{value}` refers to a witness and `{value}` refers to a constant: /// [(1, _2,_3), (2, _2), (2, _1), (1, _3)] /// [(2, _3, _4), (2,_1), (1, _4)] /// We will remove the first one and modify the second one like this: diff --git a/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs b/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs index 7155931720b..02f9699328d 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs @@ -5,7 +5,7 @@ use acir::{ use std::collections::HashSet; /// `UnusedMemoryOptimizer` will remove initializations of memory blocks which are unused. -/// A first pass collect all memory blocks which are initialized but discard the ones +/// A first pass collects all memory blocks which are initialized but discards the ones /// which are used in a MemoryOp or as input to a BrilligCall. /// The second pass removes the opcodes tagged as unused by the first pass. pub(crate) struct UnusedMemoryOptimizer { diff --git a/acvm-repo/acvm/src/compiler/transformers/csat.rs b/acvm-repo/acvm/src/compiler/transformers/csat.rs index f79c4365323..008e93f32a4 100644 --- a/acvm-repo/acvm/src/compiler/transformers/csat.rs +++ b/acvm-repo/acvm/src/compiler/transformers/csat.rs @@ -70,7 +70,7 @@ impl CSatTransformer { /// by creating intermediate variables as needed. /// Having the correct width means: /// - it has at most one multiplicative term - /// - it has at most 'width-1' linear combinations terms. + /// - it uses at most 'width-1' witness, to account for the new intermediate variable pub(crate) fn transform( &mut self, opcode: Expression, diff --git a/acvm-repo/acvm/src/pwg/blackbox/mod.rs b/acvm-repo/acvm/src/pwg/blackbox/mod.rs index 51d07be252c..c384e848e3a 100644 --- a/acvm-repo/acvm/src/pwg/blackbox/mod.rs +++ b/acvm-repo/acvm/src/pwg/blackbox/mod.rs @@ -63,8 +63,8 @@ fn contains_all_inputs( /// Solving a black box function simply means to compute the output from the inputs for /// the specific function. /// Our black box solver uses the standard rust implementation for the function if it is available. -/// However, some function depends on the backend, such as embedded curve operations, which depend on the -/// elliptic curves used by the proving system. This is why the 'solve' functions takes a blackbox solver trait. +/// However, some functions depends on the backend, such as embedded curve operations, which depend on the +/// elliptic curve used by the proving system. This is why the 'solve' functions takes a blackbox solver trait. /// The 'AcvmBigIntSolver' is also a blackbox solver, but dedicated to the BigInteger blackbox functions. pub(crate) fn solve( backend: &impl BlackBoxFunctionSolver, diff --git a/acvm-repo/acvm/src/pwg/memory_op.rs b/acvm-repo/acvm/src/pwg/memory_op.rs index c215f863562..34747371afa 100644 --- a/acvm-repo/acvm/src/pwg/memory_op.rs +++ b/acvm-repo/acvm/src/pwg/memory_op.rs @@ -25,7 +25,7 @@ pub(crate) struct MemoryOpSolver { impl MemoryOpSolver { /// Convert a field element into a memory index - /// Only 32 bits values are valid memory index + /// Only 32 bits values are valid memory indices fn index_from_field(&self, index: F) -> Result> { if index.num_bits() <= 32 { let memory_index = index.try_to_u64().unwrap() as MemoryIndex; @@ -86,7 +86,7 @@ impl MemoryOpSolver { /// Update the 'block_values' by processing the provided Memory opcode /// The opcode 'op' contains the index and value of the operation and the type /// of the operation. - /// They are all stored as 'Expressions' + /// They are all stored as an [Expression] /// The type of 'operation' is '0' for a read and '1' for a write. It must be a constant /// expression. /// Index is not required to be constant but it must reduce to a known value @@ -105,7 +105,7 @@ impl MemoryOpSolver { &mut self, op: &MemOp, initial_witness: &mut WitnessMap, - predicate: &Option>, //TODO il faut enlver le predicat !? + predicate: &Option>, pedantic_solving: bool, ) -> Result<(), OpcodeResolutionError> { let operation = get_value(&op.operation, initial_witness)?; diff --git a/acvm-repo/acvm/src/pwg/mod.rs b/acvm-repo/acvm/src/pwg/mod.rs index ad16db6d3dd..f9a4ea8d447 100644 --- a/acvm-repo/acvm/src/pwg/mod.rs +++ b/acvm-repo/acvm/src/pwg/mod.rs @@ -412,7 +412,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { /// Executes a single opcode using the dedicated solver. /// - /// Foreign or Acir Calls are deferred to the caller, which will + /// Foreign or ACIR Calls are deferred to the caller, which will /// either instantiate a new ACVM to execute the called ACIR function /// or a custom implementation to execute the foreign call. /// Then it will resume execution of the current ACVM with the results of the call. @@ -452,7 +452,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { } /// Returns the status of the ACVM - /// If the status is in error, it converts the error into 'OpcodeResolutionError' + /// If the status is an error, it converts the error into [OpcodeResolutionError] fn handle_opcode_resolution( &mut self, resolution: Result<(), OpcodeResolutionError>, @@ -535,7 +535,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { } /// Solves a Brillig Call opcode, which represents a call to an unconstrained function. - /// It first handle the predicate and returns zero values if the predicate is false. + /// It first handles the predicate and returns zero values if the predicate is false. /// Then it executes (or resumes execution) the Brillig function using a Brillig VM. fn solve_brillig_call_opcode( &mut self, @@ -675,7 +675,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> ACVM<'a, F, B> { } /// Defer execution of the ACIR call opcode to the caller, or finalize the execution. - /// 1. It first handle the predicate and return zero values if the predicate is false. + /// 1. It first handles the predicate and return zero values if the predicate is false. /// 2. If the results of the execution are not available, it issues a 'AcirCallWaitInfo' /// to notify the caller that it (the caller) needs to execute the ACIR function. /// 3. If the results are available, it updates the witness map and indicates that the opcode is solved. From 23736518aa0207d1478fc04740966624f570fd63 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 15 Apr 2025 13:38:31 +0000 Subject: [PATCH 3/4] code review --- acvm-repo/brillig_vm/src/lib.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/acvm-repo/brillig_vm/src/lib.rs b/acvm-repo/brillig_vm/src/lib.rs index ebb778d0209..ce1e26422ad 100644 --- a/acvm-repo/brillig_vm/src/lib.rs +++ b/acvm-repo/brillig_vm/src/lib.rs @@ -412,10 +412,10 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { /// compute the sum and write it to the 'result' memory address. /// 3. Update the program counter, usually by incrementing it. /// - /// - Control flow opcodes will will jump around the bytecode by setting the program counter. - /// - Foreign call opcode pause the VM until the foreign call results are available - /// - Function call opcode backup the current program counter into the call stack and jump to the function entry point. - /// stack frame for function calls are handled during codegen. + /// - Control flow opcodes jump around the bytecode by setting the program counter. + /// - Foreign call opcodes pause the VM until the foreign call results are available + /// - Function call opcodes backup the current program counter into the call stack and jump to the function entry point. + /// The stack frame for function calls is handled during codegen. fn process_opcode_internal(&mut self) -> VMStatus { let opcode = &self.bytecode[self.program_counter]; match opcode { @@ -723,14 +723,14 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { } } - /// Write a foreign call results to the VM memory. + /// Write a foreign call's results to the VM memory. /// /// We match the expected types with the actual results. - /// However foreign call results does not support nested structures: + /// However foreign call results do not support nested structures: /// They are either a single integer value or a vector of integer values (field elements). /// Therefore, nested arrays returned from foreign call results are flattened. /// If the expected array sizes do not match the actual size, we reconstruct the nested - /// structure from the flatten output array. + /// structure from the flat output array. fn write_foreign_call_result( &mut self, destinations: &[ValueOrArray], From 1ddeadf2dd4481f381f183649cd61f1fb107985c Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 18 Apr 2025 13:48:58 +0000 Subject: [PATCH 4/4] Code review --- acvm-repo/acvm/src/compiler/transformers/csat.rs | 2 +- acvm-repo/acvm/src/compiler/transformers/mod.rs | 3 +-- acvm-repo/acvm/src/pwg/blackbox/mod.rs | 2 +- acvm-repo/brillig_vm/src/lib.rs | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/transformers/csat.rs b/acvm-repo/acvm/src/compiler/transformers/csat.rs index 008e93f32a4..8a2bf9876e4 100644 --- a/acvm-repo/acvm/src/compiler/transformers/csat.rs +++ b/acvm-repo/acvm/src/compiler/transformers/csat.rs @@ -70,7 +70,7 @@ impl CSatTransformer { /// by creating intermediate variables as needed. /// Having the correct width means: /// - it has at most one multiplicative term - /// - it uses at most 'width-1' witness, to account for the new intermediate variable + /// - it uses at most 'width-1' witness linear combination terms, to account for the new intermediate variable pub(crate) fn transform( &mut self, opcode: Expression, diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index 7dd85bdac99..cfed17f92de 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -98,9 +98,8 @@ pub(super) fn transform_internal( /// that are used in exactly two arithmetic opcodes. /// This results in arithmetic opcodes having linear combinations of potentially large width. /// Finally, the 'range optimization' pass will remove any redundant range opcodes. -/// It is expected that the backend will be able to handle linear combinations of 'unbounded width' in a more efficient way +/// The backend is expected to handle linear combinations of 'unbounded width' in a more efficient way /// than the 'CSAT transformation'. -/// If it is not the case, the 'eliminate intermediate variables' pass should not be applied. #[tracing::instrument( level = "trace", name = "transform_acir_once", diff --git a/acvm-repo/acvm/src/pwg/blackbox/mod.rs b/acvm-repo/acvm/src/pwg/blackbox/mod.rs index c384e848e3a..663603cf0a2 100644 --- a/acvm-repo/acvm/src/pwg/blackbox/mod.rs +++ b/acvm-repo/acvm/src/pwg/blackbox/mod.rs @@ -63,7 +63,7 @@ fn contains_all_inputs( /// Solving a black box function simply means to compute the output from the inputs for /// the specific function. /// Our black box solver uses the standard rust implementation for the function if it is available. -/// However, some functions depends on the backend, such as embedded curve operations, which depend on the +/// However, some functions depend on the backend, such as embedded curve operations, which depend on the /// elliptic curve used by the proving system. This is why the 'solve' functions takes a blackbox solver trait. /// The 'AcvmBigIntSolver' is also a blackbox solver, but dedicated to the BigInteger blackbox functions. pub(crate) fn solve( diff --git a/acvm-repo/brillig_vm/src/lib.rs b/acvm-repo/brillig_vm/src/lib.rs index ce1e26422ad..29191a03a9a 100644 --- a/acvm-repo/brillig_vm/src/lib.rs +++ b/acvm-repo/brillig_vm/src/lib.rs @@ -415,7 +415,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { /// - Control flow opcodes jump around the bytecode by setting the program counter. /// - Foreign call opcodes pause the VM until the foreign call results are available /// - Function call opcodes backup the current program counter into the call stack and jump to the function entry point. - /// The stack frame for function calls is handled during codegen. + /// The stack frame for function calls is handled during codegen. fn process_opcode_internal(&mut self) -> VMStatus { let opcode = &self.bytecode[self.program_counter]; match opcode {