feat: Allow nested arrays and vectors in Brillig foreign calls by ggiraldez · Pull Request #4404 · AztecProtocol/aztec-packages

ggiraldez · 2024-02-03T02:23:58Z

Description

This is a mirror of noir-lang/noir#4053. From the discussion there, it was suggested that we should open the PR on this repository instead, due to the changes in the serialization format.

Problem*

Resolves noir-lang/noir#4052

The Brillig VM has no knowledge of the data types of array/slices elements, but it also references arrays and vectors by a pointer to a small structure in memory. This results in incorrect data passed to foreign functions when the values consist of arrays (or slices) nested inside other arrays (or slices).

Summary*

⚠️ THIS PR CHANGES ACIR SERIALIZATION FORMAT

This PR adds element type descriptors to HeapArray and HeapVector Brillig structures to allow the code that gathers the foreign call parameters to correctly interpret the data to be read from the VM memory.

Additional Context

The counterpart of is returning nested arrays/slices from foreign calls, which this PR does not implement. The existing foreign functions (and the ones required by the debugger oracle) do not require this, so it's not currently a problem.

Furthermore, implementing that feature would require some design decisions regarding how are the arrays/slices allocated in memory and by who.

To return arrays the compiler now generates code to pre-allocate the heap array, adjusting the heap pointer register and setting a register for the VM to write the results into memory. For nested arrays, it would need to pre-allocate the outer as well as the inner arrays and write the pointers to the inner arrays into memory, all before making the foreign call.

When adding slices (heap vectors) to the mix, it gets more complicated because they are dynamically sized, so it wouldn't be possible for the compiler to pre-allocate the inner arrays/slices. Also, for slices the compiler will allocate a register for the VM to write the size of the returned result, which it uses to generate code to adjust the heap pointer register after the call is complete.

It's unclear whose responsibility would be to allocate heap space when mixing arrays and slices, and the compiled code should handle it. A simpler approach may be for the compiler to generate the foreign call passing the heap pointer register and delegating all the allocations and heap pointer adjustments to the VM implementation. That would simplify generated Brillig code and move the burden on to the VM implementation.

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[Exceptional Case] Documentation 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.

sirasistant · 2024-02-05T16:45:00Z

I think acir.hpp in barretenberg needs to be updated with the changes in acir.cpp. Keep in mind that you'll need to replace the throw serde::deserialization_error(... with throw_or_abort(...

vezenovm · 2024-02-05T18:27:22Z

noir/acvm-repo/brillig_vm/src/lib.rs

-                            ForeignCallParam::Single(value) => {
-                                self.memory.write(*value_index, *value);
+                        ValueOrArray::MemoryAddress(value_index) => {
+                            assert!(*value_type == HeapValueType::Simple);


Rather than having these asserts on value_type couldn't we match against (destination, value_type) and then have one failure case for the match statement about having a mismatched type.

Ah, that's a good idea! Thanks!

vezenovm · 2024-02-05T18:29:51Z

noir/acvm-repo/brillig_vm/src/lib.rs

        match input {
-            ValueOrArray::MemoryAddress(value_index) => self.memory.read(value_index).into(),
+            ValueOrArray::MemoryAddress(value_index) => {
+                assert!(*value_type == HeapValueType::Simple);


Same comment about the asserts on value_type as in process_opcode

vezenovm · 2024-02-05T18:39:42Z

Some minor comments, overall looks ready to merge after the serialization is updated.

…foreign-calls

Addressing the code review comments, match on value and type simultaneously instead of just the value and then asserting on the type.

sirasistant · 2024-02-06T10:14:08Z

barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp

+        std::vector<Circuit::Opcode> opcodes;
+        Circuit::ExpressionWidth expression_width;
+        std::vector<Circuit::Witness> private_parameters;
+        Circuit::PublicInputs public_parameters;
+        Circuit::PublicInputs return_values;
+        std::vector<std::tuple<Circuit::OpcodeLocation, std::string>> assert_messages;


I think this changes need to be removed! Also it's missing passing the formatter on the whole file

To clarify, it should look like the previous diff without the Circuit:: prefixes.

Hmm... I thought this would have the same contents as the generated acir.cpp with the previously mentioned replacement only. Do you have the procedure to update this file documented somewhere?

@ggiraldez unfortunately the documentation on this process is lacking. The replacement mentioned here is the only other place. I'm going to quickly do the serialization for you and also make an issue to document this process.

I think I was able to fix this one, and found the format.sh script in the directory. I'll push a commit with the changes, but I'm unsure it's working properly. Locally, I managed to get the build dependencies installed for Barrentenberg, but it's still complaining about a different thing somewhere else in the codebase. I'm sure I'm missing some language dependency or something.

Ok the committed changes look correct. Let's see what CI says. I will document the process for any future updates

…foreign-calls

TomAFrench

Merging this is going to delay us being able to sync the repos until we have another release of aztec-packages. If we're not going to do an extra early release then we should hold off on this.

TomAFrench

Removing block as we're going to do an extra release.

This PR mirrors the changes of #4404 to make them easier to merge --------- Co-authored-by: Gustavo Giráldez <ggiraldez@manas.tech>

sirasistant · 2024-02-07T14:06:34Z

Merged here #4478

ggiraldez · 2024-02-07T15:09:45Z

Merged here #4478

Thank you!

This PR mirrors the changes of #4404 to make them easier to merge --------- Co-authored-by: Gustavo Giráldez <ggiraldez@manas.tech>

This PR mirrors the changes of AztecProtocol/aztec-packages#4404 to make them easier to merge --------- Co-authored-by: Gustavo Giráldez <ggiraldez@manas.tech>

…Protocol#4478) This PR mirrors the changes of AztecProtocol#4404 to make them easier to merge --------- Co-authored-by: Gustavo Giráldez <ggiraldez@manas.tech>

feat: Allow nested arrays and vectors in Brillig foreign calls

c1884d2

kevaundray requested review from sirasistant and vezenovm February 5, 2024 14:37

vezenovm reviewed Feb 5, 2024

View reviewed changes

ggiraldez added 3 commits February 5, 2024 15:31

Merge remote-tracking branch 'upstream/master' into nested-arrays-to-…

11cb2c9

…foreign-calls

chore: Update acir.hpp from changes in noir subrepo

47b0172

chore: Refactor matches on values and types

1ee18c0

Addressing the code review comments, match on value and type simultaneously instead of just the value and then asserting on the type.

ggiraldez requested a review from vezenovm February 5, 2024 21:01

sirasistant reviewed Feb 6, 2024

View reviewed changes

chore: Update acir.hpp

80201de

vezenovm approved these changes Feb 6, 2024

View reviewed changes

vezenovm enabled auto-merge (squash) February 6, 2024 18:30

Merge remote-tracking branch 'upstream/master' into nested-arrays-to-…

c7b041a

…foreign-calls

auto-merge was automatically disabled February 6, 2024 20:35
Head branch was pushed to by a user without write access

vezenovm enabled auto-merge (squash) February 6, 2024 20:38

TomAFrench disabled auto-merge February 7, 2024 09:28

TomAFrench requested changes Feb 7, 2024

View reviewed changes

TomAFrench approved these changes Feb 7, 2024

View reviewed changes

sirasistant mentioned this pull request Feb 7, 2024

feat: Allow nested arrays and vectors in Brillig foreign calls #4478

Merged

sirasistant added a commit that referenced this pull request Feb 7, 2024

feat: Allow nested arrays and vectors in Brillig foreign calls (#4478)

bbfa337

This PR mirrors the changes of #4404 to make them easier to merge --------- Co-authored-by: Gustavo Giráldez <ggiraldez@manas.tech>

ggiraldez closed this Feb 7, 2024

TomAFrench pushed a commit that referenced this pull request Feb 7, 2024

feat: Allow nested arrays and vectors in Brillig foreign calls (#4478)

faf5cc1

This PR mirrors the changes of #4404 to make them easier to merge --------- Co-authored-by: Gustavo Giráldez <ggiraldez@manas.tech>

Conversation

ggiraldez commented Feb 3, 2024

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

Uh oh!

sirasistant commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vezenovm commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomAFrench left a comment

Choose a reason for hiding this comment

Uh oh!

TomAFrench left a comment

Choose a reason for hiding this comment

Uh oh!

sirasistant commented Feb 7, 2024

Uh oh!

ggiraldez commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants