From 38249dd7d9243ac0084382c19f3c9d43d5af34a2 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut Date: Sat, 25 Jan 2025 06:09:21 +0300 Subject: [PATCH 01/14] Make the lookback feature opt-in --- Cargo.lock | 130 +++++++++--------- compiler/noirc_driver/src/lib.rs | 8 ++ compiler/noirc_evaluator/src/errors.rs | 2 +- compiler/noirc_evaluator/src/ssa.rs | 11 +- .../check_for_underconstrained_values.rs | 57 +++++--- compiler/noirc_evaluator/src/ssa/opt/hint.rs | 1 + 6 files changed, 120 insertions(+), 89 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f961c452862..aeaba0aa381 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -683,9 +683,9 @@ checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" [[package]] name = "bitflags" -version = "2.7.0" +version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1be3f42a67d6d345ecd59f675f3f012d6974981560836e938c22b424b85ce1be" +checksum = "8f68f53c83ab957f72c32642f3868eec03eb974d1fb82e453128456482613d36" [[package]] name = "bitmaps" @@ -854,9 +854,9 @@ checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" [[package]] name = "cc" -version = "1.2.9" +version = "1.2.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c8293772165d9345bdaaa39b45b2109591e63fe5e6fbc23c6ff930a048aa310b" +checksum = "13208fcbb66eaeffe09b99fffbe1af420f00a7b35aa99ad683dfc1aa76145229" dependencies = [ "shlex", ] @@ -917,9 +917,9 @@ dependencies = [ [[package]] name = "clap" -version = "4.5.26" +version = "4.5.27" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8eb5e908ef3a6efbe1ed62520fb7287959888c88485abe072543190ecc66783" +checksum = "769b0145982b4b48713e01ec42d61614425f27b7058bda7180a3a41f30104796" dependencies = [ "clap_builder", "clap_derive", @@ -935,9 +935,9 @@ dependencies = [ [[package]] name = "clap_builder" -version = "4.5.26" +version = "4.5.27" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "96b01801b5fc6a0a232407abc821660c9c6d25a1cafc0d4f85f29fb8d9afc121" +checksum = "1b26884eb4b57140e4d2d93652abfa49498b938b3c9179f9fc487b0acc3edad7" dependencies = [ "anstream", "anstyle", @@ -1160,9 +1160,9 @@ dependencies = [ [[package]] name = "cpufeatures" -version = "0.2.16" +version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "16b80225097f2e5ae4e7179dd2266824648f3e2f49d9134d584b76389d31c4c3" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" dependencies = [ "libc", ] @@ -1248,9 +1248,9 @@ checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" [[package]] name = "crunchy" -version = "0.2.2" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7a81dae078cea95a014a339291cec439d2f232ebe854a9d672b796c6afafa9b7" +checksum = "43da5946c66ffcc7745f48db692ffbb10a83bfe0afd96235c5c2a4fb23994929" [[package]] name = "crypto-bigint" @@ -1943,7 +1943,7 @@ version = "0.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0bf760ebf69878d9fd8f110c89703d90ce35095324d1f1edcb595c63945ee757" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", "ignore", "walkdir", ] @@ -1996,7 +1996,7 @@ dependencies = [ "futures-core", "futures-sink", "http", - "indexmap 2.7.0", + "indexmap 2.7.1", "slab", "tokio", "tokio-util", @@ -2462,9 +2462,9 @@ dependencies = [ [[package]] name = "indexmap" -version = "2.7.0" +version = "2.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "62f822373a4fe84d4bb149bf54e584a7f4abec90e072ed49cda0edea5b95471f" +checksum = "8c9c992b02b5b4c94ea26e32fe5bccb7aa7d9f390ab5c1221ff895bc7ea8b652" dependencies = [ "equivalent", "hashbrown 0.15.2", @@ -2483,7 +2483,7 @@ dependencies = [ "crossbeam-utils", "dashmap", "env_logger", - "indexmap 2.7.0", + "indexmap 2.7.1", "is-terminal", "itoa", "log", @@ -2525,13 +2525,13 @@ dependencies = [ [[package]] name = "is-terminal" -version = "0.4.13" +version = "0.4.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "261f68e344040fbd0edea105bef17c66edf46f984ddb1115b775ce31be948f4b" +checksum = "e19b23d53f35ce9f56aebc7d1bb4e6ac1e9c0db7ac85c8d1760c04379edced37" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -2599,9 +2599,9 @@ dependencies = [ [[package]] name = "jsonrpsee" -version = "0.24.7" +version = "0.24.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c5c71d8c1a731cc4227c2f698d377e7848ca12c8a48866fc5e6951c43a4db843" +checksum = "834af00800e962dee8f7bfc0f60601de215e73e78e5497d733a2919da837d3c8" dependencies = [ "jsonrpsee-core", "jsonrpsee-http-client", @@ -2614,9 +2614,9 @@ dependencies = [ [[package]] name = "jsonrpsee-core" -version = "0.24.7" +version = "0.24.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2882f6f8acb9fdaec7cefc4fd607119a9bd709831df7d7672a1d3b644628280" +checksum = "76637f6294b04e747d68e69336ef839a3493ca62b35bf488ead525f7da75c5bb" dependencies = [ "async-trait", "bytes", @@ -2637,9 +2637,9 @@ dependencies = [ [[package]] name = "jsonrpsee-http-client" -version = "0.24.7" +version = "0.24.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b3638bc4617f96675973253b3a45006933bde93c2fd8a6170b33c777cc389e5b" +checksum = "87c24e981ad17798bbca852b0738bfb7b94816ed687bd0d5da60bfa35fa0fdc3" dependencies = [ "async-trait", "base64 0.22.1", @@ -2662,9 +2662,9 @@ dependencies = [ [[package]] name = "jsonrpsee-proc-macros" -version = "0.24.7" +version = "0.24.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c06c01ae0007548e73412c08e2285ffe5d723195bf268bce67b1b77c3bb2a14d" +checksum = "6fcae0c6c159e11541080f1f829873d8f374f81eda0abc67695a13fc8dc1a580" dependencies = [ "heck 0.5.0", "proc-macro-crate", @@ -2675,9 +2675,9 @@ dependencies = [ [[package]] name = "jsonrpsee-server" -version = "0.24.7" +version = "0.24.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "82ad8ddc14be1d4290cd68046e7d1d37acd408efed6d3ca08aefcc3ad6da069c" +checksum = "66b7a3df90a1a60c3ed68e7ca63916b53e9afa928e33531e87f61a9c8e9ae87b" dependencies = [ "futures-util", "http", @@ -2702,9 +2702,9 @@ dependencies = [ [[package]] name = "jsonrpsee-types" -version = "0.24.7" +version = "0.24.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a178c60086f24cc35bb82f57c651d0d25d99c4742b4d335de04e97fa1f08a8a1" +checksum = "ddb81adb1a5ae9182df379e374a79e24e992334e7346af4d065ae5b2acb8d4c6" dependencies = [ "http", "serde", @@ -2794,7 +2794,7 @@ version = "0.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3af92c55d7d839293953fcd0fda5ecfe93297cfde6ffbdec13b41d99c0ba6607" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", "libc", "redox_syscall 0.4.1", ] @@ -2805,7 +2805,7 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c0ff37bd590ca25063e35af745c343cb7a0271906fb7b37e4813e8f79f00268d" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", "libc", "redox_syscall 0.5.8", ] @@ -3454,7 +3454,7 @@ version = "6.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6205bd8bb1e454ad2e27422015fb5e4f2bcc7e08fa8f27058670d208324a4d2d" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", "crossbeam-channel", "filetime", "fsevent-sys", @@ -3592,9 +3592,9 @@ checksum = "b410bbe7e14ab526a0e86877eb47c6996a2bd7746f027ba551028c925390e4e9" [[package]] name = "openssl-probe" -version = "0.1.5" +version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" +checksum = "d05e27ee213611ffe7d6348b942e8f942b37114c00cc03cec254295a4a17852e" [[package]] name = "overload" @@ -3700,7 +3700,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4c5cc86750666a3ed20bdaf5ca2a0344f9c67674cae0515bec2da16fbaa47db" dependencies = [ "fixedbitset", - "indexmap 2.7.0", + "indexmap 2.7.1", ] [[package]] @@ -3948,7 +3948,7 @@ checksum = "14cae93065090804185d3b75f0bf93b8eeda30c7a9b4a33d3bdb3988d6229e50" dependencies = [ "bit-set", "bit-vec", - "bitflags 2.7.0", + "bitflags 2.8.0", "lazy_static", "num-traits", "rand", @@ -4100,7 +4100,7 @@ version = "0.5.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "03a862b389f93e68874fbf580b9de08dd02facb9a788ebadaf4a3fd33cf58834" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", ] [[package]] @@ -4281,11 +4281,11 @@ dependencies = [ [[package]] name = "rustix" -version = "0.38.43" +version = "0.38.44" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a78891ee6bf2340288408954ac787aa063d8e8817e9f53abb37c695c6d834ef6" +checksum = "fdb5bc1ae2baa591800df16c9ca78619bf65c0488b41b96ccec5d11220d8c154" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", "errno", "libc", "linux-raw-sys", @@ -4534,7 +4534,7 @@ version = "2.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "897b2245f0b511c87893af39b033e5ca9cce68824c4d7e7630b5a1d339658d02" dependencies = [ - "bitflags 2.7.0", + "bitflags 2.8.0", "core-foundation", "core-foundation-sys", "libc", @@ -4554,9 +4554,9 @@ dependencies = [ [[package]] name = "semver" -version = "1.0.24" +version = "1.0.25" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3cb6eb87a131f756572d7fb904f6e7b68633f09cca868c5df1c4b8d1a694bbba" +checksum = "f79dfe2d285b0488816f30e700a7438c5a73d816b5b7d3ac72fbc48b0d185e03" [[package]] name = "serde" @@ -4614,9 +4614,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.135" +version = "1.0.137" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2b0d7ba2887406110130a978386c4e1befb98c674b4fba677954e4db976630d9" +checksum = "930cfb6e6abf99298aaad7d29abbef7a9999a9a8806a40088f55f0dcec03146b" dependencies = [ "itoa", "memchr", @@ -4654,7 +4654,7 @@ dependencies = [ "chrono", "hex", "indexmap 1.9.3", - "indexmap 2.7.0", + "indexmap 2.7.1", "serde", "serde_derive", "serde_json", @@ -4739,9 +4739,9 @@ dependencies = [ [[package]] name = "similar" -version = "2.6.0" +version = "2.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1de1d4f81173b03af4c0cbed3c898f6bff5b870e4a7f5d6f4057d62a7a4b686e" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" dependencies = [ "bstr", "unicode-segmentation", @@ -4749,9 +4749,9 @@ dependencies = [ [[package]] name = "similar-asserts" -version = "1.6.0" +version = "1.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cfe85670573cd6f0fa97940f26e7e6601213c3b0555246c24234131f88c5709e" +checksum = "9f08357795f0d604ea7d7130f22c74b03838c959bdb14adde3142aab4d18a293" dependencies = [ "console", "similar", @@ -4920,9 +4920,9 @@ checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" [[package]] name = "symbolic-common" -version = "12.13.2" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8150eae9699e3c73a3e6431dc1f80d87748797c0457336af23e94c1de619ed24" +checksum = "13a4dfe4bbeef59c1f32fc7524ae7c95b9e1de5e79a43ce1604e181081d71b0c" dependencies = [ "debugid", "memmap2", @@ -4932,9 +4932,9 @@ dependencies = [ [[package]] name = "symbolic-demangle" -version = "12.13.2" +version = "12.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "95f4a9846f7a8933b6d198c022faa2c9bd89e1a970bed9d9a98d25708bf8de17" +checksum = "98cf6a95abff97de4d7ff3473f33cacd38f1ddccad5c1feab435d6760300e3b6" dependencies = [ "cpp_demangle", "rustc-demangle", @@ -5257,7 +5257,7 @@ version = "0.19.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1b5bb770da30e5cbfde35a2d7b9b8a2c4b8ef89548a7a6aeab5c9a576e3e7421" dependencies = [ - "indexmap 2.7.0", + "indexmap 2.7.1", "serde", "serde_spanned", "toml_datetime", @@ -5270,7 +5270,7 @@ version = "0.22.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4ae48d6208a266e853d946088ed816055e556cc6028c5e8e2b84d9fa5dd7c7f5" dependencies = [ - "indexmap 2.7.0", + "indexmap 2.7.1", "toml_datetime", "winnow 0.6.24", ] @@ -5462,9 +5462,9 @@ checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" [[package]] name = "unicode-ident" -version = "1.0.14" +version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "adb9e6ca4f869e1180728b7950e35922a7fc6397f7b641499e8f3ef06e50dc83" +checksum = "11cd88e12b17c6494200a9c1b683a04fcac9573ed74cd1b62aeb2727c5592243" [[package]] name = "unicode-linebreak" @@ -5528,15 +5528,15 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "uuid" -version = "1.12.0" +version = "1.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "744018581f9a3454a9e15beb8a33b017183f1e7c0cd170232a2d1453b23a51c4" +checksum = "b3758f5e68192bb96cc8f9b7e2c2cfdabb435499a28499a42f8f984092adad4b" [[package]] name = "valuable" -version = "0.1.0" +version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" +checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" [[package]] name = "vec-collections" diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 9b0172853c0..ecb4ce751cf 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -141,6 +141,12 @@ pub struct CompileOptions { #[arg(long)] pub skip_brillig_constraints_check: bool, + /// Flag to turn on the lookback feature of the Brillig call constraints + /// check, allowing tracking argument values before the call happens + /// preventing certain false negatives (leads to a slowdown on large rollout functions) + #[arg(long)] + pub enable_brillig_constraints_check_lookback: bool, + /// Setting to decide on an inlining strategy for Brillig functions. /// A more aggressive inliner should generate larger programs but more optimized /// A less aggressive inliner should generate smaller programs @@ -680,6 +686,8 @@ pub fn compile_no_check( emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None }, skip_underconstrained_check: options.skip_underconstrained_check, skip_brillig_constraints_check: options.skip_brillig_constraints_check, + enable_brillig_constraints_check_lookback: options + .enable_brillig_constraints_check_lookback, inliner_aggressiveness: options.inliner_aggressiveness, max_bytecode_increase_percent: options.max_bytecode_increase_percent, }; diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 94c0e0554a4..578f0ddb240 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -122,7 +122,7 @@ pub enum InternalWarning { pub enum InternalBug { #[error("Input to Brillig function is in a separate subgraph to output")] IndependentSubgraph { call_stack: CallStack }, - #[error("Brillig function call isn't properly covered by a manual constraint")] + #[error("Brillig function call isn't properly covered by a manual constraint (in space of 1000 instructions)")] UncheckedBrilligCall { call_stack: CallStack }, #[error("Assertion is always false")] AssertFailed { call_stack: CallStack }, diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 4cefce1d647..9fbac8192fd 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -71,6 +71,11 @@ pub struct SsaEvaluatorOptions { /// Skip the missing Brillig call constraints check pub skip_brillig_constraints_check: bool, + /// Enable the lookback feature of the Brillig call constraints + /// check (potentially discovers more bugs, leads to a slowdown + /// on large rollout functions) + pub enable_brillig_constraints_check_lookback: bool, + /// The higher the value, the more inlined Brillig functions will be. pub inliner_aggressiveness: i64, @@ -116,7 +121,11 @@ pub(crate) fn optimize_into_acir( ssa_level_warnings.extend(time( "After Check for Missing Brillig Call Constraints", options.print_codegen_timings, - || ssa.check_for_missing_brillig_constraints(), + || { + ssa.check_for_missing_brillig_constraints( + options.enable_brillig_constraints_check_lookback, + ) + }, )); }; diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 24ad67c3b69..99621dc7a13 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -42,7 +42,10 @@ impl Ssa { /// Detect Brillig calls left unconstrained with manual asserts /// and return a vector of bug reports if any have been found - pub(crate) fn check_for_missing_brillig_constraints(&mut self) -> Vec { + pub(crate) fn check_for_missing_brillig_constraints( + &mut self, + enable_lookback: bool, + ) -> Vec { // Skip the check if there are no Brillig functions involved if !self.functions.values().any(|func| func.runtime().is_brillig()) { return vec![]; @@ -56,7 +59,10 @@ impl Ssa { let function_to_process = &self.functions[&fid]; match function_to_process.runtime() { RuntimeType::Acir { .. } => { - let mut context = DependencyContext::default(); + let mut context = DependencyContext { + enable_lookback, + ..Default::default() + }; context.build(function_to_process, &self.functions); context.collect_warnings(function_to_process) } @@ -117,6 +123,11 @@ struct DependencyContext { // Map of block indices to Brillig call ids that should not be // followed after meeting them search_limits: HashMap, + // Opt-in to use the lookback feature (tracking the argument values + // of a Brillig call before the call happens if their usage precedes + // it). Can prevent certain false negatives, at the cost of + // slowing down checking large functions considerably + enable_lookback: bool, } /// Structure keeping track of value ids descending from Brillig calls' @@ -375,15 +386,17 @@ impl DependencyContext { } }); - // Start tracking calls when their argument value ids first appear, - // or when their instruction id comes up (in case there were - // no non-constant arguments) - for argument in &arguments { - if let Some(calls) = self.call_arguments.get(argument) { - for call in calls { - if let Some(tainted_ids) = self.tainted.get_mut(call) { - tainted_ids.tracking = true; - self.tracking_count += 1; + // If the lookback feature is enabled, start tracking calls when + // their argument value ids first appear, or when their + // instruction id comes up (in case there were no non-constant arguments) + if self.enable_lookback { + for argument in &arguments { + if let Some(calls) = self.call_arguments.get(argument) { + for call in calls { + if let Some(tainted_ids) = self.tainted.get_mut(call) { + tainted_ids.tracking = true; + self.tracking_count += 1; + } } } } @@ -1034,7 +1047,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 1); } @@ -1065,7 +1078,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 1); } @@ -1095,7 +1108,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 0); } @@ -1124,7 +1137,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 0); } @@ -1179,7 +1192,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 1); } @@ -1208,7 +1221,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 2); } @@ -1240,7 +1253,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 0); } @@ -1267,7 +1280,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 1); } @@ -1296,7 +1309,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 0); } @@ -1367,7 +1380,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(false); assert_eq!(ssa_level_warnings.len(), 0); } @@ -1393,7 +1406,7 @@ mod test { "#; let mut ssa = Ssa::from_str(program).unwrap(); - let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(); + let ssa_level_warnings = ssa.check_for_missing_brillig_constraints(true); assert_eq!(ssa_level_warnings.len(), 0); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/hint.rs b/compiler/noirc_evaluator/src/ssa/opt/hint.rs index 1326c2cc010..9730a03082b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/hint.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/hint.rs @@ -19,6 +19,7 @@ mod tests { emit_ssa: None, skip_underconstrained_check: true, skip_brillig_constraints_check: true, + enable_brillig_constraints_check_lookback: false, inliner_aggressiveness: 0, max_bytecode_increase_percent: None, }; From a29a480581aacee25d3eb84f24cb749b692dccd3 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut Date: Sat, 25 Jan 2025 06:17:23 +0300 Subject: [PATCH 02/14] Fix comments --- compiler/noirc_driver/src/lib.rs | 4 ++-- compiler/noirc_evaluator/src/ssa.rs | 2 +- .../src/ssa/checks/check_for_underconstrained_values.rs | 8 +++----- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index ecb4ce751cf..5baf22870ad 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -142,8 +142,8 @@ pub struct CompileOptions { pub skip_brillig_constraints_check: bool, /// Flag to turn on the lookback feature of the Brillig call constraints - /// check, allowing tracking argument values before the call happens - /// preventing certain false negatives (leads to a slowdown on large rollout functions) + /// check, allowing tracking argument values before the call happens preventing + /// certain rare false positives (leads to a slowdown on large rollout functions) #[arg(long)] pub enable_brillig_constraints_check_lookback: bool, diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 9fbac8192fd..8da8bfe1f2b 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -72,7 +72,7 @@ pub struct SsaEvaluatorOptions { pub skip_brillig_constraints_check: bool, /// Enable the lookback feature of the Brillig call constraints - /// check (potentially discovers more bugs, leads to a slowdown + /// check (prevents some rare false positives, leads to a slowdown /// on large rollout functions) pub enable_brillig_constraints_check_lookback: bool, diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 99621dc7a13..e4efddd5b17 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -59,10 +59,8 @@ impl Ssa { let function_to_process = &self.functions[&fid]; match function_to_process.runtime() { RuntimeType::Acir { .. } => { - let mut context = DependencyContext { - enable_lookback, - ..Default::default() - }; + let mut context = + DependencyContext { enable_lookback, ..Default::default() }; context.build(function_to_process, &self.functions); context.collect_warnings(function_to_process) } @@ -125,7 +123,7 @@ struct DependencyContext { search_limits: HashMap, // Opt-in to use the lookback feature (tracking the argument values // of a Brillig call before the call happens if their usage precedes - // it). Can prevent certain false negatives, at the cost of + // it). Can prevent certain false positives, at the cost of // slowing down checking large functions considerably enable_lookback: bool, } From 50c2ef28a31414589c5096a4188dd5fe013d92ba Mon Sep 17 00:00:00 2001 From: Ratmir Karabut Date: Tue, 4 Feb 2025 06:52:47 +0300 Subject: [PATCH 03/14] Skip visited locations --- .../check_for_underconstrained_values.rs | 77 +++++++++++-------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index e4efddd5b17..6ab16a41770 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -8,6 +8,7 @@ use crate::ssa::ir::function::{Function, FunctionId}; use crate::ssa::ir::instruction::{Hint, Instruction, InstructionId, Intrinsic}; use crate::ssa::ir::value::{Value, ValueId}; use crate::ssa::ssa_gen::Ssa; +use noirc_errors::Location; use im::HashMap; use rayon::prelude::*; use std::collections::{BTreeMap, BTreeSet, HashSet}; @@ -126,6 +127,9 @@ struct DependencyContext { // it). Can prevent certain false positives, at the cost of // slowing down checking large functions considerably enable_lookback: bool, + // Code locations of brillig calls already visited (we don't + // need to recheck calls happening in the same unrolled functions) + visited_locations: HashSet, } /// Structure keeping track of value ids descending from Brillig calls' @@ -333,40 +337,49 @@ impl DependencyContext { if let Instruction::Call { func, arguments } = &function.dfg[*instruction] { if let Value::Function(callee) = &function.dfg[*func] { if all_functions[&callee].runtime().is_brillig() { - let results = function.dfg.instruction_results(*instruction); - let current_tainted = - BrilligTaintedIds::new(function, arguments, results); - - // Record arguments/results for each Brillig call for the check. - // - // Do not track Brillig calls acting as simple wrappers over - // another registered Brillig call, update the tainted sets of - // the wrapped call instead - let mut wrapped_call_found = false; - for (_, tainted_call) in self.tainted.iter_mut() { - if current_tainted.is_wrapper(tainted_call) { - tainted_call.update_results_children(results); - wrapped_call_found = true; - break; + + // Skip already visited locations (happens often in unrolled functions) + let call_stack = function.dfg.get_instruction_call_stack(*instruction); + if let Some(location) = call_stack.last() { + if !self.visited_locations.contains(location) { + let results = function.dfg.instruction_results(*instruction); + let current_tainted = + BrilligTaintedIds::new(function, arguments, results); + + // Record arguments/results for each Brillig call for the check. + // + // Do not track Brillig calls acting as simple wrappers over + // another registered Brillig call, update the tainted sets of + // the wrapped call instead + let mut wrapped_call_found = false; + for (_, tainted_call) in self.tainted.iter_mut() { + if current_tainted.is_wrapper(tainted_call) { + tainted_call.update_results_children(results); + wrapped_call_found = true; + break; + } + } + + if !wrapped_call_found { + // Record the current call, remember the argument values involved + self.tainted.insert(*instruction, current_tainted); + arguments.iter().for_each(|value| { + self.call_arguments + .entry(*value) + .or_default() + .push(*instruction); + }); + + // Set the constraint search limit for the call + self.search_limits.insert( + block_index + BRILLIG_CONSTRAINT_SEARCH_DEPTH, + *instruction, + ); + } + + self.visited_locations.insert(*location); } } - - if !wrapped_call_found { - // Record the current call, remember the argument values involved - self.tainted.insert(*instruction, current_tainted); - arguments.iter().for_each(|value| { - self.call_arguments - .entry(*value) - .or_default() - .push(*instruction); - }); - - // Set the constraint search limit for the call - self.search_limits.insert( - block_index + BRILLIG_CONSTRAINT_SEARCH_DEPTH, - *instruction, - ); - } } } } From ef8989be168947d59f7a759d20731d1bc089f223 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut Date: Tue, 4 Feb 2025 07:07:58 +0300 Subject: [PATCH 04/14] cargo fmt --- .../src/ssa/checks/check_for_underconstrained_values.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 6ab16a41770..af175da90d6 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -8,8 +8,8 @@ use crate::ssa::ir::function::{Function, FunctionId}; use crate::ssa::ir::instruction::{Hint, Instruction, InstructionId, Intrinsic}; use crate::ssa::ir::value::{Value, ValueId}; use crate::ssa::ssa_gen::Ssa; -use noirc_errors::Location; use im::HashMap; +use noirc_errors::Location; use rayon::prelude::*; use std::collections::{BTreeMap, BTreeSet, HashSet}; use tracing::trace; @@ -337,7 +337,6 @@ impl DependencyContext { if let Instruction::Call { func, arguments } = &function.dfg[*instruction] { if let Value::Function(callee) = &function.dfg[*func] { if all_functions[&callee].runtime().is_brillig() { - // Skip already visited locations (happens often in unrolled functions) let call_stack = function.dfg.get_instruction_call_stack(*instruction); if let Some(location) = call_stack.last() { @@ -376,7 +375,7 @@ impl DependencyContext { *instruction, ); } - + self.visited_locations.insert(*location); } } From dfec078a846d175c0a09e7db6233e256d8c8e2a3 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut Date: Tue, 4 Feb 2025 07:39:50 +0300 Subject: [PATCH 05/14] Fix for tests --- .../check_for_underconstrained_values.rs | 76 ++++++++++--------- 1 file changed, 42 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index af175da90d6..f8b9b5a5f5b 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -339,43 +339,51 @@ impl DependencyContext { if all_functions[&callee].runtime().is_brillig() { // Skip already visited locations (happens often in unrolled functions) let call_stack = function.dfg.get_instruction_call_stack(*instruction); - if let Some(location) = call_stack.last() { - if !self.visited_locations.contains(location) { - let results = function.dfg.instruction_results(*instruction); - let current_tainted = - BrilligTaintedIds::new(function, arguments, results); - - // Record arguments/results for each Brillig call for the check. - // - // Do not track Brillig calls acting as simple wrappers over - // another registered Brillig call, update the tainted sets of - // the wrapped call instead - let mut wrapped_call_found = false; - for (_, tainted_call) in self.tainted.iter_mut() { - if current_tainted.is_wrapper(tainted_call) { - tainted_call.update_results_children(results); - wrapped_call_found = true; - break; - } - } + let location = call_stack.last(); + + // If there is no call stack (happens for tests), consider unvisited + let mut visited = false; + if let Some(location) = location { + visited = self.visited_locations.contains(location); + } - if !wrapped_call_found { - // Record the current call, remember the argument values involved - self.tainted.insert(*instruction, current_tainted); - arguments.iter().for_each(|value| { - self.call_arguments - .entry(*value) - .or_default() - .push(*instruction); - }); - - // Set the constraint search limit for the call - self.search_limits.insert( - block_index + BRILLIG_CONSTRAINT_SEARCH_DEPTH, - *instruction, - ); + if !visited { + let results = function.dfg.instruction_results(*instruction); + let current_tainted = + BrilligTaintedIds::new(function, arguments, results); + + // Record arguments/results for each Brillig call for the check. + // + // Do not track Brillig calls acting as simple wrappers over + // another registered Brillig call, update the tainted sets of + // the wrapped call instead + let mut wrapped_call_found = false; + for (_, tainted_call) in self.tainted.iter_mut() { + if current_tainted.is_wrapper(tainted_call) { + tainted_call.update_results_children(results); + wrapped_call_found = true; + break; } + } + + if !wrapped_call_found { + // Record the current call, remember the argument values involved + self.tainted.insert(*instruction, current_tainted); + arguments.iter().for_each(|value| { + self.call_arguments + .entry(*value) + .or_default() + .push(*instruction); + }); + + // Set the constraint search limit for the call + self.search_limits.insert( + block_index + BRILLIG_CONSTRAINT_SEARCH_DEPTH, + *instruction, + ); + } + if let Some(location) = location { self.visited_locations.insert(*location); } } From 136fa47d2033efeed50e4516af03c28978cc4522 Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 13:34:14 +0000 Subject: [PATCH 06/14] Fix constraining requirements, effectively remove depth limit --- .../src/ssa/checks/check_for_underconstrained_values.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 040357eccb7..e21c82bc04a 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -16,7 +16,7 @@ use tracing::trace; /// The number of instructions that have to be passed to stop /// following a Brillig call, with assumption it wouldn't get constrained -const BRILLIG_CONSTRAINT_SEARCH_DEPTH: usize = 1000; +const BRILLIG_CONSTRAINT_SEARCH_DEPTH: usize = 10_000_000; impl Ssa { /// This function provides an SSA pass that detects if the final function has any subgraphs independent from inputs and outputs. @@ -276,12 +276,11 @@ impl BrilligTaintedIds { } // Along with it, one of the argument descendants should be constrained - // (skipped if there were no arguments, or if an actual result and not a - // descendant has been constrained _alone_, e.g. against a constant) + // (skipped if there were no arguments, or if a result descendant + // has been constrained _alone_, e.g. against a constant) if !results_involved.is_empty() && (self.arguments.is_empty() - || (constrained_values.len() == 1 - && self.root_results.intersection(constrained_values).next().is_some()) + || (constrained_values.len() == 1) || self.arguments.intersection(constrained_values).next().is_some()) { // Remember the partial constraint, clearing the sets From 2252dc36536cecd3c377c7aae23123aa54e47fd6 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut Date: Tue, 11 Feb 2025 18:04:17 +0300 Subject: [PATCH 07/14] Update compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs Co-authored-by: Akosh Farkash --- .../src/ssa/checks/check_for_underconstrained_values.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 4fc733fddd0..92d736c32f6 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -341,9 +341,7 @@ impl DependencyContext { let location = call_stack.last(); // If there is no call stack (happens for tests), consider unvisited - let mut visited = false; - if let Some(location) = location { - visited = self.visited_locations.contains(location); + let visited = location.map(|loc| self.visited_locations.contains(loc)).unwrap_or_default(); } if !visited { From e942ee24591a4689a94578f9e164c73851c267fb Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 15:08:12 +0000 Subject: [PATCH 08/14] Fix error message, syntax --- compiler/noirc_evaluator/src/errors.rs | 2 +- .../src/ssa/checks/check_for_underconstrained_values.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 578f0ddb240..94c0e0554a4 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -122,7 +122,7 @@ pub enum InternalWarning { pub enum InternalBug { #[error("Input to Brillig function is in a separate subgraph to output")] IndependentSubgraph { call_stack: CallStack }, - #[error("Brillig function call isn't properly covered by a manual constraint (in space of 1000 instructions)")] + #[error("Brillig function call isn't properly covered by a manual constraint")] UncheckedBrilligCall { call_stack: CallStack }, #[error("Assertion is always false")] AssertFailed { call_stack: CallStack }, diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 92d736c32f6..01cce641d7b 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -341,8 +341,9 @@ impl DependencyContext { let location = call_stack.last(); // If there is no call stack (happens for tests), consider unvisited - let visited = location.map(|loc| self.visited_locations.contains(loc)).unwrap_or_default(); - } + let visited = location + .map(|loc| self.visited_locations.contains(loc)) + .unwrap_or_default(); if !visited { let results = function.dfg.instruction_results(*instruction); From 761cdd57d48b55b739cae1ab2f07df7fc41331d9 Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 15:45:18 +0000 Subject: [PATCH 09/14] Remove search limiting --- .../check_for_underconstrained_values.rs | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 01cce641d7b..d9c05546f35 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -14,10 +14,6 @@ use rayon::prelude::*; use std::collections::{BTreeMap, BTreeSet, HashSet}; use tracing::trace; -/// The number of instructions that have to be passed to stop -/// following a Brillig call, with assumption it wouldn't get constrained -const BRILLIG_CONSTRAINT_SEARCH_DEPTH: usize = 10_000_000; - impl Ssa { /// This function provides an SSA pass that detects if the final function has any subgraphs independent from inputs and outputs. /// If this is the case, then part of the final circuit can be completely replaced by any other passing circuit, since there are no constraints ensuring connections. @@ -119,9 +115,6 @@ struct DependencyContext { call_arguments: HashMap>, // Maintains count of calls being tracked tracking_count: usize, - // Map of block indices to Brillig call ids that should not be - // followed after meeting them - search_limits: HashMap, // Opt-in to use the lookback feature (tracking the argument values // of a Brillig call before the call happens if their usage precedes // it). Can prevent certain false positives, at the cost of @@ -373,12 +366,6 @@ impl DependencyContext { .or_default() .push(*instruction); }); - - // Set the constraint search limit for the call - self.search_limits.insert( - block_index + BRILLIG_CONSTRAINT_SEARCH_DEPTH, - *instruction, - ); } if let Some(location) = location { @@ -422,14 +409,6 @@ impl DependencyContext { self.tracking_count += 1; } - // Stop tracking calls when their search limit is hit - if let Some(call) = self.search_limits.get(&block_index) { - if let Some(tainted_ids) = self.tainted.get_mut(call) { - tainted_ids.tracking = false; - self.tracking_count -= 1; - } - } - // We can skip over instructions while nothing is being tracked if self.tracking_count > 0 { let mut results = Vec::new(); From a42e96b6c9d6edd039c2ea71d0454b4c1c66b1ce Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 15:52:47 +0000 Subject: [PATCH 10/14] Update Cargo.lock --- Cargo.lock | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index deb3eecc23c..f5a360067ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -719,9 +719,9 @@ dependencies = [ [[package]] name = "blake2b_simd" -version = "1.0.2" +version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "23285ad32269793932e830392f2fe2f83e26488fd3ec778883a93c8323735780" +checksum = "06e903a20b159e944f91ec8499fe1e55651480c541ea0a584f5d967c49ad9d99" dependencies = [ "arrayref", "arrayvec", @@ -854,9 +854,9 @@ checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" [[package]] name = "cc" -version = "1.2.11" +version = "1.2.13" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e4730490333d58093109dc02c23174c3f4d490998c3fed3cc8e82d57afedb9cf" +checksum = "c7777341816418c02e033934a09f20dc0ccaf65a5201ef8a450ae0105a573fda" dependencies = [ "shlex", ] @@ -2639,7 +2639,7 @@ dependencies = [ "jsonrpsee-types", "parking_lot", "rand", - "rustc-hash 2.1.0", + "rustc-hash 2.1.1", "serde", "serde_json", "thiserror", @@ -3595,9 +3595,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.20.2" +version = "1.20.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1261fe7e33c73b354eab43b1273a57c8f967d0391e80353e51f764ac02cf6775" +checksum = "945462a4b81e43c4e3ba96bd7b49d834c6f61198356aa858733bc4acf3cbe62e" [[package]] name = "oorandom" @@ -3937,7 +3937,7 @@ version = "3.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ecf48c7ca261d60b74ab1a7b20da18bede46776b2e55535cb958eb595c5fa7b" dependencies = [ - "toml_edit 0.22.23", + "toml_edit 0.22.24", ] [[package]] @@ -4281,9 +4281,9 @@ checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" [[package]] name = "rustc-hash" -version = "2.1.0" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c7fb8039b3032c191086b10f11f319a6e99e1e82889c5cc6046f515c9db1d497" +checksum = "357703d41365b4b27c590e3ed91eabb1b663f07c4c084095e60cbed4362dff0d" [[package]] name = "rustc_version" @@ -4935,9 +4935,9 @@ checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" [[package]] name = "symbolic-common" -version = "12.13.3" +version = "12.13.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "13a4dfe4bbeef59c1f32fc7524ae7c95b9e1de5e79a43ce1604e181081d71b0c" +checksum = "b6189977df1d6ec30c920647919d76f29fb8d8f25e8952e835b0fcda25e8f792" dependencies = [ "debugid", "memmap2", @@ -4947,9 +4947,9 @@ dependencies = [ [[package]] name = "symbolic-demangle" -version = "12.13.3" +version = "12.13.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "98cf6a95abff97de4d7ff3473f33cacd38f1ddccad5c1feab435d6760300e3b6" +checksum = "d234917f7986498e7f62061438cee724bafb483fe84cfbe2486f68dce48240d7" dependencies = [ "cpp_demangle", "rustc-demangle", @@ -5281,13 +5281,13 @@ dependencies = [ [[package]] name = "toml_edit" -version = "0.22.23" +version = "0.22.24" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "02a8b472d1a3d7c18e2d61a489aee3453fd9031c33e4f55bd533f4a7adca1bee" +checksum = "17b4795ff5edd201c7cd6dca065ae59972ce77d1b80fa0a84d94950ece7d1474" dependencies = [ "indexmap 2.7.1", "toml_datetime", - "winnow 0.7.1", + "winnow 0.7.2", ] [[package]] @@ -5543,9 +5543,9 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "uuid" -version = "1.12.1" +version = "1.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b3758f5e68192bb96cc8f9b7e2c2cfdabb435499a28499a42f8f984092adad4b" +checksum = "ced87ca4be083373936a67f8de945faa23b6b42384bd5b64434850802c6dccd0" [[package]] name = "valuable" @@ -5937,9 +5937,9 @@ dependencies = [ [[package]] name = "winnow" -version = "0.7.1" +version = "0.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "86e376c75f4f43f44db463cf729e0d3acbf954d13e22c51e26e4c264b4ab545f" +checksum = "59690dea168f2198d1a3b0cac23b8063efcd11012f10ae4698f284808c8ef603" dependencies = [ "memchr", ] From 10aa9d2e35e0d68e1d952820cdee50b3a184976f Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 15:59:53 +0000 Subject: [PATCH 11/14] clippy --- .../check_for_underconstrained_values.rs | 92 +++++++++---------- 1 file changed, 45 insertions(+), 47 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index d9c05546f35..3820f4c68dd 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -324,62 +324,60 @@ impl DependencyContext { // First, gather information on all Brillig calls in the block // to be able to follow their arguments first appearing in the // flow graph before the calls themselves - function.dfg[block].instructions().iter().enumerate().for_each( - |(block_index, instruction)| { - if let Instruction::Call { func, arguments } = &function.dfg[*instruction] { - if let Value::Function(callee) = &function.dfg[*func] { - if all_functions[&callee].runtime().is_brillig() { - // Skip already visited locations (happens often in unrolled functions) - let call_stack = function.dfg.get_instruction_call_stack(*instruction); - let location = call_stack.last(); - - // If there is no call stack (happens for tests), consider unvisited - let visited = location - .map(|loc| self.visited_locations.contains(loc)) - .unwrap_or_default(); - - if !visited { - let results = function.dfg.instruction_results(*instruction); - let current_tainted = - BrilligTaintedIds::new(function, arguments, results); - - // Record arguments/results for each Brillig call for the check. - // - // Do not track Brillig calls acting as simple wrappers over - // another registered Brillig call, update the tainted sets of - // the wrapped call instead - let mut wrapped_call_found = false; - for (_, tainted_call) in self.tainted.iter_mut() { - if current_tainted.is_wrapper(tainted_call) { - tainted_call.update_results_children(results); - wrapped_call_found = true; - break; - } + function.dfg[block].instructions().iter().for_each(|instruction| { + if let Instruction::Call { func, arguments } = &function.dfg[*instruction] { + if let Value::Function(callee) = &function.dfg[*func] { + if all_functions[&callee].runtime().is_brillig() { + // Skip already visited locations (happens often in unrolled functions) + let call_stack = function.dfg.get_instruction_call_stack(*instruction); + let location = call_stack.last(); + + // If there is no call stack (happens for tests), consider unvisited + let visited = location + .map(|loc| self.visited_locations.contains(loc)) + .unwrap_or_default(); + + if !visited { + let results = function.dfg.instruction_results(*instruction); + let current_tainted = + BrilligTaintedIds::new(function, arguments, results); + + // Record arguments/results for each Brillig call for the check. + // + // Do not track Brillig calls acting as simple wrappers over + // another registered Brillig call, update the tainted sets of + // the wrapped call instead + let mut wrapped_call_found = false; + for (_, tainted_call) in self.tainted.iter_mut() { + if current_tainted.is_wrapper(tainted_call) { + tainted_call.update_results_children(results); + wrapped_call_found = true; + break; } + } - if !wrapped_call_found { - // Record the current call, remember the argument values involved - self.tainted.insert(*instruction, current_tainted); - arguments.iter().for_each(|value| { - self.call_arguments - .entry(*value) - .or_default() - .push(*instruction); - }); - } + if !wrapped_call_found { + // Record the current call, remember the argument values involved + self.tainted.insert(*instruction, current_tainted); + arguments.iter().for_each(|value| { + self.call_arguments + .entry(*value) + .or_default() + .push(*instruction); + }); + } - if let Some(location) = location { - self.visited_locations.insert(*location); - } + if let Some(location) = location { + self.visited_locations.insert(*location); } } } } - }, - ); + } + }); //Then, go over the instructions - for (block_index, instruction) in function.dfg[block].instructions().iter().enumerate() { + for instruction in function.dfg[block].instructions().iter() { let mut arguments = Vec::new(); // Collect non-constant instruction arguments From 4691618a7c0ea9b0108a5acc0c28a542b401bf81 Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 16:39:09 +0000 Subject: [PATCH 12/14] Reduce tracking count when calls are found constrained --- .../src/ssa/checks/check_for_underconstrained_values.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 3820f4c68dd..3961ddad4c9 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -604,7 +604,14 @@ impl DependencyContext { } } - self.tainted.retain(|_, tainted_ids| !tainted_ids.check_constrained()); + self.tainted.retain(|_, tainted_ids| { + if tainted_ids.check_constrained() { + self.tracking_count -= 1; + false + } else { + true + } + }); } /// Process ArrayGet instruction for tracked Brillig calls From 4b36a38a07bc72e9dd073d50eaf589c8a8d291eb Mon Sep 17 00:00:00 2001 From: rkarabut Date: Tue, 11 Feb 2025 17:16:36 +0000 Subject: [PATCH 13/14] Remove extra tracking count increase --- .../src/ssa/checks/check_for_underconstrained_values.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 3961ddad4c9..cb8b5b4fe6c 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -403,8 +403,10 @@ impl DependencyContext { } } if let Some(tainted_ids) = self.tainted.get_mut(instruction) { - tainted_ids.tracking = true; - self.tracking_count += 1; + if !tainted_ids.tracking { + tainted_ids.tracking = true; + self.tracking_count += 1; + } } // We can skip over instructions while nothing is being tracked From 888726be87518c201a0d80f936e56512685fee58 Mon Sep 17 00:00:00 2001 From: rkarabut Date: Wed, 12 Feb 2025 04:38:09 +0000 Subject: [PATCH 14/14] Change locations key to be (callee, loc) due to #6965 --- .../src/ssa/checks/check_for_underconstrained_values.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index cb8b5b4fe6c..3718956a93a 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -122,7 +122,7 @@ struct DependencyContext { enable_lookback: bool, // Code locations of brillig calls already visited (we don't // need to recheck calls happening in the same unrolled functions) - visited_locations: HashSet, + visited_locations: HashSet<(FunctionId, Location)>, } /// Structure keeping track of value ids descending from Brillig calls' @@ -334,7 +334,7 @@ impl DependencyContext { // If there is no call stack (happens for tests), consider unvisited let visited = location - .map(|loc| self.visited_locations.contains(loc)) + .map(|loc| self.visited_locations.contains(&(*callee, *loc))) .unwrap_or_default(); if !visited { @@ -368,7 +368,7 @@ impl DependencyContext { } if let Some(location) = location { - self.visited_locations.insert(*location); + self.visited_locations.insert((*callee, *location)); } } }