fix(frontend): Reject calling an oracle through a global variable in a constrained function

compiler/noirc_evaluator/src/brillig/brillig_check.rs

@@ -126,9 +126,7 @@ pub(crate) fn opcode_advisories<F: AcirField>(
         // Going backwards, so reads at the end are recorded before earlier writes.
         for loc in opcode_range.rev() {
             let opcode = &brillig_context.artifact().byte_code[loc];
-            if advisory_collector.should_visit_opcode(opcode) {
-                advisory_collector.visit_opcode(opcode, loc);
-            }
+            advisory_collector.visit_opcode(opcode, loc);
         }
 
         advisories.extend(advisory_collector.into_advisories());
@@ -432,8 +430,9 @@ trait OpcodeAddressVisitor {
     /// Called with all addresses written by the opcode.
     fn write(&mut self, addr: &MemoryAddress, location: OpcodeLocation);
 
-    /// Expected to be called with each opcode, traversing blocks in Post Order,
-    /// feeding the opcodes back to front.
+    /// Call with each opcode to record reads and writes.
+    ///
+    /// The expected order of visit depends on the purpose of the visitor implementation.
     fn visit_opcode<F: AcirField>(&mut self, opcode: &Opcode<F>, location: OpcodeLocation) {
         if !self.should_visit_opcode(opcode) {
             return;

compiler/noirc_evaluator/src/errors.rs

@@ -13,7 +13,7 @@ use noirc_errors::{CustomDiagnostic, Location, call_stack::CallStack};
 use noirc_frontend::signed_field::SignedField;
 use thiserror::Error;
 
-use crate::ssa::ir::types::NumericType;
+use crate::ssa::{ir::types::NumericType, ssa_gen::SHOW_INVALID_SSA_ENV_KEY};
 use serde::{Deserialize, Serialize};
 
 pub type RtResult<T> = Result<T, RuntimeError>;
@@ -240,11 +240,15 @@ impl RuntimeError {
                     call_stack.last().cloned().unwrap_or_else(Location::dummy);
 
                 let mut diagnostic = CustomDiagnostic::simple_error(
-                    message,
+                    format!("SSA validation error: {message}"),
                     String::new(),
                     location,
                 );
 
+                if std::env::var(SHOW_INVALID_SSA_ENV_KEY).is_err() {
+                    diagnostic.notes.push(format!("Set the {SHOW_INVALID_SSA_ENV_KEY} env var to see the SSA."));
+                }
+
                 if call_stack.is_empty() {
                     // Clear it otherwise it points to the top of the file.
                     diagnostic.secondaries.clear();

compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

@@ -2156,13 +2156,13 @@ mod test {
         // The arguments are not meant to make sense, just pass SSA validation and not be simplified out.
         let call_target = match test_call {
             TestCall::Function(_) => "f1".to_string(),
-            TestCall::ForeignFunction => "print".to_string(), // The ony foreign function the SSA parser allows.
+            TestCall::ForeignFunction => "print".to_string(),
             TestCall::Intrinsic(intrinsic) => format!("{intrinsic}"),
         };
 
         let src = format!(
             r#"
-        acir(inline) fn main f0 {{
+        brillig(inline) fn main f0 {{
           b0(v0: [u64; 25]):
             jmp b1(u32 0)
           b1(v1: u32):
@@ -2176,7 +2176,7 @@ mod test {
             return
         }}
 
-        acir(inline) {dummy_purity} fn dummy f1 {{
+        brillig(inline) {dummy_purity} fn dummy f1 {{
           b0(v0: [u64; 25]):
             return v0
         }}

compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

@@ -1846,8 +1846,6 @@ mod tests {
             v3 = eq v0, u128 {0}
             jmpif v3 then: b3, else: b2
           b2():
-            v2 = make_array b""
-            call print(u1 1, v0, v2, u1 0)
             v6 = unchecked_add v0, u128 1
             jmp b1(v6)
           b3():

compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs

@@ -606,8 +606,9 @@ impl Translator {
             return Ok(var_id);
         }
 
-        // We allow calls to the built-in print function
-        if &function.name == "print" {
+        // We allow calls to the built-in print function, or a function that is named as some kind of "oracle",
+        // which is a common pattern in the codebase and allows us to write tests with foreign functions in the SSA.
+        if &function.name == "print" || function.name.contains("oracle") {
             return Ok(self.builder.import_foreign_function(&function.name));
         }
 

compiler/noirc_evaluator/src/ssa/parser/tests.rs

@@ -808,6 +808,18 @@ fn test_parses_print() {
     assert_ssa_roundtrip(src);
 }
 
+#[test]
+fn test_parses_oracle() {
+    let src = "
+        brillig(inline) impure fn main f0 {
+          b0():
+            call oracle_call()
+            return
+        }
+        ";
+    assert_ssa_roundtrip(src);
+}
+
 #[test]
 fn parses_variable_from_a_syntactically_following_block_but_logically_preceding_block_with_jmp() {
     let src = "

compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -42,6 +42,8 @@ use super::{
     },
 };
 
+pub(crate) const SHOW_INVALID_SSA_ENV_KEY: &str = "NOIR_SHOW_INVALID_SSA";
+
 pub(crate) const SSA_WORD_SIZE: u32 = 32;
 
 /// Generates SSA for the given monomorphized program.
@@ -147,7 +149,7 @@ fn validate_ssa_or_err(ssa: Ssa) -> Result<Ssa, RuntimeError> {
     if let Err(payload) = result {
         // Print the SSA, but it's potentially massive, and if we resume the unwind it might be displayed
         // under the panic message, which makes it difficult to see what went wrong.
-        if std::env::var("RUST_BACKTRACE").is_ok() {
+        if std::env::var(SHOW_INVALID_SSA_ENV_KEY).is_ok() {
             eprintln!("--- The SSA failed to validate:\n{ssa}\n");
         }
 

compiler/noirc_evaluator/src/ssa/validation/mod.rs

@@ -680,19 +680,19 @@
                 //     message_hash: [u8; N],
                 //     predicate: bool,
                 // ) -> bool
                 assert_arguments_length(arguments, 5, "ecdsa_secp_256");
 
                 let public_key_x = arguments[0];
                 let public_key_x_type = dfg.type_of_value(public_key_x);
                 let public_key_x_length =
                     assert_u8_array(&public_key_x_type, "ecdsa_secp256 public_key_x");
                 assert_array_length(public_key_x_length, 32, "ecdsa_secp256 public_key_x");
 
                 let public_key_y = arguments[1];
                 let public_key_y_type = dfg.type_of_value(public_key_y);
                 let public_key_y_length =
                     assert_u8_array(&public_key_y_type, "ecdsa_secp256 public_key_y");
                 assert_array_length(public_key_y_length, 32, "ecdsa_secp256 public_key_y");
 
                 let signature = arguments[2];
                 let signature_type = dfg.type_of_value(signature);
@@ -929,17 +929,26 @@
     /// Check the inputs and outputs of function calls going from ACIR to Brillig:
     /// * cannot pass references from constrained to unconstrained code
     /// * cannot return functions
+    /// * cannot call oracles directly
     fn check_calls_in_constrained(&self, instruction: InstructionId) {
         if !self.function.runtime().is_acir() {
             return;
         }
         let Instruction::Call { func, arguments } = &self.function.dfg[instruction] else {
             return;
         };
-        let Value::Function(func_id) = &self.function.dfg[*func] else {
-            return;
+        let callee_id = match &self.function.dfg[*func] {
+            Value::Function(func_id) => func_id,
+            Value::ForeignFunction(oracle) => {
+                panic!(
+                    "Trying to call foreign function '{oracle}' from ACIR function '{} {}'",
+                    self.function.name(),
+                    self.function.id()
+                );
+            }
+            _ => return,
         };
-        let called_function = &self.ssa.functions[func_id];
+        let called_function = &self.ssa.functions[callee_id];
         if called_function.runtime().is_acir() {
             return;
         }
@@ -1721,6 +1730,21 @@
         let _ = Ssa::from_str(src).unwrap();
     }
 
+    #[test]
+    #[should_panic(
+        expected = "Trying to call foreign function 'oracle_call' from ACIR function 'main f0'"
+    )]
+    fn disallows_calling_an_oracle_from_acir() {
+        let src = "
+        acir(inline) fn main f0 {
+          b0():
+            call oracle_call()
+            return
+        }
+        ";
+        let _ = Ssa::from_str(src).unwrap();
+    }
+
     #[test]
     #[should_panic(
         expected = "Trying to pass a reference from ACIR function 'main f0' to unconstrained 'foo f1' in argument v1: &mut u32"

compiler/noirc_frontend/src/elaborator/types.rs

@@ -2321,6 +2321,7 @@ impl Elaborator<'_> {
         let is_unconstrained_call =
             func_type_is_unconstrained || self.is_unconstrained_call(call.func);
         let crossing_runtime_boundary = is_current_func_constrained && is_unconstrained_call;
+
         if crossing_runtime_boundary {
             match self.unsafe_block_status {
                 UnsafeBlockStatus::NotInUnsafeBlock => {

compiler/noirc_frontend/src/monomorphization/proxies.rs

@@ -82,7 +82,11 @@ impl ProxyContext {
             };
 
             // Create a separate proxy for the constrained and unconstrained version.
-            pair.for_each(|ident, unconstrained| {
+            pair.for_each(|ident, mut unconstrained| {
+                // If we are calling an oracle, there is no reason to create an unconstrained proxy,
+                // since such a call would be rejected by the SSA validation.
+                unconstrained |= matches!(ident.definition, Definition::Oracle(_));
+
                 let key = (ident.definition.clone(), unconstrained);
 
                 let proxy_id = match self.replacements.get(&key) {
@@ -249,7 +253,10 @@ fn make_proxy(id: FuncId, ident: Ident, unconstrained: bool) -> Function {
 
 #[cfg(test)]
 mod tests {
-    use crate::test_utils::get_monomorphized_no_emit_test;
+    use crate::{
+        hir::{def_collector::dc_crate::CompilationError, resolution::errors::ResolverError},
+        test_utils::{get_monomorphized_no_emit_test, get_monomorphized_with_error_filter},
+    };
 
     #[test]
     fn creates_proxies_for_oracle() {
@@ -270,18 +277,59 @@ mod tests {
         let program = get_monomorphized_no_emit_test(src).unwrap();
         insta::assert_snapshot!(program, @r"
         unconstrained fn main$f0() -> () {
-            foo$f1((bar$f2, bar$f3));
+            foo$f1((bar$f2, bar$f2));
         }
         unconstrained fn foo$f1(f$l0: (fn(Field) -> (), unconstrained fn(Field) -> ())) -> () {
             f$l0.1(0);
         }
         #[inline_always]
-        fn bar_proxy$f2(p0$l0: Field) -> () {
+        unconstrained fn bar_proxy$f2(p0$l0: Field) -> () {
             bar$my_oracle(p0$l0)
         }
+        ");
+    }
+
+    #[test]
+    fn creates_proxies_for_builtin() {
+        let src = "
+        unconstrained fn main() {
+            foo(bar);
+        }
+
+        unconstrained fn foo(f: fn() -> bool) {
+          f();
+        }
+
+        #[builtin(is_unconstrained)]
+        pub fn bar() -> bool {
+        }
+        ";
+
+        let program = get_monomorphized_with_error_filter(src, |err| {
+            matches!(
+                err,
+                // Ignore the error about creating a builtin function.
+                CompilationError::ResolverError(
+                    ResolverError::LowLevelFunctionOutsideOfStdlib { .. }
+                )
+            )
+        })
+        .unwrap();
+
+        insta::assert_snapshot!(program, @r"
+        unconstrained fn main$f0() -> () {
+            foo$f1((bar$f2, bar$f3));
+        }
+        unconstrained fn foo$f1(f$l0: (fn() -> bool, unconstrained fn() -> bool)) -> () {
+            f$l0.1();
+        }
         #[inline_always]
-        unconstrained fn bar_proxy$f3(p0$l0: Field) -> () {
-            bar$my_oracle(p0$l0)
+        fn bar_proxy$f2() -> bool {
+            bar$is_unconstrained()
+        }
+        #[inline_always]
+        unconstrained fn bar_proxy$f3() -> bool {
+            bar$is_unconstrained()
         }
         ");
     }

compiler/noirc_frontend/src/node_interner/function.rs

@@ -7,6 +7,7 @@ use crate::{
     hir_def::{
         expr::{HirExpression, HirIdent},
         function::{FuncMeta, HirFunction},
+        stmt::{HirLetStatement, HirStatement},
     },
     node_interner::{
         DefinitionId, DefinitionKind, ExprId, FuncId, FunctionModifiers, Node, ReferenceId, TraitId,
@@ -109,14 +110,27 @@ impl NodeInterner {
         self.function_modules[&func]
     }
 
-    /// Returns the [`FuncId`] corresponding to the function referred to by `expr_id`
-    pub fn lookup_function_from_expr(&self, expr: &ExprId) -> Option<FuncId> {
+    /// Returns the [`FuncId`] corresponding to the function referred to by `expr_id`,
+    /// _iff_ it refers to an immutable (local or global) [HirExpression::Ident],
+    /// and it's statically known to point global function.
+    ///
+    /// Returns `None` for all other cases (tuples, array, mutable variables).
+    pub(crate) fn lookup_function_from_expr(&self, expr: &ExprId) -> Option<FuncId> {
         if let HirExpression::Ident(HirIdent { id, .. }, _) = self.expression(expr) {
             match self.try_definition(id).map(|def| &def.kind) {
                 Some(DefinitionKind::Function(func_id)) => Some(*func_id),
                 Some(DefinitionKind::Local(Some(expr_id))) => {
                     self.lookup_function_from_expr(expr_id)
                 }
+                Some(DefinitionKind::Global(global_id)) => {
+                    let info = self.get_global(*global_id);
+                    let HirStatement::Let(HirLetStatement { expression, .. }) =
+                        self.statement(&info.let_statement)
+                    else {
+                        unreachable!("global refers to a let statement");
+                    };
+                    self.lookup_function_from_expr(&expression)
+                }
                 _ => None,
             }
         } else {

compiler/noirc_frontend/src/test_utils.rs

@@ -31,7 +31,16 @@ pub fn get_monomorphized_no_emit_test(src: &str) -> Result<Program, Monomorphiza
 }
 
 pub fn get_monomorphized(src: &str) -> Result<Program, MonomorphizationError> {
+    get_monomorphized_with_error_filter(src, |_| false)
+}
+
+pub(crate) fn get_monomorphized_with_error_filter(
+    src: &str,
+    ignore_error: impl Fn(&CompilationError) -> bool,
+) -> Result<Program, MonomorphizationError> {
     let (_parsed_module, mut context, errors) = get_program(src);
+
+    let errors = errors.into_iter().filter(|e| !ignore_error(e)).collect::<Vec<_>>();
     assert!(
         errors.iter().all(|err| !err.is_error()),
         "Expected monomorphized program to have no errors before monomorphization, but found: {errors:?}"

compiler/noirc_frontend/src/tests/oracles.rs

@@ -34,3 +34,44 @@ fn errors_if_oracle_returns_multiple_vectors() {
     "#;
     check_errors(src);
 }
+
+#[test]
+fn errors_if_oracle_called_directly_from_constrained_via_local_var() {
+    let src = r#"
+    fn main() {
+        let oracle: unconstrained fn() = oracle_call;
+
+        // safety:
+        unsafe {
+            oracle();
+            ^^^^^^^^ Oracle functions cannot be called directly from constrained functions
+            ~~~~~~~~ This oracle call must be wrapped in a call to another unconstrained function before being returned to a constrained runtime
+        }
+    }
+
+    #[oracle(oracle_call)]
+    unconstrained fn oracle_call() {}
+    "#;
+    check_errors(src);
+}
+
+#[test]
+fn errors_if_oracle_called_directly_from_constrained_via_global_var() {
+    let src = r#"
+    global ORACLE: unconstrained fn() = oracle_call;
+
+    fn main() {
+
+        // safety:
+        unsafe {
+            ORACLE();
+            ^^^^^^^^ Oracle functions cannot be called directly from constrained functions
+            ~~~~~~~~ This oracle call must be wrapped in a call to another unconstrained function before being returned to a constrained runtime
+        }
+    }
+
+    #[oracle(oracle_call)]
+    unconstrained fn oracle_call() {}
+    "#;
+    check_errors(src);
+}

test_programs/compile_success_no_bug/regression_10298/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "regression_10298"
+type = "bin"
+authors = [""]
+
+[dependencies]