From dea6b323fe8db636f5991cfc206ea9222addca30 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 15 Jul 2024 16:59:51 -0400 Subject: [PATCH] feat(nargo): Default expression width field in `Nargo.toml` (#5505) # Description ## Problem\* Resolves #4961 ## Summary\* This PR let's a developer override the default expression width target with a field in the `Nargo.toml` manifest. Due to the limitations of the `clap` macros I had to switch `expression_width` on the `CompileOptions` struct to optional. I then hardcoded a constant that specifies we want width 4 as Noir's compilation default. We then settle on a target expression width by checking the following: 1. Is there a user supplied expression width from the CLI flag? If there is use that width. 2. Is there no user supplied width and an expression width specified in the manifest? Use the expression width in the manifest. 3. Is there no user supplied width and no width specified in the manifest? Use the hardcoded default value. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- Cargo.lock | 1 + compiler/noirc_driver/src/lib.rs | 8 ++--- compiler/wasm/src/compile.rs | 20 +++++-------- compiler/wasm/src/compile_new.rs | 15 ++++++---- .../hello_noir/project_breakdown.md | 1 + .../poseidon_bn254_hash_width_3/Nargo.toml | 9 ++++++ .../poseidon_bn254_hash_width_3/Prover.toml | 4 +++ .../poseidon_bn254_hash_width_3/src/main.nr | 9 ++++++ tooling/lsp/src/lib.rs | 1 + tooling/nargo/src/package.rs | 2 ++ tooling/nargo_cli/src/cli/compile_cmd.rs | 30 +++++++++++++++++-- tooling/nargo_cli/src/cli/debug_cmd.rs | 7 +++-- tooling/nargo_cli/src/cli/info_cmd.rs | 12 ++++---- tooling/nargo_cli/tests/stdlib-tests.rs | 1 + tooling/nargo_toml/Cargo.toml | 1 + tooling/nargo_toml/src/errors.rs | 3 ++ tooling/nargo_toml/src/lib.rs | 28 +++++++++++++++++ tooling/nargo_toml/src/semver.rs | 6 ++++ 18 files changed, 126 insertions(+), 32 deletions(-) create mode 100644 test_programs/execution_success/poseidon_bn254_hash_width_3/Nargo.toml create mode 100644 test_programs/execution_success/poseidon_bn254_hash_width_3/Prover.toml create mode 100644 test_programs/execution_success/poseidon_bn254_hash_width_3/src/main.nr diff --git a/Cargo.lock b/Cargo.lock index 1df84a80bc7..5d487f4f23f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2594,6 +2594,7 @@ dependencies = [ "dirs", "fm", "nargo", + "noirc_driver", "noirc_frontend", "semver", "serde", diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 2b0769e30d4..3ec8c102aec 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -52,9 +52,9 @@ pub const NOIR_ARTIFACT_VERSION_STRING: &str = #[derive(Args, Clone, Debug, Default)] pub struct CompileOptions { - /// Override the expression width requested by the backend. - #[arg(long, value_parser = parse_expression_width, default_value = "4")] - pub expression_width: ExpressionWidth, + /// Specify the backend expression width that should be targeted + #[arg(long, value_parser = parse_expression_width)] + pub expression_width: Option, /// Force a full recompilation. #[arg(long = "force")] @@ -113,7 +113,7 @@ pub struct CompileOptions { pub show_artifact_paths: bool, } -fn parse_expression_width(input: &str) -> Result { +pub fn parse_expression_width(input: &str) -> Result { use std::io::{Error, ErrorKind}; let width = input .parse::() diff --git a/compiler/wasm/src/compile.rs b/compiler/wasm/src/compile.rs index 59b0e00e49f..05f42bc91a1 100644 --- a/compiler/wasm/src/compile.rs +++ b/compiler/wasm/src/compile.rs @@ -164,10 +164,9 @@ pub fn compile_program( console_error_panic_hook::set_once(); let (crate_id, mut context) = prepare_context(entry_point, dependency_graph, file_source_map)?; - let compile_options = CompileOptions { - expression_width: ExpressionWidth::Bounded { width: 4 }, - ..CompileOptions::default() - }; + let expression_width = ExpressionWidth::Bounded { width: 4 }; + let compile_options = + CompileOptions { expression_width: Some(expression_width), ..CompileOptions::default() }; let compiled_program = noirc_driver::compile_main(&mut context, crate_id, &compile_options, None) @@ -180,8 +179,7 @@ pub fn compile_program( })? .0; - let optimized_program = - nargo::ops::transform_program(compiled_program, compile_options.expression_width); + let optimized_program = nargo::ops::transform_program(compiled_program, expression_width); let warnings = optimized_program.warnings.clone(); Ok(JsCompileProgramResult::new(optimized_program.into(), warnings)) @@ -196,10 +194,9 @@ pub fn compile_contract( console_error_panic_hook::set_once(); let (crate_id, mut context) = prepare_context(entry_point, dependency_graph, file_source_map)?; - let compile_options = CompileOptions { - expression_width: ExpressionWidth::Bounded { width: 4 }, - ..CompileOptions::default() - }; + let expression_width = ExpressionWidth::Bounded { width: 4 }; + let compile_options = + CompileOptions { expression_width: Some(expression_width), ..CompileOptions::default() }; let compiled_contract = noirc_driver::compile_contract(&mut context, crate_id, &compile_options) @@ -212,8 +209,7 @@ pub fn compile_contract( })? .0; - let optimized_contract = - nargo::ops::transform_contract(compiled_contract, compile_options.expression_width); + let optimized_contract = nargo::ops::transform_contract(compiled_contract, expression_width); let warnings = optimized_contract.warnings.clone(); Ok(JsCompileContractResult::new(optimized_contract.into(), warnings)) diff --git a/compiler/wasm/src/compile_new.rs b/compiler/wasm/src/compile_new.rs index d5f02833521..ef2af1dd654 100644 --- a/compiler/wasm/src/compile_new.rs +++ b/compiler/wasm/src/compile_new.rs @@ -100,7 +100,10 @@ impl CompilerContext { } else { ExpressionWidth::Bounded { width: 4 } }; - let compile_options = CompileOptions { expression_width, ..CompileOptions::default() }; + let compile_options = CompileOptions { + expression_width: Some(expression_width), + ..CompileOptions::default() + }; let root_crate_id = *self.context.root_crate_id(); let compiled_program = @@ -114,8 +117,7 @@ impl CompilerContext { })? .0; - let optimized_program = - nargo::ops::transform_program(compiled_program, compile_options.expression_width); + let optimized_program = nargo::ops::transform_program(compiled_program, expression_width); let warnings = optimized_program.warnings.clone(); Ok(JsCompileProgramResult::new(optimized_program.into(), warnings)) @@ -130,7 +132,10 @@ impl CompilerContext { } else { ExpressionWidth::Bounded { width: 4 } }; - let compile_options = CompileOptions { expression_width, ..CompileOptions::default() }; + let compile_options = CompileOptions { + expression_width: Some(expression_width), + ..CompileOptions::default() + }; let root_crate_id = *self.context.root_crate_id(); let compiled_contract = @@ -145,7 +150,7 @@ impl CompilerContext { .0; let optimized_contract = - nargo::ops::transform_contract(compiled_contract, compile_options.expression_width); + nargo::ops::transform_contract(compiled_contract, expression_width); let warnings = optimized_contract.warnings.clone(); Ok(JsCompileContractResult::new(optimized_contract.into(), warnings)) diff --git a/docs/docs/getting_started/hello_noir/project_breakdown.md b/docs/docs/getting_started/hello_noir/project_breakdown.md index 29688df148f..525b8dabdd8 100644 --- a/docs/docs/getting_started/hello_noir/project_breakdown.md +++ b/docs/docs/getting_started/hello_noir/project_breakdown.md @@ -67,6 +67,7 @@ The package section defines a number of fields including: - `entry` (optional) - a relative filepath to use as the entry point into your package (overrides the default of `src/lib.nr` or `src/main.nr`) - `backend` (optional) - `license` (optional) +- `expression_width` (optional) - Sets the default backend expression width. This field will override the default backend expression width specified by the Noir compiler (currently set to width 4). #### Dependencies section diff --git a/test_programs/execution_success/poseidon_bn254_hash_width_3/Nargo.toml b/test_programs/execution_success/poseidon_bn254_hash_width_3/Nargo.toml new file mode 100644 index 00000000000..7047f0aeef2 --- /dev/null +++ b/test_programs/execution_success/poseidon_bn254_hash_width_3/Nargo.toml @@ -0,0 +1,9 @@ +[package] +name = "poseidon_bn254_hash_width_3" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" +# Test usage of `expression_width` field +expression_width = "3" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/poseidon_bn254_hash_width_3/Prover.toml b/test_programs/execution_success/poseidon_bn254_hash_width_3/Prover.toml new file mode 100644 index 00000000000..8eecf9a3db2 --- /dev/null +++ b/test_programs/execution_success/poseidon_bn254_hash_width_3/Prover.toml @@ -0,0 +1,4 @@ +x1 = [1,2] +y1 = "0x115cc0f5e7d690413df64c6b9662e9cf2a3617f2743245519e19607a4417189a" +x2 = [1,2,3,4] +y2 = "0x299c867db6c1fdd79dcefa40e4510b9837e60ebb1ce0663dbaa525df65250465" diff --git a/test_programs/execution_success/poseidon_bn254_hash_width_3/src/main.nr b/test_programs/execution_success/poseidon_bn254_hash_width_3/src/main.nr new file mode 100644 index 00000000000..bb441a1ace3 --- /dev/null +++ b/test_programs/execution_success/poseidon_bn254_hash_width_3/src/main.nr @@ -0,0 +1,9 @@ +use std::hash::poseidon; + +fn main(x1: [Field; 2], y1: pub Field, x2: [Field; 4], y2: pub Field) { + let hash1 = poseidon::bn254::hash_2(x1); + assert(hash1 == y1); + + let hash2 = poseidon::bn254::hash_4(x2); + assert(hash2 == y2); +} diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 28ba5425fe7..542876d6319 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -283,6 +283,7 @@ pub(crate) fn resolve_workspace_for_source_path( name: CrateName::from_str(parent_folder) .map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?, dependencies: BTreeMap::new(), + expression_width: None, }; let workspace = Workspace { root_dir: PathBuf::from(parent_folder), diff --git a/tooling/nargo/src/package.rs b/tooling/nargo/src/package.rs index 44f0a3504f7..f55ca5550a3 100644 --- a/tooling/nargo/src/package.rs +++ b/tooling/nargo/src/package.rs @@ -1,5 +1,6 @@ use std::{collections::BTreeMap, fmt::Display, path::PathBuf}; +use acvm::acir::circuit::ExpressionWidth; use noirc_frontend::graph::CrateName; use crate::constants::PROVER_INPUT_FILE; @@ -51,6 +52,7 @@ pub struct Package { pub entry_path: PathBuf, pub name: CrateName, pub dependencies: BTreeMap, + pub expression_width: Option, } impl Package { diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index e83b1728c93..a2877ebdeac 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -2,6 +2,7 @@ use std::io::Write; use std::path::Path; use std::time::Duration; +use acvm::acir::circuit::ExpressionWidth; use fm::FileManager; use nargo::ops::{collect_errors, compile_contract, compile_program, report_errors}; use nargo::package::Package; @@ -190,7 +191,11 @@ fn compile_programs( compile_options, load_cached_program(package), )?; - let program = nargo::ops::transform_program(program, compile_options.expression_width); + + let target_width = + get_target_width(package.expression_width, compile_options.expression_width); + let program = nargo::ops::transform_program(program, target_width); + save_program_to_file( &program.clone().into(), &package.name, @@ -216,8 +221,9 @@ fn compiled_contracts( .map(|package| { let (contract, warnings) = compile_contract(file_manager, parsed_files, package, compile_options)?; - let contract = - nargo::ops::transform_contract(contract, compile_options.expression_width); + let target_width = + get_target_width(package.expression_width, compile_options.expression_width); + let contract = nargo::ops::transform_contract(contract, target_width); save_contract(contract, package, target_dir, compile_options.show_artifact_paths); Ok(((), warnings)) }) @@ -243,3 +249,21 @@ fn save_contract( println!("Saved contract artifact to: {}", artifact_path.display()); } } + +/// Default expression width used for Noir compilation. +/// The ACVM native type `ExpressionWidth` has its own default which should always be unbounded, +/// while we can sometimes expect the compilation target width to change. +/// Thus, we set it separately here rather than trying to alter the default derivation of the type. +const DEFAULT_EXPRESSION_WIDTH: ExpressionWidth = ExpressionWidth::Bounded { width: 4 }; + +/// If a target width was not specified in the CLI we can safely override the default. +pub(crate) fn get_target_width( + package_default_width: Option, + compile_options_width: Option, +) -> ExpressionWidth { + if let (Some(manifest_default_width), None) = (package_default_width, compile_options_width) { + manifest_default_width + } else { + compile_options_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH) + } +} diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 2014a3acaa4..311af9b9db0 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -22,6 +22,7 @@ use noirc_frontend::debug::DebugInstrumenter; use noirc_frontend::graph::CrateName; use noirc_frontend::hir::ParsedFiles; +use super::compile_cmd::get_target_width; use super::fs::{inputs::read_inputs_from_file, witness::save_witness_to_dir}; use super::NargoConfig; use crate::errors::CliError; @@ -80,8 +81,10 @@ pub(crate) fn run(args: DebugCommand, config: NargoConfig) -> Result<(), CliErro args.compile_options.clone(), )?; - let compiled_program = - nargo::ops::transform_program(compiled_program, args.compile_options.expression_width); + let target_width = + get_target_width(package.expression_width, args.compile_options.expression_width); + + let compiled_program = nargo::ops::transform_program(compiled_program, target_width); run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir) } diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index 3759fb31c76..a6395d1c8c9 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -16,7 +16,9 @@ use serde::Serialize; use crate::errors::CliError; use super::{ - compile_cmd::compile_workspace_full, fs::program::read_program_from_file, NargoConfig, + compile_cmd::{compile_workspace_full, get_target_width}, + fs::program::read_program_from_file, + NargoConfig, }; /// Provides detailed information on each of a program's function (represented by a single circuit) @@ -84,11 +86,9 @@ pub(crate) fn run(args: InfoCommand, config: NargoConfig) -> Result<(), CliError .into_iter() .par_bridge() .map(|(package, program)| { - count_opcodes_and_gates_in_program( - program, - &package, - args.compile_options.expression_width, - ) + let target_width = + get_target_width(package.expression_width, args.compile_options.expression_width); + count_opcodes_and_gates_in_program(program, &package, target_width) }) .collect(); diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index 10711b6d011..cc44b269c6a 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -27,6 +27,7 @@ fn run_stdlib_tests() { entry_path: PathBuf::from("main.nr"), name: "stdlib".parse().unwrap(), dependencies: BTreeMap::new(), + expression_width: None, }; let (mut context, dummy_crate_id) = diff --git a/tooling/nargo_toml/Cargo.toml b/tooling/nargo_toml/Cargo.toml index 574972d99e7..7c9faa4562a 100644 --- a/tooling/nargo_toml/Cargo.toml +++ b/tooling/nargo_toml/Cargo.toml @@ -18,6 +18,7 @@ serde.workspace = true thiserror.workspace = true toml.workspace = true url.workspace = true +noirc_driver.workspace = true semver = "1.0.20" [dev-dependencies] diff --git a/tooling/nargo_toml/src/errors.rs b/tooling/nargo_toml/src/errors.rs index 77fe77bcdbb..1ee8e90c8e5 100644 --- a/tooling/nargo_toml/src/errors.rs +++ b/tooling/nargo_toml/src/errors.rs @@ -72,6 +72,9 @@ pub enum ManifestError { #[error("Cyclic package dependency found when processing {cycle}")] CyclicDependency { cycle: String }, + + #[error("Failed to parse expression width with the following error: {0}")] + ParseExpressionWidth(String), } #[allow(clippy::enum_variant_names)] diff --git a/tooling/nargo_toml/src/lib.rs b/tooling/nargo_toml/src/lib.rs index 985cb30dc24..c0d8c7997fd 100644 --- a/tooling/nargo_toml/src/lib.rs +++ b/tooling/nargo_toml/src/lib.rs @@ -14,6 +14,7 @@ use nargo::{ package::{Dependency, Package, PackageType}, workspace::Workspace, }; +use noirc_driver::parse_expression_width; use noirc_frontend::graph::CrateName; use serde::Deserialize; @@ -199,6 +200,16 @@ impl PackageConfig { })?; } + let expression_width = self + .package + .expression_width + .as_ref() + .map(|expression_width| { + parse_expression_width(expression_width) + .map_err(|err| ManifestError::ParseExpressionWidth(err.to_string())) + }) + .map_or(Ok(None), |res| res.map(Some))?; + Ok(Package { version: self.package.version.clone(), compiler_required_version: self.package.compiler_version.clone(), @@ -207,6 +218,7 @@ impl PackageConfig { package_type, name, dependencies, + expression_width, }) } } @@ -275,6 +287,7 @@ struct PackageMetadata { // so you will not need to supply an ACIR and compiler version compiler_version: Option, license: Option, + expression_width: Option, } #[derive(Debug, Deserialize, Clone)] @@ -531,3 +544,18 @@ fn parse_workspace_default_member_toml() { assert!(Config::try_from(String::from(src)).is_ok()); assert!(Config::try_from(src).is_ok()); } + +#[test] +fn parse_package_expression_width_toml() { + let src = r#" + [package] + name = "test" + version = "0.1.0" + type = "bin" + authors = [""] + expression_width = "3" + "#; + + assert!(Config::try_from(String::from(src)).is_ok()); + assert!(Config::try_from(src).is_ok()); +} diff --git a/tooling/nargo_toml/src/semver.rs b/tooling/nargo_toml/src/semver.rs index 7c6e2a18b31..253ac82aa34 100644 --- a/tooling/nargo_toml/src/semver.rs +++ b/tooling/nargo_toml/src/semver.rs @@ -89,6 +89,7 @@ mod tests { name: CrateName::from_str("test").unwrap(), dependencies: BTreeMap::new(), version: Some("1.0".to_string()), + expression_width: None, }; if let Err(err) = semver_check_package(&package, &compiler_version) { panic!("semver check should have passed. compiler version is 0.1.0 and required version from the package is 0.1.0\n error: {err:?}") @@ -120,6 +121,7 @@ mod tests { name: CrateName::from_str("test").unwrap(), dependencies: BTreeMap::new(), version: Some("1.0".to_string()), + expression_width: None, }; let valid_dependency = Package { @@ -130,6 +132,7 @@ mod tests { name: CrateName::from_str("good_dependency").unwrap(), dependencies: BTreeMap::new(), version: Some("1.0".to_string()), + expression_width: None, }; let invalid_dependency = Package { compiler_required_version: Some("0.2.0".to_string()), @@ -139,6 +142,7 @@ mod tests { name: CrateName::from_str("bad_dependency").unwrap(), dependencies: BTreeMap::new(), version: Some("1.0".to_string()), + expression_width: None, }; package.dependencies.insert( @@ -179,6 +183,7 @@ mod tests { name: CrateName::from_str("test").unwrap(), dependencies: BTreeMap::new(), version: Some("1.0".to_string()), + expression_width: None, }; if let Err(err) = semver_check_package(&package, &compiler_version) { @@ -198,6 +203,7 @@ mod tests { name: CrateName::from_str("test").unwrap(), dependencies: BTreeMap::new(), version: Some("1.0".to_string()), + expression_width: None, }; if let Err(err) = semver_check_package(&package, &compiler_version) {