chore: fixing `@safety` warnings #11094

benesjan · 2025-01-07T19:59:28Z

Note for reviewer

I originally addressed a bunch of other stuff in this PR but as @nventuro pointed out it became too messy so I separated those changes into a PR up the stack.

Merging currently blocked by this nargo fmt bug

benesjan · 2025-01-07T19:59:54Z

refactor: random fixes and readability improvements #11129
chore: fixing @safety warnings #11094 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

nventuro · 2025-01-07T20:23:40Z

noir-projects/aztec-nr/aztec/src/context/private_context.nr

+            //@safety TODO(https://github.com/AztecProtocol/aztec-packages/issues/8985): Fix this.
+            // WARNING: This is insecure and should be temporary!
+            // The oracle repacks the arguments and returns a new args_hash.
+            // new_args = [selector, ...old_args], so as to make it suitable to call the public dispatch function.
+            // We don't validate or compute it in the circuit because a) it's harder to do with slices, and
+            // b) this is only temporary.
+            enqueue_public_function_call_internal(
+                contract_address,
+                function_selector,
+                args_hash,
+                counter,
+                is_static_call,
+            )


@fcarreiro this is an old issue from october that doesn't seem to have much activity or tracking - is there any blocker to getting this done?

Maybe it's a good idea to add tracking, you are right. The pointed ticket is tracking something else which is considered done.

Regarding a solution: TBH IDK how to solve it, and currently the AVM team is quite busy with other stuff. IDK if we'd be able to come back to this until just after Ignition (especially since it's not really an AVM thing).

Possible solutions: (1) The easy way to make this secure, IIRC, is to constrain the hashes there in place. Something like assert(old_hash == hash([a,b,c])) and assert(new_hash == hash([selector,a,b,c]). This would blow up the constraints and when I was doing this everyone was chipping away constraints so I thought it wouldn't fly.
(2) Something that I tried was to change the the private interfaces (macros) and call methods to take a slice. Then I could do the same as I did in public. This didn't work because sizes need to be known at comptime sadly.
(3) The other possible solution (more elegant from some PoV and less from other) is to change all the users of the "call with hash" method to include the selector themselves. IIRC from a discussion with Palla, the major offenders and only real need for passing the hash are the contract (?) entry points which then kind of call other functions with an already given set of params. Selectors are now an aztec-nr/js only concept but IIRC then it would be ok to change the JS code to insert the selector appropiately.

This is all from the top of my mind so there might be mistakes. Please add that info if you create a ticket, thanks!

nventuro · 2025-01-07T20:24:30Z

noir-projects/aztec-nr/aztec/src/context/private_context.nr

-            is_static_call,
-        );
+        let args_hash = unsafe {
+            //@safety TODO(https://github.com/AztecProtocol/aztec-packages/issues/8985): Fix this.


Can we add some whitespace and do // @safety? It looks very odd otherwise

Currently this makes the compiler cry. But it will be trivial to change later as it's just a string replacement.

noir-lang/noir#6992

noir-projects/aztec-nr/aztec/src/initializer.nr

nventuro · 2025-01-07T20:34:22Z

noir/noir-repo/noir_stdlib/src/hash/mod.nr

@@ -95,7 +95,7 @@ fn __derive_generators<let N: u32, let M: u32>(
 // Same as from_field but:
 // does not assert the limbs are 128 bits
 // does not assert the decomposition does not overflow the EmbeddedCurveScalar
-fn from_field_unsafe(scalar: Field) -> EmbeddedCurveScalar {
+pub fn from_field_unsafe(scalar: Field) -> EmbeddedCurveScalar {


I don't think the Noir team would necessarily be happy about exposing this in their stdlib - did you check with them?

No, decided to go ahead and then tag Tom for a review as the change is easy to revert.

I would prefer to keep this as an internal implementation detail to the stdlib. If you need something similar then you can vendor it into aztec-nr.

That would require me to copy not only from_field_unsafe but also compute_decomposition which seems like quite a lot of code to copy and I feel like it does not really belong to Aztec.nr (it feels too low-level).

But maybe we could make the API std nicer by moving the from_field_unsafe func to EmbeddedCurveScalar just like was done for from_field?

compute_decomposition is a very small function though?

Regardless, the noir stdlib is a very conservative library as breaking changes are a breaking change to the language itself. We're not going to be exposing this function oublically for that reason.

@TomAFrench As you please. I will revert this change as the scope of the PR is already quite big and don't want to make it bigger.

Regardless, the noir stdlib is a very conservative library as breaking changes are a breaking change to the language itself.

I feel like in this case this might not be that big of a deal because we are exposing the function and if we did the change I proposed we would be moving a function which was not intended for external use. So the breaking change would be only for users that used the func anyway just like we did in aztec-packages.

I would also argue that us needing this function is a good sign that it's desirable to expose.

But the best approach might be to just change the API such that we don't need the function. Which could be done by having an MSM func that takes in the values from the smaller field and then does the conversion on its own. This would also improve safety since we rely on MSM to constrain the limbs.

@TomAFrench WDYT?

noir/noir-repo/noir_stdlib/src/uint128.nr

noir-projects/aztec-nr/aztec/src/note/note_getter_options.nr

noir-projects/aztec-nr/aztec/src/utils/array/collapse.nr

noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr

TomAFrench · 2025-01-08T11:14:39Z

Merging master as I want to check if failures in another branch are from master.

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

noir/noir-repo/noir_stdlib/src/hash/poseidon2.nr

noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays/sort_by_counter.nr

noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays/get_sorted_tuple.nr

...oir-protocol-circuits/crates/types/src/utils/arrays/assert_split_transformed_value_arrays.nr

noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr

noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays/assert_combined_array.nr

...ates/types/src/utils/arrays/assert_exposed_sorted_transformed_value_array/get_order_hints.nr

TomAFrench · 2025-01-09T15:58:43Z

Note that the format of safety comments is in the process of changing.

benesjan · 2025-01-10T18:13:05Z

@TomAFrench @nventuro could I get an approval here or are we waiting for new Noir sync?

I've already addressed all of Nico's comments here and it now contains only safety warnings so I think this should be fine to merge.

I am having too many PRs open...

TomAFrench · 2025-01-10T22:23:04Z

As soon as ci is unfucked we're going to be merging a change to the safety comment format so i think we should either hold off until that's in or just preemptively switch to the new format .

TomAFrench · 2025-01-13T09:15:53Z

The new comment format is now in Aztec packages

Sets the L1 tx utils timeout to the end of the L2 slot.

nventuro reviewed Jan 7, 2025

View reviewed changes

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from d579beb to e29c345 Compare January 7, 2025 22:07

benesjan changed the title ~~chore: fixing aztec-nr warnings~~ chore: fixing aztec-nr and protocol circuits warnings Jan 7, 2025

benesjan marked this pull request as ready for review January 7, 2025 22:08

benesjan requested review from fcarreiro and dbanks12 as code owners January 7, 2025 22:08

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from e29c345 to 26d8be5 Compare January 8, 2025 02:36

benesjan commented Jan 8, 2025

View reviewed changes

noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr Outdated Show resolved Hide resolved

benesjan requested review from nventuro and TomAFrench and removed request for fcarreiro and dbanks12 January 8, 2025 03:00

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 50c83c0 to d9308ca Compare January 8, 2025 18:14

nventuro reviewed Jan 8, 2025

View reviewed changes

benesjan marked this pull request as draft January 8, 2025 18:39

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 9461346 to 48fb467 Compare January 8, 2025 21:22

benesjan removed the request for review from TomAFrench January 8, 2025 23:02

benesjan added the e2e-all CI: Enables this CI job. label Jan 8, 2025

benesjan marked this pull request as ready for review January 8, 2025 23:19

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from ca3ec19 to 35f4b12 Compare January 9, 2025 14:38

benesjan mentioned this pull request Jan 9, 2025

refactor: random fixes and readability improvements #11129

Draft

benesjan changed the title ~~chore: fixing aztec-nr and protocol circuits warnings~~ chore: fixing Aztec.nr and protocol circuits @safety warnings Jan 9, 2025

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 35f4b12 to b7b7b66 Compare January 9, 2025 15:27

benesjan changed the title ~~chore: fixing Aztec.nr and protocol circuits @safety warnings~~ chore: fixing @safety warnings Jan 9, 2025

benesjan requested a review from nventuro January 9, 2025 16:25

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from c9df6c8 to 7e6e87e Compare January 10, 2025 21:43

benesjan marked this pull request as draft January 13, 2025 17:49

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch 2 times, most recently from e1f8440 to fc91f1f Compare January 13, 2025 18:12

benesjan marked this pull request as ready for review January 13, 2025 18:29

benesjan mentioned this pull request Jan 13, 2025

chore: require safety doc comment for unsafe instead of //@safety noir-lang/noir#6992

Merged

5 tasks

spalladino and others added 7 commits January 13, 2025 19:13

fix: Sequencer times out L1 tx at end of L2 slot (#11112)

f10d88a

Sets the L1 tx utils timeout to the end of the L2 slot.

new safety comment format

f7921e2

reverting accidental change

61116ef

WIP

b29ece4

last touches

893c55a

better formatting

c25eb90

fix: triple /

80d9ab4

benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 3e094bc to 80d9ab4 Compare January 13, 2025 19:14

benesjan added the S-blocked Status: Blocked label Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fixing `@safety` warnings #11094

chore: fixing `@safety` warnings #11094

benesjan commented Jan 7, 2025 •

edited

Loading

benesjan commented Jan 7, 2025 •

edited

Loading

nventuro Jan 7, 2025

fcarreiro Jan 8, 2025 •

edited

Loading

nventuro Jan 7, 2025

benesjan Jan 7, 2025

nventuro Jan 8, 2025

nventuro Jan 7, 2025

benesjan Jan 7, 2025

TomAFrench Jan 7, 2025

benesjan Jan 8, 2025 •

edited

Loading

TomAFrench Jan 8, 2025

benesjan Jan 8, 2025

benesjan Jan 8, 2025

TomAFrench commented Jan 8, 2025

TomAFrench commented Jan 9, 2025

benesjan commented Jan 10, 2025 •

edited

Loading

TomAFrench commented Jan 10, 2025

TomAFrench commented Jan 13, 2025

chore: fixing @safety warnings #11094

Are you sure you want to change the base?

chore: fixing @safety warnings #11094

Conversation

benesjan commented Jan 7, 2025 • edited Loading

Note for reviewer

benesjan commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

fcarreiro Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesjan Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAFrench commented Jan 8, 2025

TomAFrench commented Jan 9, 2025

benesjan commented Jan 10, 2025 • edited Loading

TomAFrench commented Jan 10, 2025

TomAFrench commented Jan 13, 2025

chore: fixing `@safety` warnings #11094

chore: fixing `@safety` warnings #11094

benesjan commented Jan 7, 2025 •

edited

Loading

benesjan commented Jan 7, 2025 •

edited

Loading

fcarreiro Jan 8, 2025 •

edited

Loading

benesjan Jan 8, 2025 •

edited

Loading

benesjan commented Jan 10, 2025 •

edited

Loading