From 7feb6589d98246400497dfa492f39d67bae85977 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 15:01:10 +0000 Subject: [PATCH 1/2] feat(tooling): Skip program transformation when loaded from cache (#6689) --- Cargo.lock | 1 + acvm-repo/acir/src/circuit/brillig.rs | 6 +-- acvm-repo/acir/src/circuit/mod.rs | 12 +++--- acvm-repo/acir/src/circuit/opcodes.rs | 4 +- .../opcodes/black_box_function_call.rs | 6 +-- .../src/circuit/opcodes/memory_operation.rs | 2 +- acvm-repo/brillig/src/black_box.rs | 2 +- acvm-repo/brillig/src/opcodes.rs | 18 ++++----- compiler/noirc_driver/src/debug.rs | 2 +- compiler/noirc_driver/src/lib.rs | 25 +++++++++--- compiler/noirc_driver/src/program.rs | 2 +- compiler/noirc_errors/src/debug_info.rs | 2 +- compiler/noirc_evaluator/src/errors.rs | 6 +-- compiler/noirc_evaluator/src/ssa.rs | 2 +- tooling/nargo/src/ops/check.rs | 2 +- tooling/nargo/src/ops/transform.rs | 3 +- tooling/nargo_cli/Cargo.toml | 1 + tooling/nargo_cli/src/cli/compile_cmd.rs | 38 ++++++++++++++++++- tooling/nargo_cli/tests/stdlib-props.rs | 1 + tooling/noirc_abi/src/lib.rs | 13 ++++--- 20 files changed, 102 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index af91bafef52..2f19ed704b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2789,6 +2789,7 @@ dependencies = [ "dirs", "file-lock", "fm", + "fxhash", "iai", "iter-extended", "lazy_static", diff --git a/acvm-repo/acir/src/circuit/brillig.rs b/acvm-repo/acir/src/circuit/brillig.rs index a9714ce29b2..ef75d088f8c 100644 --- a/acvm-repo/acir/src/circuit/brillig.rs +++ b/acvm-repo/acir/src/circuit/brillig.rs @@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize}; /// Inputs for the Brillig VM. These are the initial inputs /// that the Brillig VM will use to start. -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug, Hash)] pub enum BrilligInputs { Single(Expression), Array(Vec>), @@ -14,7 +14,7 @@ pub enum BrilligInputs { /// Outputs for the Brillig VM. Once the VM has completed /// execution, this will be the object that is returned. -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug, Hash)] pub enum BrilligOutputs { Simple(Witness), Array(Vec), @@ -23,7 +23,7 @@ pub enum BrilligOutputs { /// This is purely a wrapper struct around a list of Brillig opcode's which represents /// a full Brillig function to be executed by the Brillig VM. /// This is stored separately on a program and accessed through a [BrilligPointer]. -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default, Debug)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default, Debug, Hash)] pub struct BrilligBytecode { pub bytecode: Vec>, } diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index 33982065c2a..88605d3bdab 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -25,7 +25,7 @@ use self::{brillig::BrilligBytecode, opcodes::BlockId}; /// Bounded Expressions are useful if you are eventually going to pass the ACIR /// into a proving system which supports PLONK, where arithmetic expressions have a /// finite fan-in. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default, Hash)] pub enum ExpressionWidth { #[default] Unbounded, @@ -36,13 +36,13 @@ pub enum ExpressionWidth { /// A program represented by multiple ACIR circuits. The execution trace of these /// circuits is dictated by construction of the [crate::native_types::WitnessStack]. -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default, Hash)] pub struct Program { pub functions: Vec>, pub unconstrained_functions: Vec>, } -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default, Hash)] pub struct Circuit { // current_witness_index is the highest witness index in the circuit. The next witness to be added to this circuit // will take on this value. (The value is cached here as an optimization.) @@ -69,13 +69,13 @@ pub struct Circuit { pub assert_messages: Vec<(OpcodeLocation, AssertionPayload)>, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum ExpressionOrMemory { Expression(Expression), Memory(BlockId), } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] pub struct AssertionPayload { pub error_selector: u64, pub payload: Vec>, @@ -355,7 +355,7 @@ impl std::fmt::Debug for Program { } } -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Default, Hash)] pub struct PublicInputs(pub BTreeSet); impl PublicInputs { diff --git a/acvm-repo/acir/src/circuit/opcodes.rs b/acvm-repo/acir/src/circuit/opcodes.rs index 06effd3c5b6..f47c40b0dd7 100644 --- a/acvm-repo/acir/src/circuit/opcodes.rs +++ b/acvm-repo/acir/src/circuit/opcodes.rs @@ -15,7 +15,7 @@ pub use black_box_function_call::{ }; pub use memory_operation::{BlockId, MemOp}; -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum BlockType { Memory, CallData(u32), @@ -29,7 +29,7 @@ impl BlockType { } #[allow(clippy::large_enum_variant)] -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum Opcode { /// An `AssertZero` opcode adds the constraint that `P(w) = 0`, where /// `w=(w_1,..w_n)` is a tuple of `n` witnesses, and `P` is a multi-variate diff --git a/acvm-repo/acir/src/circuit/opcodes/black_box_function_call.rs b/acvm-repo/acir/src/circuit/opcodes/black_box_function_call.rs index fa51caf5155..e756eedefbc 100644 --- a/acvm-repo/acir/src/circuit/opcodes/black_box_function_call.rs +++ b/acvm-repo/acir/src/circuit/opcodes/black_box_function_call.rs @@ -9,13 +9,13 @@ use thiserror::Error; // Note: Some functions will not use all of the witness // So we need to supply how many bits of the witness is needed -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum ConstantOrWitnessEnum { Constant(F), Witness(Witness), } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Hash)] pub struct FunctionInput { input: ConstantOrWitnessEnum, num_bits: u32, @@ -79,7 +79,7 @@ impl std::fmt::Display for FunctionInput { } } -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum BlackBoxFuncCall { AES128Encrypt { inputs: Vec>, diff --git a/acvm-repo/acir/src/circuit/opcodes/memory_operation.rs b/acvm-repo/acir/src/circuit/opcodes/memory_operation.rs index 90e3ee0563a..c9a78983204 100644 --- a/acvm-repo/acir/src/circuit/opcodes/memory_operation.rs +++ b/acvm-repo/acir/src/circuit/opcodes/memory_operation.rs @@ -7,7 +7,7 @@ pub struct BlockId(pub u32); /// Operation on a block of memory /// We can either write or read at an index in memory -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug, Hash)] pub struct MemOp { /// A constant expression that can be 0 (read) or 1 (write) pub operation: Expression, diff --git a/acvm-repo/brillig/src/black_box.rs b/acvm-repo/brillig/src/black_box.rs index 3264388c8ef..cbb268c0a50 100644 --- a/acvm-repo/brillig/src/black_box.rs +++ b/acvm-repo/brillig/src/black_box.rs @@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize}; /// These opcodes provide an equivalent of ACIR blackbox functions. /// They are implemented as native functions in the VM. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum BlackBoxOp { /// Encrypts a message using AES128. AES128Encrypt { diff --git a/acvm-repo/brillig/src/opcodes.rs b/acvm-repo/brillig/src/opcodes.rs index 8b72b5a9b41..1cb31ca3d0a 100644 --- a/acvm-repo/brillig/src/opcodes.rs +++ b/acvm-repo/brillig/src/opcodes.rs @@ -56,7 +56,7 @@ impl MemoryAddress { } /// Describes the memory layout for an array/vector element -#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize, Hash)] pub enum HeapValueType { // A single field element is enough to represent the value with a given bit size Simple(BitSize), @@ -81,7 +81,7 @@ impl HeapValueType { } /// A fixed-sized array starting from a Brillig memory location. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, Hash)] pub struct HeapArray { pub pointer: MemoryAddress, pub size: usize, @@ -94,13 +94,13 @@ impl Default for HeapArray { } /// A memory-sized vector passed starting from a Brillig memory location and with a memory-held size -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, Hash)] pub struct HeapVector { pub pointer: MemoryAddress, pub size: MemoryAddress, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, PartialOrd, Ord)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, PartialOrd, Ord, Hash)] pub enum IntegerBitSize { U1, U8, @@ -152,7 +152,7 @@ impl std::fmt::Display for IntegerBitSize { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, PartialOrd, Ord)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, PartialOrd, Ord, Hash)] pub enum BitSize { Field, Integer(IntegerBitSize), @@ -181,7 +181,7 @@ impl BitSize { /// While we are usually agnostic to how memory is passed within Brillig, /// this needs to be encoded somehow when dealing with an external system. /// For simplicity, the extra type information is given right in the ForeignCall instructions. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, Hash)] pub enum ValueOrArray { /// A single value passed to or from an external call /// It is an 'immediate' value - used without dereferencing. @@ -198,7 +198,7 @@ pub enum ValueOrArray { HeapVector(HeapVector), } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum BrilligOpcode { /// Takes the fields in addresses `lhs` and `rhs` /// Performs the specified binary operation @@ -314,7 +314,7 @@ pub enum BrilligOpcode { } /// Binary fixed-length field expressions -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum BinaryFieldOp { Add, Sub, @@ -332,7 +332,7 @@ pub enum BinaryFieldOp { } /// Binary fixed-length integer expressions -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum BinaryIntOp { Add, Sub, diff --git a/compiler/noirc_driver/src/debug.rs b/compiler/noirc_driver/src/debug.rs index f5eaede89b2..6044e6c0e65 100644 --- a/compiler/noirc_driver/src/debug.rs +++ b/compiler/noirc_driver/src/debug.rs @@ -8,7 +8,7 @@ use std::{ /// For a given file, we store the source code and the path to the file /// so consumers of the debug artifact can reconstruct the original source code structure. -#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, Serialize, Deserialize, Hash)] pub struct DebugFile { pub source: String, pub path: PathBuf, diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index e2237c5c0d9..5bedefaf563 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -334,6 +334,8 @@ pub fn compute_function_abi( /// /// On success this returns the compiled program alongside any warnings that were found. /// On error this returns the non-empty list of warnings and errors. +/// +/// See [compile_no_check] for further information about the use of `cached_program`. pub fn compile_main( context: &mut Context, crate_id: CrateId, @@ -555,6 +557,15 @@ pub const DEFAULT_EXPRESSION_WIDTH: ExpressionWidth = ExpressionWidth::Bounded { /// Compile the current crate using `main_function` as the entrypoint. /// /// This function assumes [`check_crate`] is called beforehand. +/// +/// If the program is not returned from cache, it is backend-agnostic and must go through a transformation +/// pass before usage in proof generation; if it's returned from cache these transformations might have +/// already been applied. +/// +/// The transformations are _not_ covered by the check that decides whether we can use the cached artifact. +/// That comparison is based on on [CompiledProgram::hash] which is a persisted version of the hash of the input +/// [`ast::Program`][noirc_frontend::monomorphization::ast::Program], whereas the output [`circuit::Program`][acir::circuit::Program] +/// contains the final optimized ACIR opcodes, including the transformation done after this compilation. #[tracing::instrument(level = "trace", skip_all, fields(function_name = context.function_name(&main_function)))] pub fn compile_no_check( context: &mut Context, @@ -569,8 +580,6 @@ pub fn compile_no_check( monomorphize(main_function, &mut context.def_interner)? }; - let hash = fxhash::hash64(&program); - let hashes_match = cached_program.as_ref().map_or(false, |program| program.hash == hash); if options.show_monomorphized { println!("{program}"); } @@ -584,10 +593,16 @@ pub fn compile_no_check( || options.show_ssa || options.emit_ssa; - if !force_compile && hashes_match { - info!("Program matches existing artifact, returning early"); - return Ok(cached_program.expect("cache must exist for hashes to match")); + // Hash the AST program, which is going to be used to fingerprint the compilation artifact. + let hash = fxhash::hash64(&program); + + if let Some(cached_program) = cached_program { + if !force_compile && cached_program.hash == hash { + info!("Program matches existing artifact, returning early"); + return Ok(cached_program); + } } + let return_visibility = program.return_visibility; let ssa_evaluator_options = noirc_evaluator::ssa::SsaEvaluatorOptions { ssa_logging: match &options.show_ssa_pass_name { diff --git a/compiler/noirc_driver/src/program.rs b/compiler/noirc_driver/src/program.rs index 88460482928..4b4d6662e8e 100644 --- a/compiler/noirc_driver/src/program.rs +++ b/compiler/noirc_driver/src/program.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use super::debug::DebugFile; -#[derive(Debug, Serialize, Deserialize, Clone)] +#[derive(Debug, Serialize, Deserialize, Clone, Hash)] pub struct CompiledProgram { pub noir_version: String, /// Hash of the [`Program`][noirc_frontend::monomorphization::ast::Program] from which this [`CompiledProgram`] diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 77028f739bd..a5e12b37712 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -94,7 +94,7 @@ impl ProgramDebugInfo { } #[serde_as] -#[derive(Default, Debug, Clone, Deserialize, Serialize)] +#[derive(Default, Debug, Clone, Deserialize, Serialize, Hash)] pub struct DebugInfo { /// Map opcode index of an ACIR circuit into the source code location /// Serde does not support mapping keys being enums for json, so we indicate diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 994e97eabb8..75a3ceb3a72 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -63,7 +63,7 @@ pub enum RuntimeError { UnknownReference { call_stack: CallStack }, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, Hash)] pub enum SsaReport { Warning(InternalWarning), Bug(InternalBug), @@ -107,7 +107,7 @@ impl From for FileDiagnostic { } } -#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize)] +#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize, Hash)] pub enum InternalWarning { #[error("Return variable contains a constant value")] ReturnConstant { call_stack: CallStack }, @@ -115,7 +115,7 @@ pub enum InternalWarning { VerifyProof { call_stack: CallStack }, } -#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize)] +#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize, Hash)] pub enum InternalBug { #[error("Input to brillig function is in a separate subgraph to output")] IndependentSubgraph { call_stack: CallStack }, diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index a04791d764c..8f31023f790 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -238,7 +238,7 @@ impl SsaProgramArtifact { } } -/// Compiles the [`Program`] into [`ACIR``][acvm::acir::circuit::Program]. +/// Compiles the [`Program`] into [`ACIR`][acvm::acir::circuit::Program]. /// /// The output ACIR is backend-agnostic and so must go through a transformation pass before usage in proof generation. #[tracing::instrument(level = "trace", skip_all)] diff --git a/tooling/nargo/src/ops/check.rs b/tooling/nargo/src/ops/check.rs index 14d629ab0f6..707353ccdad 100644 --- a/tooling/nargo/src/ops/check.rs +++ b/tooling/nargo/src/ops/check.rs @@ -2,8 +2,8 @@ use acvm::compiler::CircuitSimulator; use noirc_driver::{CompiledProgram, ErrorsAndWarnings}; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; +/// Run each function through a circuit simulator to check that they are solvable. pub fn check_program(compiled_program: &CompiledProgram) -> Result<(), ErrorsAndWarnings> { - // Check if the program is solvable for (i, circuit) in compiled_program.program.functions.iter().enumerate() { let mut simulator = CircuitSimulator::default(); if !simulator.check_circuit(circuit) { diff --git a/tooling/nargo/src/ops/transform.rs b/tooling/nargo/src/ops/transform.rs index 9255ac3e0ec..fdda368d150 100644 --- a/tooling/nargo/src/ops/transform.rs +++ b/tooling/nargo/src/ops/transform.rs @@ -6,6 +6,7 @@ use iter_extended::vecmap; use noirc_driver::{CompiledContract, CompiledProgram}; use noirc_errors::debug_info::DebugInfo; +/// Apply ACVM optimizations on the circuit. pub fn transform_program( mut compiled_program: CompiledProgram, expression_width: ExpressionWidth, @@ -18,6 +19,7 @@ pub fn transform_program( compiled_program } +/// Apply the optimizing transformation on each function in the contract. pub fn transform_contract( contract: CompiledContract, expression_width: ExpressionWidth, @@ -25,7 +27,6 @@ pub fn transform_contract( let functions = vecmap(contract.functions, |mut func| { func.bytecode = transform_program_internal(func.bytecode, &mut func.debug, expression_width); - func }); diff --git a/tooling/nargo_cli/Cargo.toml b/tooling/nargo_cli/Cargo.toml index 02e669f5c68..5603b7f4fca 100644 --- a/tooling/nargo_cli/Cargo.toml +++ b/tooling/nargo_cli/Cargo.toml @@ -25,6 +25,7 @@ toml.workspace = true [dependencies] clap.workspace = true fm.workspace = true +fxhash.workspace = true iter-extended.workspace = true nargo.workspace = true nargo_fmt.workspace = true diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 304988ed516..ff6009981c7 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -65,6 +65,7 @@ pub(crate) fn run(args: CompileCommand, config: NargoConfig) -> Result<(), CliEr Ok(()) } +/// Continuously recompile the workspace on any Noir file change event. fn watch_workspace(workspace: &Workspace, compile_options: &CompileOptions) -> notify::Result<()> { let (tx, rx) = std::sync::mpsc::channel(); @@ -108,6 +109,8 @@ fn watch_workspace(workspace: &Workspace, compile_options: &CompileOptions) -> n Ok(()) } +/// Parse and compile the entire workspace, then report errors. +/// This is the main entry point used by all other commands that need compilation. pub(super) fn compile_workspace_full( workspace: &Workspace, compile_options: &CompileOptions, @@ -129,6 +132,8 @@ pub(super) fn compile_workspace_full( Ok(()) } +/// Compile binary and contract packages. +/// Returns the merged warnings or errors. fn compile_workspace( file_manager: &FileManager, parsed_files: &ParsedFiles, @@ -144,6 +149,7 @@ fn compile_workspace( // Compile all of the packages in parallel. let program_warnings_or_errors: CompilationResult<()> = compile_programs(file_manager, parsed_files, workspace, &binary_packages, compile_options); + let contract_warnings_or_errors: CompilationResult<()> = compiled_contracts( file_manager, parsed_files, @@ -164,6 +170,7 @@ fn compile_workspace( } } +/// Compile the given binary packages in the workspace. fn compile_programs( file_manager: &FileManager, parsed_files: &ParsedFiles, @@ -171,6 +178,8 @@ fn compile_programs( binary_packages: &[Package], compile_options: &CompileOptions, ) -> CompilationResult<()> { + // Load any existing artifact for a given package, _iff_ it was compiled with the same nargo version. + // The loaded circuit includes backend specific transformations, which might be different from the current target. let load_cached_program = |package| { let program_artifact_path = workspace.package_build_path(package); read_program_from_file(program_artifact_path) @@ -180,19 +189,45 @@ fn compile_programs( }; let compile_package = |package| { + let cached_program = load_cached_program(package); + + // Hash over the entire compiled program, including any post-compile transformations. + // This is used to detect whether `cached_program` is returned by `compile_program`. + let cached_hash = cached_program.as_ref().map(fxhash::hash64); + + // Compile the program, or use the cached artifacts if it matches. let (program, warnings) = compile_program( file_manager, parsed_files, workspace, package, compile_options, - load_cached_program(package), + cached_program, )?; + // Choose the target width for the final, backend specific transformation. let target_width = get_target_width(package.expression_width, compile_options.expression_width); + + // If the compiled program is the same as the cached one, we don't apply transformations again, unless the target width has changed. + // The transformations might not be idempotent, which would risk creating witnesses that don't work with earlier versions, + // based on which we might have generated a verifier already. + if cached_hash == Some(fxhash::hash64(&program)) { + let width_matches = program + .program + .functions + .iter() + .all(|circuit| circuit.expression_width == target_width); + + if width_matches { + return Ok(((), warnings)); + } + } + // Run ACVM optimizations and set the target width. let program = nargo::ops::transform_program(program, target_width); + // Check solvability. nargo::ops::check_program(&program)?; + // Overwrite the build artifacts with the final circuit, which includes the backend specific transformations. save_program_to_file(&program.into(), &package.name, workspace.target_directory_path()); Ok(((), warnings)) @@ -208,6 +243,7 @@ fn compile_programs( collect_errors(program_results).map(|(_, warnings)| ((), warnings)) } +/// Compile the given contracts in the workspace. fn compiled_contracts( file_manager: &FileManager, parsed_files: &ParsedFiles, diff --git a/tooling/nargo_cli/tests/stdlib-props.rs b/tooling/nargo_cli/tests/stdlib-props.rs index 370d2c04052..86c225831b9 100644 --- a/tooling/nargo_cli/tests/stdlib-props.rs +++ b/tooling/nargo_cli/tests/stdlib-props.rs @@ -61,6 +61,7 @@ fn prepare_and_compile_snippet( ) -> CompilationResult { let (mut context, root_crate_id) = prepare_snippet(source); let options = CompileOptions { force_brillig, ..Default::default() }; + // TODO: Run nargo::ops::transform_program? compile_main(&mut context, root_crate_id, &options, None) } diff --git a/tooling/noirc_abi/src/lib.rs b/tooling/noirc_abi/src/lib.rs index b1b199727c2..bd5674d64f1 100644 --- a/tooling/noirc_abi/src/lib.rs +++ b/tooling/noirc_abi/src/lib.rs @@ -49,6 +49,7 @@ pub const MAIN_RETURN_NAME: &str = "return"; /// depends on the types of programs that users want to do. I don't envision string manipulation /// in programs, however it is possible to support, with many complications like encoding character set /// support. +#[derive(Hash)] pub enum AbiType { Field, Array { @@ -77,7 +78,7 @@ pub enum AbiType { }, } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Hash)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[serde(rename_all = "lowercase")] /// Represents whether the parameter is public or known only to the prover. @@ -89,7 +90,7 @@ pub enum AbiVisibility { DataBus, } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Hash)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[serde(rename_all = "lowercase")] pub enum Sign { @@ -146,7 +147,7 @@ impl From<&AbiType> for PrintableType { } } -#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, Hash)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] /// An argument or return value of the circuit's `main` function. pub struct AbiParameter { @@ -163,7 +164,7 @@ impl AbiParameter { } } -#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, Serialize, Deserialize, Hash)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] pub struct AbiReturnType { #[cfg_attr(test, proptest(strategy = "arbitrary::arb_abi_type()"))] @@ -171,7 +172,7 @@ pub struct AbiReturnType { pub visibility: AbiVisibility, } -#[derive(Clone, Debug, Default, Serialize, Deserialize)] +#[derive(Clone, Debug, Default, Serialize, Deserialize, Hash)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] pub struct Abi { /// An ordered list of the arguments to the program's `main` function, specifying their types and visibility. @@ -459,7 +460,7 @@ pub enum AbiValue { }, } -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Hash)] #[serde(tag = "error_kind", rename_all = "lowercase")] pub enum AbiErrorType { FmtString { length: u32, item_types: Vec }, From 80c5c7d83abdd29e7cd157dfdb1880cde68f48bb Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 3 Dec 2024 15:05:10 +0000 Subject: [PATCH 2/2] chore: fix tests in `noirc_abi_wasm` (#6688) --- tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts | 6 +++--- tooling/noirc_abi_wasm/test/node/abi_encode.test.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts b/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts index 77903ea8ddd..ac18495919c 100644 --- a/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts +++ b/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts @@ -15,8 +15,8 @@ it('recovers original inputs when abi encoding and decoding', async () => { const foo: Field = inputs.foo as Field; const bar: Field[] = inputs.bar as Field[]; expect(BigInt(decoded_inputs.inputs.foo)).to.be.equal(BigInt(foo)); - expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0])); - expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1])); - expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2])); + expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0].toString())); + expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1].toString())); + expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2].toString())); expect(decoded_inputs.return_value).to.be.null; }); diff --git a/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts b/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts index 4bb46f1ddd1..e87618d84da 100644 --- a/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts +++ b/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts @@ -11,8 +11,8 @@ it('recovers original inputs when abi encoding and decoding', async () => { const foo: Field = inputs.foo as Field; const bar: Field[] = inputs.bar as Field[]; expect(BigInt(decoded_inputs.inputs.foo)).to.be.equal(BigInt(foo)); - expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0])); - expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1])); - expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2])); + expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0].toString())); + expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1].toString())); + expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2].toString())); expect(decoded_inputs.return_value).to.be.null; });