Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Allow passing nested arrays and slices into foreign calls #4053

Closed
wants to merge 12 commits into from
Closed

fix: Allow passing nested arrays and slices into foreign calls #4053

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cd30430
feat: (incomplete) support (de)flattening arrays/vectors for foreign …
ggiraldez Jan 10, 2024
ad59a27
feat: Flatten arrays and vectors when calling foreign functions
ggiraldez Jan 11, 2024
3b050eb
feat: test for passing nested arrays/vectors into foreign calls
ggiraldez Jan 12, 2024
11d2441
chore: cargo fmt
ggiraldez Jan 16, 2024
08d61b8
feat: update serialization or ACIR circuits
ggiraldez Jan 16, 2024
182d5f8
chore: run prettier
ggiraldez Jan 16, 2024
f7666e4
fix: test compilation in noirc_evaluator
ggiraldez Jan 16, 2024
60e4c9e
feat: Move heap value types into the ForeignCall opcode
ggiraldez Jan 24, 2024
a90e230
feat: Revert removing the Copy trait from Brillig types
ggiraldez Jan 24, 2024
a168419
Merge remote-tracking branch 'upstream/master' into nested-arrays-to-…
ggiraldez Jan 24, 2024
0ae9831
Merge remote-tracking branch 'upstream/master' into nested-arrays-to-…
ggiraldez Feb 2, 2024
5bc8cca
chore: Add case to debug_logs to show nested arrays passing to foreig…
ggiraldez Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
812 changes: 505 additions & 307 deletions acvm-repo/acir/codegen/acir.cpp
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion acvm-repo/acir/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
mod reflection {
//! Getting test failures? You've probably changed the ACIR serialization format.
//!
//! These tests generate C++ deserializers for [`ACIR bytecode`][super::circuit::Circuit]

Check warning on line 20 in acvm-repo/acir/src/lib.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (deserializers)
//! and the [`WitnessMap`] structs. These get checked against the C++ files committed to the `codegen` folder
//! to see if changes have been to the serialization format. These are almost always a breaking change!
//!
Expand All @@ -32,7 +32,8 @@
};

use brillig::{
BinaryFieldOp, BinaryIntOp, BlackBoxOp, Opcode as BrilligOpcode, RegisterOrMemory,
BinaryFieldOp, BinaryIntOp, BlackBoxOp, HeapValueType, Opcode as BrilligOpcode,
RegisterOrMemory,
};
use serde_reflection::{Tracer, TracerConfig};

Expand Down Expand Up @@ -70,6 +71,7 @@
tracer.trace_simple_type::<BlackBoxOp>().unwrap();
tracer.trace_simple_type::<Directive>().unwrap();
tracer.trace_simple_type::<RegisterOrMemory>().unwrap();
tracer.trace_simple_type::<HeapValueType>().unwrap();

let registry = tracer.registry().unwrap();

Expand Down
36 changes: 24 additions & 12 deletions acvm-repo/acir/tests/test_program_serialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use acir::{
native_types::{Expression, Witness},
};
use acir_field::FieldElement;
use brillig::{HeapArray, RegisterIndex, RegisterOrMemory};
use brillig::{HeapArray, HeapValueType, RegisterIndex, RegisterOrMemory};

#[test]
fn addition_circuit() {
Expand Down Expand Up @@ -180,7 +180,9 @@ fn simple_brillig_foreign_call() {
bytecode: vec![brillig::Opcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))],
input_value_types: vec![HeapValueType::Simple],
}],
predicate: None,
};
Expand All @@ -196,10 +198,11 @@ fn simple_brillig_foreign_call() {
let bytes = Circuit::serialize_circuit(&circuit);

let expected_serialization: Vec<u8> = vec![
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 173, 143, 49, 10, 64, 33, 12, 67, 99, 63, 124, 60, 142,
222, 192, 203, 56, 184, 56, 136, 120, 126, 5, 21, 226, 160, 139, 62, 40, 13, 45, 132, 68,
3, 80, 232, 124, 164, 153, 121, 115, 99, 155, 59, 172, 122, 231, 101, 56, 175, 80, 86, 221,
230, 31, 58, 196, 226, 83, 62, 53, 91, 16, 122, 10, 246, 84, 99, 243, 0, 30, 59, 1, 0, 0,
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 173, 143, 49, 10, 0, 33, 12, 4, 215, 28, 28, 62, 199,
251, 193, 125, 198, 194, 198, 66, 196, 247, 43, 168, 176, 136, 218, 232, 64, 200, 50, 69,
216, 104, 0, 10, 149, 135, 50, 211, 221, 223, 182, 57, 227, 83, 247, 110, 25, 238, 43, 212,
85, 151, 121, 91, 118, 62, 217, 16, 119, 159, 141, 121, 234, 132, 164, 96, 77, 6, 148, 102,
152, 53, 83, 1, 0, 0,
];

assert_eq!(bytes, expected_serialization)
Expand Down Expand Up @@ -248,11 +251,20 @@ fn complex_brillig_foreign_call() {
RegisterOrMemory::HeapArray(HeapArray { pointer: 0.into(), size: 3 }),
RegisterOrMemory::RegisterIndex(RegisterIndex::from(1)),
],
input_value_types: vec![
HeapValueType::Array { size: 3, value_types: vec![HeapValueType::Simple] },
HeapValueType::Simple,
],
destinations: vec![
RegisterOrMemory::HeapArray(HeapArray { pointer: 0.into(), size: 3 }),
RegisterOrMemory::RegisterIndex(RegisterIndex::from(1)),
RegisterOrMemory::RegisterIndex(RegisterIndex::from(2)),
],
destination_value_types: vec![
HeapValueType::Array { size: 3, value_types: vec![HeapValueType::Simple] },
HeapValueType::Simple,
HeapValueType::Simple,
],
},
],
predicate: None,
Expand All @@ -269,13 +281,13 @@ fn complex_brillig_foreign_call() {
let bytes = Circuit::serialize_circuit(&circuit);

let expected_serialization: Vec<u8> = vec![
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 83, 219, 10, 128, 48, 8, 117, 174, 139, 159, 179,
254, 160, 127, 137, 222, 138, 122, 236, 243, 19, 114, 32, 22, 244, 144, 131, 118, 64, 156,
178, 29, 14, 59, 74, 0, 16, 224, 66, 228, 64, 57, 7, 169, 53, 242, 189, 81, 114, 250, 134,
33, 248, 113, 165, 82, 26, 177, 2, 141, 177, 128, 198, 60, 15, 63, 245, 219, 211, 23, 215,
255, 139, 15, 251, 211, 112, 180, 28, 157, 212, 189, 100, 82, 179, 64, 170, 63, 109, 235,
190, 204, 135, 166, 178, 150, 216, 62, 154, 252, 250, 70, 147, 35, 220, 119, 93, 227, 4,
182, 131, 81, 25, 36, 4, 0, 0,
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 83, 109, 10, 128, 48, 8, 85, 215, 199, 142, 179,
110, 208, 93, 162, 127, 69, 253, 236, 248, 13, 114, 240, 88, 81, 65, 27, 180, 7, 226, 116,
238, 41, 83, 45, 17, 49, 29, 48, 94, 68, 207, 172, 54, 34, 196, 245, 170, 221, 55, 116,
156, 142, 203, 229, 170, 81, 10, 168, 209, 100, 168, 49, 204, 195, 79, 251, 157, 178, 47,
73, 255, 207, 92, 236, 79, 229, 165, 246, 210, 168, 221, 170, 182, 48, 11, 22, 252, 195,
50, 175, 211, 184, 33, 85, 220, 146, 216, 47, 209, 61, 223, 188, 195, 248, 39, 110, 121,
195, 135, 73, 133, 206, 201, 16, 59, 245, 249, 221, 124, 112, 4, 0, 0,
];

assert_eq!(bytes, expected_serialization)
Expand Down
13 changes: 13 additions & 0 deletions acvm-repo/acvm/tests/solver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use acir::{

use acvm::pwg::{ACVMStatus, ErrorLocation, ForeignCallWaitInfo, OpcodeResolutionError, ACVM};
use acvm_blackbox_solver::StubbedBlackBoxSolver;
use brillig_vm::brillig::HeapValueType;

// Reenable these test cases once we move the brillig implementation of inversion down into the acvm stdlib.

Expand Down Expand Up @@ -64,7 +65,9 @@ fn inversion_brillig_oracle_equivalence() {
BrilligOpcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(1))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))],
input_value_types: vec![HeapValueType::Simple],
},
],
predicate: None,
Expand Down Expand Up @@ -185,12 +188,16 @@ fn double_inversion_brillig_oracle() {
BrilligOpcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(1))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))],
input_value_types: vec![HeapValueType::Simple],
},
BrilligOpcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(3))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(2))],
input_value_types: vec![HeapValueType::Simple],
},
],
predicate: None,
Expand Down Expand Up @@ -310,12 +317,16 @@ fn oracle_dependent_execution() {
BrilligOpcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(1))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))],
input_value_types: vec![HeapValueType::Simple],
},
BrilligOpcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(3))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(2))],
input_value_types: vec![HeapValueType::Simple],
},
],
predicate: None,
Expand Down Expand Up @@ -430,7 +441,9 @@ fn brillig_oracle_predicate() {
BrilligOpcode::ForeignCall {
function: "invert".into(),
destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(1))],
destination_value_types: vec![HeapValueType::Simple],
inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))],
input_value_types: vec![HeapValueType::Simple],
},
],
predicate: Some(Expression::default()),
Expand Down
11 changes: 6 additions & 5 deletions acvm-repo/acvm_js/test/shared/complex_foreign_call.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { WitnessMap } from '@noir-lang/acvm_js';

// See `complex_brillig_foreign_call` integration test in `acir/tests/test_program_serialization.rs`.
export const bytecode = Uint8Array.from([
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 83, 219, 10, 128, 48, 8, 117, 174, 139, 159, 179, 254, 160, 127, 137, 222,
138, 122, 236, 243, 19, 114, 32, 22, 244, 144, 131, 118, 64, 156, 178, 29, 14, 59, 74, 0, 16, 224, 66, 228, 64, 57, 7,
169, 53, 242, 189, 81, 114, 250, 134, 33, 248, 113, 165, 82, 26, 177, 2, 141, 177, 128, 198, 60, 15, 63, 245, 219,
211, 23, 215, 255, 139, 15, 251, 211, 112, 180, 28, 157, 212, 189, 100, 82, 179, 64, 170, 63, 109, 235, 190, 204, 135,
166, 178, 150, 216, 62, 154, 252, 250, 70, 147, 35, 220, 119, 93, 227, 4, 182, 131, 81, 25, 36, 4, 0, 0,
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 83, 109, 10, 128, 48, 8, 85, 215, 199, 142, 179, 110, 208, 93, 162, 127, 69,
253, 236, 248, 13, 114, 240, 88, 81, 65, 27, 180, 7, 226, 116, 238, 41, 83, 45, 17, 49, 29, 48, 94, 68, 207, 172, 54,
34, 196, 245, 170, 221, 55, 116, 156, 142, 203, 229, 170, 81, 10, 168, 209, 100, 168, 49, 204, 195, 79, 251, 157, 178,
47, 73, 255, 207, 92, 236, 79, 229, 165, 246, 210, 168, 221, 170, 182, 48, 11, 22, 252, 195, 50, 175, 211, 184, 33,
85, 220, 146, 216, 47, 209, 61, 223, 188, 195, 248, 39, 110, 121, 195, 135, 73, 133, 206, 201, 16, 59, 245, 249, 221,
124, 112, 4, 0, 0,
]);
export const initialWitnessMap: WitnessMap = new Map([
[1, '0x0000000000000000000000000000000000000000000000000000000000000001'],
Expand Down
8 changes: 4 additions & 4 deletions acvm-repo/acvm_js/test/shared/foreign_call.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ import { WitnessMap } from '@noir-lang/acvm_js';

// See `simple_brillig_foreign_call` integration test in `acir/tests/test_program_serialization.rs`.
export const bytecode = Uint8Array.from([
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 173, 143, 49, 10, 64, 33, 12, 67, 99, 63, 124, 60, 142, 222, 192, 203, 56, 184, 56,
136, 120, 126, 5, 21, 226, 160, 139, 62, 40, 13, 45, 132, 68, 3, 80, 232, 124, 164, 153, 121, 115, 99, 155, 59, 172,
122, 231, 101, 56, 175, 80, 86, 221, 230, 31, 58, 196, 226, 83, 62, 53, 91, 16, 122, 10, 246, 84, 99, 243, 0, 30, 59,
1, 0, 0,
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 173, 143, 49, 10, 0, 33, 12, 4, 215, 28, 28, 62, 199, 251, 193, 125, 198, 194, 198,
66, 196, 247, 43, 168, 176, 136, 218, 232, 64, 200, 50, 69, 216, 104, 0, 10, 149, 135, 50, 211, 221, 223, 182, 57,
227, 83, 247, 110, 25, 238, 43, 212, 85, 151, 121, 91, 118, 62, 217, 16, 119, 159, 141, 121, 234, 132, 164, 96, 77, 6,
148, 102, 152, 53, 83, 1, 0, 0,
]);
export const initialWitnessMap: WitnessMap = new Map([
[1, '0x0000000000000000000000000000000000000000000000000000000000000005'],
Expand Down
3 changes: 2 additions & 1 deletion acvm-repo/brillig/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ mod value;
pub use black_box::BlackBoxOp;
pub use foreign_call::{ForeignCallParam, ForeignCallResult};
pub use opcodes::{
BinaryFieldOp, BinaryIntOp, HeapArray, HeapVector, RegisterIndex, RegisterOrMemory,
BinaryFieldOp, BinaryIntOp, HeapArray, HeapValueType, HeapVector, RegisterIndex,
RegisterOrMemory,
};
pub use opcodes::{BrilligOpcode as Opcode, Label};
pub use value::Typ;
Expand Down
26 changes: 26 additions & 0 deletions acvm-repo/brillig/src/opcodes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,27 @@ impl From<usize> for RegisterIndex {
}
}

/// Describes the memory layout for an array/vector element
#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
pub enum HeapValueType {
// A single field element is enough to represent the value
Simple,
// The value read should be interpreted as a pointer to a heap array, which
// consists of a pointer to a slice of memory of size elements, and a
// reference count
Array { value_types: Vec<HeapValueType>, size: usize },
// The value read should be interpreted as a pointer to a heap vector, which
// consists of a pointer to a slice of memory, a number of elements in that
// slice, and a reference count
Vector { value_types: Vec<HeapValueType> },
}

impl HeapValueType {
pub fn all_simple(types: &[HeapValueType]) -> bool {
types.iter().all(|typ| matches!(typ, HeapValueType::Simple))
}
}

/// A fixed-sized array starting from a Brillig register memory location.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy)]
pub struct HeapArray {
Expand Down Expand Up @@ -111,8 +132,13 @@ pub enum BrilligOpcode {
function: String,
/// Destination registers (may be single values or memory pointers).
destinations: Vec<RegisterOrMemory>,
/// Destination value types
destination_value_types: Vec<HeapValueType>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't entirely clear to me as to why these types need to be attached. Can we not determine this from the ForeignCallParam?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, ForeignCallParam is the type to hold the value at runtime of the inputs and outputs for the foreign call. It doesn't belong in the opcode representation of the Brillig program, and it doesn't really have typing information. Maybe I'm missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now you are mitigating that RegisterOrMemory does not containing enough information about whether the internal pointer is itself referencing memory. Could you add a Noir test that shows nested arrays as inputs to a foreign call (can be just a new temporary foreign call or accurate printing of nested arrays/slices).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now you are mitigating that RegisterOrMemory does not containing enough information about whether the internal pointer is itself referencing memory.

Yes, exactly. That info is not available at runtime otherwise.

Could you add a Noir test that shows nested arrays as inputs to a foreign call (can be just a new temporary foreign call or accurate printing of nested arrays/slices).

Sure! I can add new cases to the debug_logs Noir test. I think that would be best.

/// Input registers (may be single values or memory pointers).
inputs: Vec<RegisterOrMemory>,
/// Input value types (for heap allocated structures indicates how to
/// retrieve the elements)
input_value_types: Vec<HeapValueType>,
},
Mov {
destination: RegisterIndex,
Expand Down
Loading
Loading