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

[aakoshh]

Description

Problem

Resolves #10298

Summary

Fixes the NodeInterner::lookup_function_from_expr method to handle DefinitionKind::Global by looking up the expression in the let statement that created the global, and recursively find out if it refers to a function.

This allows type_check_call to run the oracle_called_from_constrained_function and reject the call.

I added a rejection instead of trying to wrap the call in a proxy, because that's how the compiler deals with local variable pointing at an oracle (added a test for that too), but auto-wrapping calls from ACIR to oracles would be an option.

However, while looking at this I noticed that lookup_function_from_expr only handles DefinitionKind::Local(Some(expr_id)), but not DefinitionKind::Local(None); the latter is what we get for mutable variables, since their definition can change, it is not statically known. Therefore if we have a mutable function variable, then this lint does not kick in, and the panic still occurred, regardless of whether we went through a global or not.

I added an integration test to show this, and an SSA validation step to reject it, which is now shown without a panic. Also changed the env var to show the SSA from RUST_BACKTRACE to NOIR_SHOW_INVALID_SSA and added a hint to the error (see below).

To be able to write SSA tests with oracles in it, I modified the SSA parser to treat any call to a function with "oracle" in the name as a foreign function, rather than reject the SSA with "unknown function 'oracle_call'" for example.

Finally I modified the proxies pass to not create a constrained proxy to forward a call to an oracle, since it would be rejected by the new SSA validation. The consequence of this is that with a mutable variable, we can get around the rejection in the frontend (see below).

Additional Context

We have this code in the test:

fn main() {
    let mut foo: unconstrained fn() = foo_wrapper;
    foo = foo_oracle;
    // safety:
    unsafe {
        foo();
    }
}

#[oracle(foo)]
unconstrained fn foo_oracle() {}

unconstrained fn foo_wrapper() {
    foo_oracle();
}

The compiler is okay with let mut foo: unconstrained fn() = foo_wrapper;, but it would also be okay with let mut foo: unconstrained fn() = foo_oracle;, because the mut prevents the lint from running.

If we try to compile this, we get an error:

❯ cargo run -q -p nargo_cli -- compile --force
error: SSA validation error: Trying to call foreign function 'foo' from ACIR function 'foo_oracle_proxy f2'
 = Set the NOIR_SHOW_INVALID_SSA env var to see the SSA.

Aborting due to 1 previous error

Try it with the env var set:

❯ NOIR_SHOW_INVALID_SSA=1 cargo run -q -p nargo_cli -- compile --force
--- The SSA failed to validate:
acir(inline) fn main f0 {
  b0():
    v1 = allocate -> &mut function
    store f1 at v1
    v2 = allocate -> &mut function
    store f1 at v2
    store f2 at v1
    store f3 at v2
    v5 = load v1 -> function
    v6 = load v2 -> function
    call v5()
    return
}
brillig(inline) fn foo_wrapper f1 {
  b0():
    call foo()
    return
}
acir(inline_always) fn foo_oracle_proxy f2 {
  b0():
    call foo()
    return
}
brillig(inline_always) fn foo_oracle_proxy f3 {
  b0():
    call foo()
    return
}


error: SSA validation error: Trying to call foreign function 'foo' from ACIR function 'foo_oracle_proxy f2'

Aborting due to 1 previous error

We can see that the proxies pass added in #10160 actually created both a constrained and an unconstrained foo_oracle_proxy when it saw that we use foo_oracle as a value in foo = foo_oracle;, but the body is just a forwarding of the call to the original, which is invalid in the acir proxy.

I didn't think this was a fault with the proxies pass: its job is to make sure we have a global ID f2 and f3 to use instead of foo_oracle directly. Ostensibly it could further wrap the call from f2 to foo to go through a brillig proxy, but that points at the aforementioned auto-wrapping of oracle calls, and, under different (non-mutable) circumstances, the frontend would have already rejected this call completely, so I didn't think we should wrap here.

OTOH I noticed that the monomorphized AST does allow a different solution:

fn main$f0() -> () {
    let mut foo$l0 = (foo_wrapper$f1, foo_wrapper$f1);
    foo$l0 = (foo_oracle$f2, foo_oracle$f3);
    {
        foo$l0.0();
    }
}
unconstrained fn foo_wrapper$f1() -> () {
    foo_oracle$foo();
}
#[inline_always]
fn foo_oracle_proxy$f2() -> () {
    foo_oracle$foo()
}
#[inline_always]
unconstrained fn foo_oracle_proxy$f3() -> () {
    foo_oracle$foo()
}

We can see that the original let mut foo$l0 = (foo_wrapper$f1, foo_wrapper$f1); consists of a pair of unconstrained functions, whereas foo$l0 = (foo_oracle$f2, foo_oracle$f3); has both constrained and unconstrained. The proxies pass could recognise that it's futile to create the constrained variant. NB the code does compile if we don't have the re-assignment, since foo(); is correctly called form unsafe.

Actually, because of the new SSA validation we must not generate an ACIR proxy to call an oracle, otherwise existing tests fail as well.

With that change, the code above does compile, because it no longer creates the constrained proxy:

fn main$f0() -> () {
    let mut foo$l0 = (foo_wrapper$f1, foo_wrapper$f1);
    foo$l0 = (foo_oracle$f2, foo_oracle$f2);
    {
        foo$l0.0();
    }
}
unconstrained fn foo_wrapper$f1() -> () {
    foo_oracle$foo();
}
#[inline_always]
unconstrained fn foo_oracle_proxy$f2() -> () {
    foo_oracle$foo()
}

So this is a bit of a loop hole: if we do let foo: unconstrained fn() = foo_oracle; then the frontend rejects it, but if we change it to let mut foo: unconstrained fn() = foo_oracle; then it doesn't, and the proxy pass fixes it. This is a strong hint that we should probably do auto-wrapping, rather than rejection, which is already under way in #10351

User Documentation

Check one:

• No user documentation needed.
• Changes in docs/ included in this PR.
• [For Experimental Features] Changes in docs/ to be submitted in a separate PR.

PR Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 8912289	Previous: 087ce65	Ratio
rollup-block-root-first-empty-tx	1.826 s	1.402 s	1.30
rollup-block-root-single-tx	1.8 s	1.41 s	1.28
rollup-block-root	1.95 s	1.49 s	1.31
rollup-checkpoint-merge	1.82 s	1.45 s	1.26
rollup-tx-merge	1.87 s	1.38 s	1.36

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: af21bd5	Previous: 693c23c	Ratio
perfectly_parallel_batch_inversion_opcodes	2791282 ns/iter (± 33169)	2262930 ns/iter (± 1745)	1.23

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 539ff08	Previous: 3900aa8	Ratio
perfectly_parallel_batch_inversion_opcodes	2789706 ns/iter (± 5758)	2261607 ns/iter (± 1770)	1.23

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: 08f2309818ece166fabaf669aaa71bf75521b815, compared to commit: 087ce656da52d5835fc122dcb760506c686a8eb5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_10156_inliner_max	-1 ✅	-0.05%
regression_10156_inliner_min	-1 ✅	-0.05%
regression_10156_inliner_zero	-1 ✅	-0.05%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_10156_inliner_max	2,132 (-1)	-0.05%
regression_10156_inliner_min	2,132 (-1)	-0.05%
regression_10156_inliner_zero	2,132 (-1)	-0.05%

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: 08f2309818ece166fabaf669aaa71bf75521b815, compared to commit: 087ce656da52d5835fc122dcb760506c686a8eb5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_10156_inliner_max	-1 ✅	-0.05%
regression_10156_inliner_min	-1 ✅	-0.05%
regression_10156_inliner_zero	-1 ✅	-0.05%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_10156_inliner_max	2,134 (-1)	-0.05%
regression_10156_inliner_min	2,134 (-1)	-0.05%
regression_10156_inliner_zero	2,134 (-1)	-0.05%

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 6f798c4	Previous: 3900aa8	Ratio
test_report_zkpassport_noir_rsa_	2 s	1 s	2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 8a2cea0	Previous: 087ce65	Ratio
rollup-root	0.005 s	0.004 s	1.25

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 539ff08	Previous: 3900aa8	Ratio
rollup-block-root-single-tx	0.003 s	0.002 s	1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[asterite]

I didn't check the changes here, but should this code compile?

struct Foo {
    foo: unconstrained fn(),
}

global FOO: Foo = Foo { foo };

fn main() {
    unsafe {
        (FOO.foo)()
    }
}

#[oracle(foo)]
unconstrained fn foo() {}

In this branch it does, but it seems that it should also be rejected if a global oracle function is rejected.

[aakoshh]

Thanks @asterite for always pointing out an extra case that I didn't think of.

I tried with just a local and that compiles as well:

struct Foo {
    foo: unconstrained fn(),
}

fn main() {
    let foo = Foo { foo };

    unsafe {
        (foo.foo)()
    }
}

#[oracle(foo)]
unconstrained fn foo() {}

I'll check what prevents the lint from executing this time, but automatic wrapping will probably be a better way to handle in the long run.

[aakoshh]

So this is an inherent limitation in the lookup_function_from_expr method: it should say that it only works for HirExpression::Ident.

The above is a HirExpression::MemberAccess, for which I suppose I should try to look up what the lhs expression is, then try to evaluate the rhs string to figure out which field we are looking for, try to pry it out of the HirExpression::Constructor that I assume we can reach through the lhs.

And then what about tuples, arrays, etc. For example the lint does not reject this either:

fn main() {
    let bar = (foo, foo);
    let baz = bar.0;
    unsafe {
        baz();
    }
}

[aakoshh]

Added a more elaborate description of what to expect from this method and limited it to pub(crate). I'm not sure it's worth trying to evaluate the HIR at compile time just for this. Maybe instead checking this particular lint at the call site, we should generally ban assigning oracles as values to anything. I suppose with auto-wrap this will happen implicitly.

[aakoshh]

Or maybe we should relax the lint and say that as long as you are not trying to call the oracle directly, but through a variable, the lint lets you off the hook, because the proxies pass will have your back and wrap it for you, and we can't figure out all the potential indirections either. If you call it directly, then currently we don't wrap it, but the lint can see this and say no.

[aakoshh]

Opened an alternative in #10826 based on this PR, which I think is better as it points more towards the auto-wrapping case.

[aakoshh]

Closing this as I don't see any reason why we should not adopt the other PR.