diff --git a/acvm-repo/acir_field/src/generic_ark.rs b/acvm-repo/acir_field/src/generic_ark.rs index 4e2aaec0263..e6779bda82e 100644 --- a/acvm-repo/acir_field/src/generic_ark.rs +++ b/acvm-repo/acir_field/src/generic_ark.rs @@ -52,8 +52,12 @@ pub trait AcirField: /// This is the number of bits required to represent this specific field element fn num_bits(&self) -> u32; + /// Downcast the field into a `u128`. + /// + /// If the value does not fit, it is truncated. fn to_u128(self) -> u128; + /// Downcast the field into a `u128` if it fits into 128 bits, otherwise return `None`. fn try_into_u128(self) -> Option; fn to_i128(self) -> i128; diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index b38ee1402ea..188ab139204 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -172,7 +172,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { } pub fn step(&mut self) -> Result, OpcodeResolutionError> { - let status = self.vm.process_opcode(); + let status = self.vm.process_opcode().clone(); self.handle_vm_status(status) } diff --git a/acvm-repo/brillig/README.md b/acvm-repo/brillig/README.md index 1e4b2ce4908..ea72cb6eb4f 100644 --- a/acvm-repo/brillig/README.md +++ b/acvm-repo/brillig/README.md @@ -78,7 +78,7 @@ Finite Field VM The Brillig VM operates over finite fields and supports using up to a 128 bit integer representation. The finite field Brillig supports is generalizable and based upon the field ACIR supports (where [Arkworks](https://github.com/arkworks-rs/algebra/tree/master/ff) is used for the finite field interface). The ACIR fields currently supported are the prime fields of the bn254 curve and the bls12-381 curve. -All integers ultimately translate to the finite field which Brillig is based upon. For example when a BinaryIntOp is used, the field is first cast to a fixed bitsize integer that can be accommodated within the field, prior to performing operations. Certain operations, such as ordered comparison or signed division, only make sense in the context of BinaryIntOp. The exact maximum integer value the field can store should not be relied upon, however, it is assured to be capable of packing 3 32-bit integers. +All integers ultimately translate to the finite field which Brillig is based upon. For example when a `BinaryIntOp` is used, the field is first cast to a fixed bitsize integer that can be accommodated within the field, prior to performing operations. Certain operations, such as ordered comparison or signed division, only make sense in the context of `BinaryIntOp`. The exact maximum integer value the field can store should not be relied upon, however, it is assured to be capable of packing 3 32-bit integers. The decision to have Brillig operate over finite fields simplifies SNARK proving and optimizes for efficiency, as ultimately all data types translate to a finite field which is the native data type for SNARKs. @@ -101,13 +101,13 @@ The VM accepts four initialization parameters. 3. The black-box solver, which is used to execute black-box functions 4. A flag, that can be used for profiling purposes -The VM then processes each opcode according to their [specification](02_opcode_listing.md). +The VM then processes each opcode according to their [specification](./src/opcodes.rs). If the VM reaches a foreign call instruction it will first fetch the call's input according to the information inside of the instruction. Through an internal counter and the foreign call results supplied to the VM, the VM will determine whether the outputs have been resolved. If they have not been resolved, the VM will then pause and return a status containing the foreign call inputs. The caller of the VM should interpret the call information returned and update the Brillig process. Execution can then continue as normal. While technically the foreign call result is considered part of the VM's input along with bytecode and calldata, it is practically an input to the program. Error and Exception Handling --- -Failed asserts, represented by the TRAP opcode, during the execution of an unconstrained function, result in an error in Brillig bytecode, accompanied by data detailing the failure. In a hedged trust blockchain environment, a prover might still want to generate a 'valid' proof of an error result so that incorrectness can be correctly attributed. This emphasizes the importance of handling errors and exceptions within the context of a blockchain-based VM. +Failed asserts, represented by the `Trap` opcode, during the execution of an unconstrained function, result in an error in Brillig bytecode, accompanied by data detailing the failure. In a hedged trust blockchain environment, a prover might still want to generate a 'valid' proof of an error result so that incorrectness can be correctly attributed. This emphasizes the importance of handling errors and exceptions within the context of a blockchain-based VM. Conclusion --- @@ -119,9 +119,9 @@ For the following Noir: ```noir fn main() { - let mut a = 10; - let mut b = 5; - let mut c = 0; + let mut a = 10_u32; + let mut b = 5_u32; + let mut c = 0_u32; c = a + b; @@ -131,7 +131,7 @@ fn main() { b = a + b; } - print(a, b, c); + println((a, b, c)); } ``` @@ -141,58 +141,62 @@ One possible Brillig output would be: [ { // location = 0 "Const": { - "destination": 0, + "destination": { "Direct": 0 }, + "bit_size": { "Integer": "U32" }, "value": { "inner": "10" } } }, { // location = 1 "Const": { - "destination": 1, + "destination": { "Direct": 1 }, + "bit_size": { "Integer": "U32" }, "value": { "inner": "5" } } }, { // location = 2 "Const": { - "destination": 2, + "destination": { "Direct": 2 }, + "bit_size": { "Integer": "U32" }, "value": { "inner": "0" } } }, { // location = 3 "Const": { - "destination": 3, + "destination": { "Direct": 3 }, + "bit_size": { "Integer": "U32" }, "value": { "inner": "15" } } }, { // location = 4 "BinaryIntOp": { - "destination": 2, + "destination": { "Direct": 2 }, "op": "Add", - "bit_size": 32, - "lhs": 0, - "rhs": 1 + "bit_size": { "Integer": "U32" }, + "lhs": { "Direct": 0 }, + "rhs": { "Direct": 1 } } }, { // location = 5 "BinaryIntOp": { - "destination": 4, + "destination": { "Direct": 4 }, "op": "LessThanEquals", - "bit_size": 32, - "lhs": 2, - "rhs": 3 + "bit_size": { "Integer": "U32" }, + "lhs": { "Direct": 2 }, + "rhs": { "Direct": 3 } } }, { // location = 6 "JumpIf": { - "condition": 4, + "condition": { "Direct": 4 }, "location": 9 } }, { // location = 7 "BinaryFieldOp": { - "destination": 0, + "destination": { "Direct": 0 }, "op": "Multiply", - "lhs": 0, - "rhs": 1 + "lhs": { "Direct": 0 }, + "rhs": { "Direct": 1 } } }, { // location = 8 @@ -202,20 +206,26 @@ One possible Brillig output would be: }, { // location = 9 "BinaryFieldOp": { - "destination": 1, + "destination": { "Direct": 1 }, "op": "Add", - "lhs": 0, - "rhs": 1 + "lhs": { "Direct": 0 }, + "rhs": { "Direct": 1 } } }, { // location = 10 "ForeignCall": { "function": "print", "destination": [], // No output - "input": [ - {"RegisterIndex": 0}, - {"RegisterIndex": 1}, - {"RegisterIndex": 2} + "destination_value_types": [], + "inputs": [ + { "MemoryAddress": { "Direct": 0 } }, + { "MemoryAddress": { "Direct": 1 } }, + { "MemoryAddress": { "Direct": 2 } } + ], + "input_value_types": [ + { "Simple": { "Integer": "U32" } }, + { "Simple": { "Integer": "U32" } }, + { "Simple": { "Integer": "U32" } } ] } }, @@ -233,6 +243,8 @@ The execution and interpretation of the program would be as follows: 5. At location 10, we queue up inputs to a foreign call from reg0, reg1, reg2 (variables a, b, and c). This interrupts execution, calls to the outer system, and then returns to Brillig execution. If this had outputs, they might be written to registers and memory. 6. We finally reach the final location where we `Stop`. If this were a function to be called by another Brillig function, we would `Return`. +The output above is only for illustrative purposes. To see the actual opcodes, use the `--show-brillig` CLI option. + ## Usage in Noir Runtime diff --git a/acvm-repo/brillig/src/black_box.rs b/acvm-repo/brillig/src/black_box.rs index 5ef722cdcaf..50a04b748bb 100644 --- a/acvm-repo/brillig/src/black_box.rs +++ b/acvm-repo/brillig/src/black_box.rs @@ -6,15 +6,15 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Hash)] #[cfg_attr(feature = "arb", derive(proptest_derive::Arbitrary))] pub enum BlackBoxOp { - /// Encrypts a message using AES128. + /// Encrypts a message using AES-128. AES128Encrypt { inputs: HeapVector, iv: HeapArray, key: HeapArray, outputs: HeapVector }, /// Calculates the Blake2s hash of the inputs. Blake2s { message: HeapVector, output: HeapArray }, /// Calculates the Blake3 hash of the inputs. Blake3 { message: HeapVector, output: HeapArray }, - /// Keccak Permutation function of 1600 width + /// Keccak permutation function of 1600 width. Keccakf1600 { input: HeapArray, output: HeapArray }, - /// Verifies a ECDSA signature over the secp256k1 curve. + /// Verifies an ECDSA signature over the secp256k1 curve. EcdsaSecp256k1 { hashed_msg: HeapVector, public_key_x: HeapArray, @@ -22,7 +22,7 @@ pub enum BlackBoxOp { signature: HeapArray, result: MemoryAddress, }, - /// Verifies a ECDSA signature over the secp256r1 curve. + /// Verifies an ECDSA signature over the secp256r1 curve. EcdsaSecp256r1 { hashed_msg: HeapVector, public_key_x: HeapArray, diff --git a/acvm-repo/brillig/src/foreign_call.rs b/acvm-repo/brillig/src/foreign_call.rs index 1036a262255..2927c85799d 100644 --- a/acvm-repo/brillig/src/foreign_call.rs +++ b/acvm-repo/brillig/src/foreign_call.rs @@ -30,6 +30,7 @@ impl ForeignCallParam { } } + /// Unwrap the field in a `Single` input. Panics if it's an `Array`. pub fn unwrap_field(&self) -> F { match self { ForeignCallParam::Single(value) => *value, @@ -45,18 +46,21 @@ pub struct ForeignCallResult { pub values: Vec>, } +/// Result of a call returning a one output value. impl From for ForeignCallResult { fn from(value: F) -> Self { ForeignCallResult { values: vec![value.into()] } } } +/// Result of a call returning a one output array. impl From> for ForeignCallResult { fn from(values: Vec) -> Self { ForeignCallResult { values: vec![values.into()] } } } +/// Result of a call returning multiple outputs. impl From>> for ForeignCallResult { fn from(values: Vec>) -> Self { ForeignCallResult { values } diff --git a/acvm-repo/brillig/src/lib.rs b/acvm-repo/brillig/src/lib.rs index 3b776e47ea8..8cc357cfa09 100644 --- a/acvm-repo/brillig/src/lib.rs +++ b/acvm-repo/brillig/src/lib.rs @@ -9,6 +9,7 @@ mod opcodes; pub use black_box::BlackBoxOp; pub use foreign_call::{ForeignCallParam, ForeignCallResult}; pub use opcodes::{ - BinaryFieldOp, BinaryIntOp, HeapArray, HeapValueType, HeapVector, MemoryAddress, ValueOrArray, + ArrayAddress, BinaryFieldOp, BinaryIntOp, HeapArray, HeapValueType, HeapVector, MemoryAddress, + ValueOrArray, VectorAddress, }; pub use opcodes::{BitSize, BrilligOpcode as Opcode, IntegerBitSize, Label}; diff --git a/acvm-repo/brillig/src/opcodes.rs b/acvm-repo/brillig/src/opcodes.rs index c0f54e0f256..7c6a2495d4d 100644 --- a/acvm-repo/brillig/src/opcodes.rs +++ b/acvm-repo/brillig/src/opcodes.rs @@ -9,22 +9,30 @@ pub type Label = usize; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] #[cfg_attr(feature = "arb", derive(proptest_derive::Arbitrary))] pub enum MemoryAddress { - /// Specifies an exact index in the VM's memory + /// Specifies an exact index in the VM's memory. Direct(usize), /// Specifies an index relative to the stack pointer. /// /// It is resolved as the current stack pointer plus the offset stored here. + /// + /// The current stack pointer is wherever slot 0 of the `Memory` points at. Relative(usize), } impl MemoryAddress { + /// Create a `Direct` address. pub fn direct(address: usize) -> Self { MemoryAddress::Direct(address) } + + /// Create a `Relative` address. pub fn relative(offset: usize) -> Self { MemoryAddress::Relative(offset) } + /// Return the index in a `Direct` address. + /// + /// Panics if it's `Relative`. pub fn unwrap_direct(self) -> usize { match self { MemoryAddress::Direct(address) => address, @@ -32,6 +40,9 @@ impl MemoryAddress { } } + /// Return the index in a `Relative` address. + /// + /// Panics if it's `Direct`. pub fn unwrap_relative(self) -> usize { match self { MemoryAddress::Direct(_) => panic!("Expected relative memory address"), @@ -39,6 +50,7 @@ impl MemoryAddress { } } + /// Return the index in the address. pub fn to_usize(self) -> usize { match self { MemoryAddress::Direct(address) => address, @@ -53,6 +65,11 @@ impl MemoryAddress { } } + pub fn is_direct(&self) -> bool { + !self.is_relative() + } + + /// Offset the address by `amount`, while preserving its type. pub fn offset(&self, amount: usize) -> Self { match self { MemoryAddress::Direct(address) => MemoryAddress::Direct(address + amount), @@ -70,26 +87,70 @@ impl std::fmt::Display for MemoryAddress { } } +/// Wrapper for array addresses, with convenience methods for various offsets. +/// +/// The array consists of a ref-count followed by a number of items according +/// the size indicated by the type. +pub struct ArrayAddress(MemoryAddress); + +impl ArrayAddress { + pub fn items_start(&self) -> MemoryAddress { + self.0.offset(1) + } +} + +impl From for ArrayAddress { + fn from(value: MemoryAddress) -> Self { + Self(value) + } +} + +/// Wrapper for vector addresses, with convenience methods for various offsets. +/// +/// The vector consists of a ref-count, followed by the capacity, and then a +/// number of items indicated by the capacity. +/// +/// The semantic length of the vector is maintained at a separate address. +pub struct VectorAddress(MemoryAddress); + +impl VectorAddress { + /// Capacity of the vector. + pub fn size_addr(&self) -> MemoryAddress { + self.0.offset(1) + } + pub fn items_start(&self) -> MemoryAddress { + self.0.offset(2) + } +} + +impl From for VectorAddress { + fn from(value: MemoryAddress) -> Self { + Self(value) + } +} + /// Describes the memory layout for an array/vector element #[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize, Hash)] pub enum HeapValueType { - // A single field element is enough to represent the value with a given bit size + /// A single field element is enough to represent the value with a given bit size. Simple(BitSize), - // 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 + /// The value read should be interpreted as a pointer to a [HeapArray], which + /// consists of a pointer to a slice of memory of size elements, and a + /// reference count. Array { value_types: Vec, 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 + /// The value read should be interpreted as a pointer to a [HeapVector], 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 }, } impl HeapValueType { + /// Check that all types are `Simple`. pub fn all_simple(types: &[HeapValueType]) -> bool { types.iter().all(|typ| matches!(typ, HeapValueType::Simple(_))) } + /// Create a `Simple` type to represent a `Field`. pub fn field() -> HeapValueType { HeapValueType::Simple(BitSize::Field) } @@ -157,7 +218,11 @@ impl std::fmt::Display for HeapValueType { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, Hash)] #[cfg_attr(feature = "arb", derive(proptest_derive::Arbitrary))] pub struct HeapArray { + /// Pointer to a memory address which hold the address to the start of the items in the array. + /// + /// That is to say, the address retrieved from the pointer doesn't need any more offsetting. pub pointer: MemoryAddress, + /// Statically known size of the array. pub size: usize, } @@ -173,11 +238,15 @@ impl std::fmt::Display for HeapArray { } } -/// A memory-sized vector passed starting from a Brillig memory location and with a memory-held size +/// A memory-sized vector passed starting from a Brillig memory location and with a memory-held size. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, Hash)] #[cfg_attr(feature = "arb", derive(proptest_derive::Arbitrary))] pub struct HeapVector { + /// Pointer to a memory address which hold the address to the start of the items in the vector. + /// + /// That is to say, the address retrieved from the pointer doesn't need any more offsetting. pub pointer: MemoryAddress, + /// Address to a memory slot holding the semantic length of the vector. pub size: MemoryAddress, } @@ -278,22 +347,22 @@ impl std::fmt::Display for BitSize { /// /// While we are usually agnostic to how memory is passed within Brillig, /// this needs to be encoded somehow when dealing with an external system. -/// For simplicity, the extra type information is given right in the ForeignCall instructions. +/// For simplicity, the extra type information is given right in the `ForeignCall` instructions. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Copy, Hash)] #[cfg_attr(feature = "arb", derive(proptest_derive::Arbitrary))] pub enum ValueOrArray { - /// A single value passed to or from an external call + /// A single value to be passed to or from an external call. /// It is an 'immediate' value - used without dereferencing. /// For a foreign call input, the value is read directly from memory. /// For a foreign call output, the value is written directly to memory. MemoryAddress(MemoryAddress), - /// An array passed to or from an external call - /// In the case of a foreign call input, the array is read from this Brillig memory location + usize more cells. - /// In the case of a foreign call output, the array is written to this Brillig memory location with the usize being here just as a sanity check for the size write. + /// An array to be passed to or from an external call. + /// In the case of a foreign call input, the array is read from this Brillig memory location + `size` more cells. + /// In the case of a foreign call output, the array is written to this Brillig memory location with the `size` being here just as a sanity check for the write size. HeapArray(HeapArray), - /// A vector passed to or from an external call - /// In the case of a foreign call input, the vector is read from this Brillig memory location + as many cells as the 2nd address indicates. - /// In the case of a foreign call output, the vector is written to this Brillig memory location and as 'size' cells, with size being stored in the second address. + /// A vector to be passed to or from an external call. + /// In the case of a foreign call input, the vector is read from this Brillig memory location + as many cells as the second address indicates. + /// In the case of a foreign call output, the vector is written to this Brillig memory location as 'size' cells, with size being stored in the second address. HeapVector(HeapVector), } @@ -316,8 +385,8 @@ impl std::fmt::Display for ValueOrArray { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Hash)] #[cfg_attr(feature = "arb", derive(proptest_derive::Arbitrary))] pub enum BrilligOpcode { - /// Takes the fields in addresses `lhs` and `rhs` - /// Performs the specified binary operation + /// Takes the fields in addresses `lhs` and `rhs`, + /// performs the specified binary operation, /// and stores the value in the `destination` address. BinaryFieldOp { destination: MemoryAddress, @@ -325,8 +394,8 @@ pub enum BrilligOpcode { lhs: MemoryAddress, rhs: MemoryAddress, }, - /// Takes the `bit_size` size integers in addresses `lhs` and `rhs` - /// Performs the specified binary operation + /// Takes the `bit_size` size integers in addresses `lhs` and `rhs`, + /// performs the specified binary operation, /// and stores the value in the `destination` address. BinaryIntOp { destination: MemoryAddress, @@ -335,80 +404,64 @@ pub enum BrilligOpcode { lhs: MemoryAddress, rhs: MemoryAddress, }, - Not { - destination: MemoryAddress, - source: MemoryAddress, - bit_size: IntegerBitSize, - }, - Cast { - destination: MemoryAddress, - source: MemoryAddress, - bit_size: BitSize, - }, + /// Takes the value from the `source` address, inverts it, + /// and stores the value in the `destination` address. + Not { destination: MemoryAddress, source: MemoryAddress, bit_size: IntegerBitSize }, + /// Takes the value from the `source` address, + /// casts it into the type indicated by `bit_size`, + /// and stores the value in the `destination` address. + Cast { destination: MemoryAddress, source: MemoryAddress, bit_size: BitSize }, /// Sets the program counter to the value of `location` - /// If the value at `condition` is non-zero - JumpIf { - condition: MemoryAddress, - location: Label, - }, + /// if the value at `condition` is non-zero. + JumpIf { condition: MemoryAddress, location: Label }, /// Sets the program counter to the value of `location`. - Jump { - location: Label, - }, - /// Copies calldata after the offset to the specified address and length + Jump { location: Label }, + /// Copies calldata after the `offset_address` with length indicated by `size_address` + /// to the specified `destination_address`. CalldataCopy { destination_address: MemoryAddress, size_address: MemoryAddress, offset_address: MemoryAddress, }, - /// We don't support dynamic jumps or calls - /// See for reasoning - /// /// Pushes the current program counter to the call stack as to set a return location. /// Sets the program counter to the value of `location`. - Call { - location: Label, - }, + /// + /// We don't support dynamic jumps or calls; + /// see for reasoning. + Call { location: Label }, /// Stores a constant `value` with a `bit_size` in the `destination` address. - Const { - destination: MemoryAddress, - bit_size: BitSize, - value: F, - }, + Const { destination: MemoryAddress, bit_size: BitSize, value: F }, /// Reads the address from `destination_pointer`, then stores a constant `value` with a `bit_size` at that address. - IndirectConst { - destination_pointer: MemoryAddress, - bit_size: BitSize, - value: F, - }, + IndirectConst { destination_pointer: MemoryAddress, bit_size: BitSize, value: F }, /// Pops the top element from the call stack, which represents the return location, /// and sets the program counter to that value. This operation is used to return /// from a function call. Return, /// Used to get data from an outside source. - /// Also referred to as an Oracle. However, we don't use that name as - /// this is intended for things like state tree reads, and shouldn't be confused - /// with e.g. blockchain price oracles. + /// + /// Also referred to as an Oracle, intended for things like state tree reads; + /// it shouldn't be confused with e.g. blockchain price oracles. ForeignCall { - /// Interpreted by caller context, ie this will have different meanings depending on + /// Interpreted by caller context, ie. this will have different meanings depending on /// who the caller is. function: String, /// Destination addresses (may be single values or memory pointers). destinations: Vec, - /// Destination value types + /// Destination value types. destination_value_types: Vec, /// Input addresses (may be single values or memory pointers). inputs: Vec, /// Input value types (for heap allocated structures indicates how to - /// retrieve the elements) + /// retrieve the elements). input_value_types: Vec, }, /// Moves the content in the `source` address to the `destination` address. - Mov { - destination: MemoryAddress, - source: MemoryAddress, - }, - /// destination = condition > 0 ? source_a : source_b + Mov { destination: MemoryAddress, source: MemoryAddress }, + /// If the value at `condition` is non-zero, moves the content in the `source_a` + /// address to the `destination` address, otherwise moves the content from the + /// `source_b` address instead. + /// + /// `destination = condition > 0 ? source_a : source_b` ConditionalMov { destination: MemoryAddress, source_a: MemoryAddress, @@ -417,27 +470,17 @@ pub enum BrilligOpcode { }, /// Reads the `source_pointer` to obtain a memory address, then retrieves the data /// stored at that address and writes it to the `destination` address. - Load { - destination: MemoryAddress, - source_pointer: MemoryAddress, - }, + Load { destination: MemoryAddress, source_pointer: MemoryAddress }, /// Reads the `destination_pointer` to obtain a memory address, then stores the value /// from the `source` address at that location. - Store { - destination_pointer: MemoryAddress, - source: MemoryAddress, - }, + Store { destination_pointer: MemoryAddress, source: MemoryAddress }, /// Native functions in the VM. /// These are equivalent to the black box functions in ACIR. BlackBox(BlackBoxOp), /// Used to denote execution failure, halting the VM and returning data specified by a dynamically-sized vector. - Trap { - revert_data: HeapVector, - }, + Trap { revert_data: HeapVector }, /// Halts execution and returns data specified by a dynamically-sized vector. - Stop { - return_data: HeapVector, - }, + Stop { return_data: HeapVector }, } impl std::fmt::Display for BrilligOpcode { @@ -490,7 +533,7 @@ impl std::fmt::Display for BrilligOpcode { if !destinations.is_empty() { for (index, (destination, destination_value_type)) in - destinations.iter().zip(destination_value_types.iter()).enumerate() + destinations.iter().zip(destination_value_types).enumerate() { if index > 0 { write!(f, ", ")?; @@ -504,7 +547,7 @@ impl std::fmt::Display for BrilligOpcode { assert_eq!(inputs.len(), input_value_types.len()); for (index, (input, input_value_type)) in - inputs.iter().zip(input_value_types.iter()).enumerate() + inputs.iter().zip(input_value_types).enumerate() { if index > 0 { write!(f, ", ")?; @@ -551,11 +594,11 @@ pub enum BinaryFieldOp { Div, /// Unsigned integer division IntegerDiv, - /// (==) equal + /// (==) Equal Equals, /// (<) Field less than LessThan, - /// (<=) field less or equal + /// (<=) Field less or equal LessThanEquals, } @@ -582,11 +625,11 @@ pub enum BinaryIntOp { Sub, Mul, Div, - /// (==) equal + /// (==) Equal Equals, - /// (<) Field less than + /// (<) Integer less than LessThan, - /// (<=) field less or equal + /// (<=) Integer less or equal LessThanEquals, /// (&) Bitwise AND And, diff --git a/acvm-repo/brillig_vm/src/arithmetic.rs b/acvm-repo/brillig_vm/src/arithmetic.rs index 6da126f9c58..5366db1a220 100644 --- a/acvm-repo/brillig_vm/src/arithmetic.rs +++ b/acvm-repo/brillig_vm/src/arithmetic.rs @@ -27,25 +27,22 @@ pub(crate) fn evaluate_binary_field_op( lhs: MemoryValue, rhs: MemoryValue, ) -> Result, BrilligArithmeticError> { - let a = lhs.expect_field().map_err(|err| { - if let MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } = err { - BrilligArithmeticError::MismatchedLhsBitSize { - lhs_bit_size: value_bit_size, - op_bit_size: expected_bit_size, + let expect_field = |value: MemoryValue, make_err: fn(u32, u32) -> BrilligArithmeticError| { + value.expect_field().map_err(|err| { + if let MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } = err { + make_err(value_bit_size, expected_bit_size) + } else { + unreachable!("MemoryTypeError NotInteger is only produced by to_u128") } - } else { - unreachable!("MemoryTypeError NotInteger is only produced by to_u128") - } + }) + }; + let a = expect_field(lhs, |vbs, ebs| BrilligArithmeticError::MismatchedLhsBitSize { + lhs_bit_size: vbs, + op_bit_size: ebs, })?; - let b = rhs.expect_field().map_err(|err| { - if let MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } = err { - BrilligArithmeticError::MismatchedRhsBitSize { - rhs_bit_size: value_bit_size, - op_bit_size: expected_bit_size, - } - } else { - unreachable!("MemoryTypeError NotInteger is only produced by to_u128") - } + let b = expect_field(rhs, |vbs, ebs| BrilligArithmeticError::MismatchedRhsBitSize { + rhs_bit_size: vbs, + op_bit_size: ebs, })?; Ok(match op { @@ -182,51 +179,19 @@ pub(crate) fn evaluate_binary_int_op( } } (MemoryValue::U8(lhs), MemoryValue::U8(rhs), IntegerBitSize::U8) => { - if rhs < 8 { - Ok(MemoryValue::U8(evaluate_binary_int_op_shifts(op, lhs, rhs))) - } else { - Err(BrilligArithmeticError::BitshiftOverflow { - bit_size: 8, - shift_size: u128::from(rhs), - }) - } + Ok(MemoryValue::U8(evaluate_binary_int_op_shifts(op, lhs, rhs)?)) } (MemoryValue::U16(lhs), MemoryValue::U16(rhs), IntegerBitSize::U16) => { - if rhs < 16 { - Ok(MemoryValue::U16(evaluate_binary_int_op_shifts(op, lhs, rhs))) - } else { - Err(BrilligArithmeticError::BitshiftOverflow { - bit_size: 16, - shift_size: u128::from(rhs), - }) - } + Ok(MemoryValue::U16(evaluate_binary_int_op_shifts(op, lhs, rhs)?)) } (MemoryValue::U32(lhs), MemoryValue::U32(rhs), IntegerBitSize::U32) => { - if rhs < 32 { - Ok(MemoryValue::U32(evaluate_binary_int_op_shifts(op, lhs, rhs))) - } else { - Err(BrilligArithmeticError::BitshiftOverflow { - bit_size: 32, - shift_size: u128::from(rhs), - }) - } + Ok(MemoryValue::U32(evaluate_binary_int_op_shifts(op, lhs, rhs)?)) } (MemoryValue::U64(lhs), MemoryValue::U64(rhs), IntegerBitSize::U64) => { - if rhs < 64 { - Ok(MemoryValue::U64(evaluate_binary_int_op_shifts(op, lhs, rhs))) - } else { - Err(BrilligArithmeticError::BitshiftOverflow { - bit_size: 64, - shift_size: u128::from(rhs), - }) - } + Ok(MemoryValue::U64(evaluate_binary_int_op_shifts(op, lhs, rhs)?)) } (MemoryValue::U128(lhs), MemoryValue::U128(rhs), IntegerBitSize::U128) => { - if rhs < 128 { - Ok(MemoryValue::U128(evaluate_binary_int_op_shifts(op, lhs, rhs))) - } else { - Err(BrilligArithmeticError::BitshiftOverflow { bit_size: 128, shift_size: rhs }) - } + Ok(MemoryValue::U128(evaluate_binary_int_op_shifts(op, lhs, rhs)?)) } _ => Err(BrilligArithmeticError::MismatchedLhsBitSize { lhs_bit_size: lhs.bit_size().to_u32::(), @@ -243,8 +208,8 @@ pub(crate) fn evaluate_binary_int_op( /// - Err([BrilligArithmeticError::DivisionByZero]) if division by zero occurs. /// /// # Panics -/// If an operation other than Add, Sub, Mul, Div, And, Or, Xor, Equals, LessThan, -/// or LessThanEquals is supplied as an argument. +/// If an operation other than `Add`, `Sub`, `Mul`, `Div`, `And`, `Or`, `Xor`, `Equals`, `LessThan`, +/// or `LessThanEquals` is supplied as an argument. fn evaluate_binary_int_op_u1( op: &BinaryIntOp, lhs: bool, @@ -269,11 +234,11 @@ fn evaluate_binary_int_op_u1( Ok(result) } -/// Evaluates comparison operations (Equals, LessThan, LessThanEquals) +/// Evaluates comparison operations (`Equals`, `LessThan`, `LessThanEquals`) /// between two values of an ordered type (e.g., fields are unordered). /// /// # Panics -/// If an unsupported operator is provided (i.e., not Equals, LessThan, or LessThanEquals). +/// If an unsupported operator is provided (i.e., not `Equals`, `LessThan`, or `LessThanEquals`). fn evaluate_binary_int_op_cmp(op: &BinaryIntOp, lhs: T, rhs: T) -> bool { match op { BinaryIntOp::Equals => lhs == rhs, @@ -283,25 +248,33 @@ fn evaluate_binary_int_op_cmp(op: &BinaryIntOp, lhs: T, rhs: } } -/// Evaluates shift operations (Shl, Shr) for unsigned integers. +/// Evaluates shift operations (`Shl`, `Shr`) for unsigned integers. /// Ensures that shifting beyond the type width returns zero. /// +/// # Returns +/// - Ok(result) if successful. +/// - Err([BrilligArithmeticError::DivisionByZero]) if the RHS is not less than the bit size. +/// /// # Panics -/// If an unsupported operator is provided (i.e., not Shl or Shr). -fn evaluate_binary_int_op_shifts + Shr>( +/// If an unsupported operator is provided (i.e., not `Shl` or `Shr`). +fn evaluate_binary_int_op_shifts< + T: ToPrimitive + Zero + Shl + Shr + Into, +>( op: &BinaryIntOp, lhs: T, rhs: T, -) -> T { +) -> Result { + let bit_size = 8 * size_of::(); + let rhs_usize: usize = rhs.to_usize().expect("Could not convert rhs to usize"); + if rhs_usize >= bit_size { + return Err(BrilligArithmeticError::BitshiftOverflow { + bit_size: bit_size as u32, + shift_size: rhs.into(), + }); + } match op { - BinaryIntOp::Shl => { - let rhs_usize: usize = rhs.to_usize().expect("Could not convert rhs to usize"); - if rhs_usize >= 8 * size_of::() { T::zero() } else { lhs << rhs } - } - BinaryIntOp::Shr => { - let rhs_usize: usize = rhs.to_usize().expect("Could not convert rhs to usize"); - if rhs_usize >= 8 * size_of::() { T::zero() } else { lhs >> rhs } - } + BinaryIntOp::Shl => Ok(lhs << rhs), + BinaryIntOp::Shr => Ok(lhs >> rhs), _ => unreachable!("Operator not handled by this function: {op:?}"), } } diff --git a/acvm-repo/brillig_vm/src/black_box.rs b/acvm-repo/brillig_vm/src/black_box.rs index 2547cf80a98..da8f0e0cafd 100644 --- a/acvm-repo/brillig_vm/src/black_box.rs +++ b/acvm-repo/brillig_vm/src/black_box.rs @@ -12,20 +12,47 @@ use crate::Memory; use crate::memory::MemoryValue; /// Reads a dynamically-sized [vector][HeapVector] from memory. +/// +/// The data is not expected to contain pointers to nested arrays or vector. fn read_heap_vector<'a, F: AcirField>( memory: &'a Memory, vector: &HeapVector, ) -> &'a [MemoryValue] { + let items_start = memory.read_ref(vector.pointer); let size = memory.read(vector.size); - memory.read_slice(memory.read_ref(vector.pointer), size.to_usize()) + memory.read_slice(items_start, size.to_usize()) +} + +/// Write values to a [vector][HeapVector] in memory. +fn write_heap_vector( + memory: &mut Memory, + vector: &HeapVector, + values: &[MemoryValue], +) { + let items_start = memory.read_ref(vector.pointer); + memory.write(vector.size, values.len().into()); + memory.write_slice(items_start, values); } /// Reads a fixed-size [array][HeapArray] from memory. +/// +/// The data is not expected to contain pointers to nested arrays or vector. fn read_heap_array<'a, F: AcirField>( memory: &'a Memory, array: &HeapArray, ) -> &'a [MemoryValue] { - memory.read_slice(memory.read_ref(array.pointer), array.size) + let items_start = memory.read_ref(array.pointer); + memory.read_slice(items_start, array.size) +} + +/// Write values to a [array][HeapArray] in memory. +fn write_heap_array( + memory: &mut Memory, + array: &HeapArray, + values: &[MemoryValue], +) { + let items_start = memory.read_ref(array.pointer); + memory.write_slice(items_start, values); } /// Extracts the last byte of every value @@ -82,21 +109,20 @@ pub(crate) fn evaluate_black_box })?; let ciphertext = aes128_encrypt(&inputs, iv, key)?; - memory.write(outputs.size, ciphertext.len().into()); - memory.write_slice(memory.read_ref(outputs.pointer), &to_value_vec(&ciphertext)); + write_heap_vector(memory, outputs, &to_value_vec(&ciphertext)); Ok(()) } BlackBoxOp::Blake2s { message, output } => { let message = to_u8_vec(read_heap_vector(memory, message)); let bytes = blake2s(message.as_slice())?; - memory.write_slice(memory.read_ref(output.pointer), &to_value_vec(&bytes)); + write_heap_array(memory, output, &to_value_vec(&bytes)); Ok(()) } BlackBoxOp::Blake3 { message, output } => { let message = to_u8_vec(read_heap_vector(memory, message)); let bytes = blake3(message.as_slice())?; - memory.write_slice(memory.read_ref(output.pointer), &to_value_vec(&bytes)); + write_heap_array(memory, output, &to_value_vec(&bytes)); Ok(()) } BlackBoxOp::Keccakf1600 { input, output } => { @@ -109,7 +135,7 @@ pub(crate) fn evaluate_black_box let new_state = keccakf1600(state)?; let new_state: Vec> = new_state.into_iter().map(|x| x.into()).collect(); - memory.write_slice(memory.read_ref(output.pointer), &new_state); + write_heap_array(memory, output, &new_state); Ok(()) } BlackBoxOp::EcdsaSecp256k1 { @@ -195,8 +221,9 @@ pub(crate) fn evaluate_black_box } } let (x, y, is_infinite) = solver.multi_scalar_mul(&points, &scalars_lo, &scalars_hi)?; - memory.write_slice( - memory.read_ref(result.pointer), + write_heap_array( + memory, + result, &[ MemoryValue::new_field(x), MemoryValue::new_field(y), @@ -228,8 +255,10 @@ pub(crate) fn evaluate_black_box &input2_y, &input2_infinite.into(), )?; - memory.write_slice( - memory.read_ref(result.pointer), + + write_heap_array( + memory, + result, &[ MemoryValue::new_field(x), MemoryValue::new_field(y), @@ -246,7 +275,7 @@ pub(crate) fn evaluate_black_box for i in result { values.push(MemoryValue::new_field(i)); } - memory.write_slice(memory.read_ref(output.pointer), &values); + write_heap_array(memory, output, &values); Ok(()) } BlackBoxOp::Sha256Compression { input, hash_values, output } => { @@ -276,7 +305,7 @@ pub(crate) fn evaluate_black_box sha256_compression(&mut state, &message); let state = state.map(|x| x.into()); - memory.write_slice(memory.read_ref(output.pointer), &state); + write_heap_array(memory, output, &state); Ok(()) } BlackBoxOp::ToRadix { input, radix, output_pointer, num_limbs, output_bits } => { diff --git a/acvm-repo/brillig_vm/src/foreign_call.rs b/acvm-repo/brillig_vm/src/foreign_call.rs index 9c35c05a413..a4b9d5c30c3 100644 --- a/acvm-repo/brillig_vm/src/foreign_call.rs +++ b/acvm-repo/brillig_vm/src/foreign_call.rs @@ -2,8 +2,8 @@ use acir::{ AcirField, brillig::{ - BitSize, ForeignCallParam, HeapArray, HeapValueType, HeapVector, IntegerBitSize, - MemoryAddress, ValueOrArray, + ArrayAddress, BitSize, ForeignCallParam, HeapArray, HeapValueType, HeapVector, + IntegerBitSize, MemoryAddress, ValueOrArray, VectorAddress, }, }; use acvm_blackbox_solver::BlackBoxFunctionSolver; @@ -46,11 +46,11 @@ impl> VM<'_, F, B> { destination_value_types: &[HeapValueType], inputs: &[ValueOrArray], input_value_types: &[HeapValueType], - ) -> VMStatus { + ) -> &VMStatus { assert_eq!(inputs.len(), input_value_types.len()); assert_eq!(destinations.len(), destination_value_types.len()); - if self.foreign_call_counter >= self.foreign_call_results.len() { + if !self.has_unprocessed_foreign_call_result() { // When this opcode is called, it is possible that the results of a foreign call are // not yet known (not enough entries in `foreign_call_results`). // If that is the case, just resolve the inputs and pause the VM with a status @@ -115,6 +115,7 @@ impl> VM<'_, F, B> { return self.fail(e); } + // Mark the foreign call result as processed. self.foreign_call_counter += 1; self.increment_program_counter() } @@ -126,14 +127,14 @@ impl> VM<'_, F, B> { value_type: &HeapValueType, ) -> ForeignCallParam { match (input, value_type) { - (ValueOrArray::MemoryAddress(value_index), HeapValueType::Simple(_)) => { - ForeignCallParam::Single(self.memory.read(value_index).to_field()) + (ValueOrArray::MemoryAddress(value_addr), HeapValueType::Simple(_)) => { + ForeignCallParam::Single(self.memory.read(value_addr).to_field()) } ( - ValueOrArray::HeapArray(HeapArray { pointer: pointer_index, size }), + ValueOrArray::HeapArray(HeapArray { pointer, size }), HeapValueType::Array { value_types, size: type_size }, ) if *type_size == size => { - let start = self.memory.read_ref(pointer_index); + let start = self.memory.read_ref(pointer); self.read_slice_of_values_from_memory(start, size, value_types) .into_iter() .map(|mem_value| mem_value.to_field()) @@ -141,11 +142,11 @@ impl> VM<'_, F, B> { .into() } ( - ValueOrArray::HeapVector(HeapVector { pointer: pointer_index, size: size_index }), + ValueOrArray::HeapVector(HeapVector { pointer, size: size_addr }), HeapValueType::Vector { value_types }, ) => { - let start = self.memory.read_ref(pointer_index); - let size = self.memory.read(size_index).to_usize(); + let start = self.memory.read_ref(pointer); + let size = self.memory.read(size_addr).to_usize(); self.read_slice_of_values_from_memory(start, size, value_types) .into_iter() .map(|mem_value| mem_value.to_field()) @@ -166,7 +167,7 @@ impl> VM<'_, F, B> { size: usize, value_types: &[HeapValueType], ) -> Vec> { - assert!(!start.is_relative(), "read_slice_of_values_from_memory requires direct addresses"); + assert!(start.is_direct(), "read_slice_of_values_from_memory requires direct addresses"); if HeapValueType::all_simple(value_types) { self.memory.read_slice(start, size).to_vec() } else { @@ -189,20 +190,22 @@ impl> VM<'_, F, B> { vec![self.memory.read(value_address)] } HeapValueType::Array { value_types, size } => { - let array_address = self.memory.read_ref(value_address); + let array_address = + ArrayAddress::from(self.memory.read_ref(value_address)); self.read_slice_of_values_from_memory( - array_address.offset(1), + array_address.items_start(), *size, value_types, ) } HeapValueType::Vector { value_types } => { - let vector_address = self.memory.read_ref(value_address); - let size_address = - MemoryAddress::direct(vector_address.unwrap_direct() + 1); - let items_start = vector_address.offset(2); - let vector_size = self.memory.read(size_address).to_usize(); + let vector_address = + VectorAddress::from(self.memory.read_ref(value_address)); + + let side_addr = vector_address.size_addr(); + let items_start = vector_address.items_start(); + let vector_size = self.memory.read(side_addr).to_usize(); self.read_slice_of_values_from_memory( items_start, vector_size, @@ -241,7 +244,7 @@ impl> VM<'_, F, B> { &mut self, function: String, inputs: Vec>, - ) -> VMStatus { + ) -> &VMStatus { self.status(VMStatus::ForeignCallWait { function, inputs }) } @@ -259,6 +262,7 @@ impl> VM<'_, F, B> { destination_value_types: &[HeapValueType], foreign_call_index: usize, ) -> Result<(), String> { + // Take ownership of values to allow calling mutating methods on self. let values = std::mem::take(&mut self.foreign_call_results[foreign_call_index].values); if destinations.len() != values.len() { @@ -274,16 +278,12 @@ impl> VM<'_, F, B> { destination_value_types.len(), "Number of destinations must match number of value types", ); - debug_assert_eq!( - destinations.len(), - values.len(), - "Number of foreign call return values must match number of destinations", - ); + for ((destination, value_type), output) in destinations.iter().zip(destination_value_types).zip(&values) { match (destination, value_type) { - (ValueOrArray::MemoryAddress(value_index), HeapValueType::Simple(bit_size)) => { + (ValueOrArray::MemoryAddress(value_addr), HeapValueType::Simple(bit_size)) => { let output_fields = output.fields(); if value_type .flattened_size() @@ -298,7 +298,7 @@ impl> VM<'_, F, B> { match output { ForeignCallParam::Single(value) => { - self.write_value_to_memory(*value_index, value, *bit_size)?; + self.write_value_to_memory(*value_addr, value, *bit_size)?; } _ => { return Err(format!( @@ -308,9 +308,14 @@ impl> VM<'_, F, B> { } } ( - ValueOrArray::HeapArray(HeapArray { pointer: pointer_index, size }), + ValueOrArray::HeapArray(HeapArray { pointer, size }), HeapValueType::Array { value_types, size: type_size }, - ) if size == type_size => { + ) => { + if size != type_size { + return Err(format!( + "Destination array size of {size} does not match the type size of {type_size}" + )); + } let output_fields = output.fields(); if value_type .flattened_size() @@ -324,50 +329,39 @@ impl> VM<'_, F, B> { } if HeapValueType::all_simple(value_types) { - match output { - ForeignCallParam::Array(values) => { - if values.len() != *size { - // foreign call returning flattened values into a nested type, so the sizes do not match - let destination = self.memory.read_ref(*pointer_index); - - let mut flatten_values_idx = 0; //index of values read from flatten_values - self.write_slice_of_values_to_memory( - destination, - &output_fields, - &mut flatten_values_idx, - value_type, - )?; - // Should be caught earlier but we want to be explicit. - debug_assert_eq!( - flatten_values_idx, - output_fields.len(), - "Not all values were written to memory" - ); - } else { - self.write_values_to_memory_slice( - *pointer_index, - values, - value_types, - )?; - } - } - _ => { - return Err( - "Function result size does not match brillig bytecode size" - .to_string(), - ); - } + let ForeignCallParam::Array(values) = output else { + return Err("Foreign call returned a single value for an array type" + .to_string()); + }; + if values.len() != *size { + // foreign call returning flattened values into a nested type, so the sizes do not match + let destination = self.memory.read_ref(*pointer); + + let mut flatten_values_idx = 0; //index of values read from flatten_values + self.write_flattened_values_to_memory( + destination, + &output_fields, + &mut flatten_values_idx, + value_type, + )?; + // Should be caught earlier but we want to be explicit. + debug_assert_eq!( + flatten_values_idx, + output_fields.len(), + "Not all values were written to memory" + ); + } else { + self.write_values_to_memory(*pointer, values, value_types)?; } } else { // foreign call returning flattened values into a nested type, so the sizes do not match - let destination = self.memory.read_ref(*pointer_index); - let return_type = value_type; + let destination = self.memory.read_ref(*pointer); let mut flatten_values_idx = 0; //index of values read from flatten_values - self.write_slice_of_values_to_memory( + self.write_flattened_values_to_memory( destination, &output_fields, &mut flatten_values_idx, - return_type, + value_type, )?; debug_assert_eq!( flatten_values_idx, @@ -377,34 +371,22 @@ impl> VM<'_, F, B> { } } ( - ValueOrArray::HeapVector(HeapVector { - pointer: pointer_index, - size: size_index, - }), + ValueOrArray::HeapVector(HeapVector { pointer, size: size_addr }), HeapValueType::Vector { value_types }, ) => { if HeapValueType::all_simple(value_types) { - match output { - ForeignCallParam::Array(values) => { - if values.len() % value_types.len() != 0 { - return Err("Returned data does not match vector element size" - .to_string()); - } - // Set our size in the size address - self.memory.write(*size_index, values.len().into()); - self.write_values_to_memory_slice( - *pointer_index, - values, - value_types, - )?; - } - _ => { - return Err( - "Function result size does not match brillig bytecode size" - .to_string(), - ); - } + let ForeignCallParam::Array(values) = output else { + return Err("Foreign call returned a single value for an vector type" + .to_string()); + }; + if values.len() % value_types.len() != 0 { + return Err( + "Returned data does not match vector element size".to_string() + ); } + // Set the size in the size address + self.memory.write(*size_addr, values.len().into()); + self.write_values_to_memory(*pointer, values, value_types)?; } else { unimplemented!("deflattening heap vectors from foreign calls"); } @@ -422,6 +404,7 @@ impl> VM<'_, F, B> { Ok(()) } + /// Write a single numeric value to the destination address, ensuring that the bit size matches the expectation. fn write_value_to_memory( &mut self, destination: MemoryAddress, @@ -440,9 +423,10 @@ impl> VM<'_, F, B> { Ok(()) } - fn write_values_to_memory_slice( + /// Write an array or slice to the destination under the pointer. + fn write_values_to_memory( &mut self, - pointer_index: MemoryAddress, + pointer: MemoryAddress, values: &[F], value_types: &[HeapValueType], ) -> Result<(), String> { @@ -454,9 +438,9 @@ impl> VM<'_, F, B> { }) .cycle(); - // Convert the destination pointer to a usize - let destination = self.memory.read_ref(pointer_index); - // Write to our destination memory + // Convert the destination pointer to an address. + let destination = self.memory.read_ref(pointer); + // Write to the destination memory. let memory_values: Option> = values .iter() .zip(bit_sizes_iterator) @@ -472,12 +456,12 @@ impl> VM<'_, F, B> { Ok(()) } - /// Writes flattened values to memory, using the provided type - /// Function calls itself recursively in order to work with recursive types (nested arrays) - /// values_idx is the current index in the values vector and is incremented every time - /// a value is written to memory - /// The function returns the address of the next value to be written - fn write_slice_of_values_to_memory( + /// Writes flattened values to memory, using the provided type. + /// + /// The method calls itself recursively in order to work with recursive types (nested arrays). + /// `values_idx` is the current index in the `values` vector and is incremented every time + /// a value is written to memory. + fn write_flattened_values_to_memory( &mut self, destination: MemoryAddress, values: &Vec, @@ -485,10 +469,9 @@ impl> VM<'_, F, B> { value_type: &HeapValueType, ) -> Result<(), String> { assert!( - !destination.is_relative(), - "write_slice_of_values_to_memory requires direct addresses" + destination.is_direct(), + "write_flattened_values_to_memory requires direct addresses" ); - let mut current_pointer = destination; match value_type { HeapValueType::Simple(bit_size) => { self.write_value_to_memory(destination, &values[*values_idx], *bit_size)?; @@ -496,26 +479,32 @@ impl> VM<'_, F, B> { Ok(()) } HeapValueType::Array { value_types, size } => { + let mut current_pointer = destination; for _ in 0..*size { for typ in value_types { match typ { - HeapValueType::Simple(len) => { + HeapValueType::Simple(bit_size) => { self.write_value_to_memory( current_pointer, &values[*values_idx], - *len, + *bit_size, )?; *values_idx += 1; current_pointer = current_pointer.offset(1); } HeapValueType::Array { .. } => { - let destination = self.memory.read_ref(current_pointer).offset(1); - self.write_slice_of_values_to_memory( - destination, + // The next memory destination is an array, somewhere else in memory where the pointer points to. + let destination = + ArrayAddress::from(self.memory.read_ref(current_pointer)); + + self.write_flattened_values_to_memory( + destination.items_start(), values, values_idx, typ, )?; + + // Move on to the next slot in *this* array. current_pointer = current_pointer.offset(1); } HeapValueType::Vector { .. } => { diff --git a/acvm-repo/brillig_vm/src/fuzzing.rs b/acvm-repo/brillig_vm/src/fuzzing.rs index 54fd3157642..306a9170090 100644 --- a/acvm-repo/brillig_vm/src/fuzzing.rs +++ b/acvm-repo/brillig_vm/src/fuzzing.rs @@ -27,14 +27,25 @@ pub type BranchToFeatureMap = HashMap; /// Context structure for all information necessary to compute the fuzzing trace #[derive(Debug, PartialEq, Eq, Clone, Default)] pub(super) struct FuzzingTrace { - /// Fuzzer tracing memory ddd + /// Fuzzer tracing memory. + /// + /// It records each time a key in the `branch_to_feature_map` was observed. + /// Its length is equal to that of the `branch_to_feature_map`. trace: Vec, - /// Branch to feature map for fuzzing - /// Maps program counter + feature to index in the trace vector + /// Branch to feature map for fuzzing. + /// Maps program counter + feature to index in the trace vector. branch_to_feature_map: HashMap<(usize, usize), usize>, } impl FuzzingTrace { + fn branch_state(cond: bool) -> usize { + if cond { FUZZING_COMPARISON_TRUE_STATE } else { FUZZING_COMPARISON_FALSE_STATE } + } + + fn log_range_state(log: usize) -> usize { + FUZZING_COMPARISON_LOG_RANGE_START_STATE + log + } + pub(super) fn new(branch_to_feature_map: HashMap<(usize, usize), usize>) -> Self { let len = branch_to_feature_map.len(); Self { trace: vec![0; len], branch_to_feature_map } @@ -46,10 +57,7 @@ impl FuzzingTrace { } fn record_conditional_mov(&mut self, pc: usize, branch: bool) { - let index = self.branch_to_feature_map[&( - pc, - if branch { FUZZING_COMPARISON_TRUE_STATE } else { FUZZING_COMPARISON_FALSE_STATE }, - )]; + let index = self.branch_to_feature_map[&(pc, Self::branch_state(branch))]; self.trace[index] += 1; } @@ -62,37 +70,33 @@ impl FuzzingTrace { result: MemoryValue, ) { match op { - BinaryFieldOp::Add - | BinaryFieldOp::Sub - | BinaryFieldOp::Mul - | BinaryFieldOp::Div - | BinaryFieldOp::IntegerDiv => {} BinaryFieldOp::Equals | BinaryFieldOp::LessThan | BinaryFieldOp::LessThanEquals => { - let a = match lhs { - MemoryValue::Field(a) => a, - _ => return, + let MemoryValue::Field(a) = lhs else { + return; }; - let b = match rhs { - MemoryValue::Field(b) => b, - _ => return, + let MemoryValue::Field(b) = rhs else { + return; }; - let c = match result { - MemoryValue::U1(value) => value, - _ => return, + let MemoryValue::U1(c) = result else { + return; }; - let approach_index = self.branch_to_feature_map[&( - pc, - FUZZING_COMPARISON_LOG_RANGE_START_STATE - + BigUint::from_bytes_be(&(b - a).to_be_bytes()).bits() as usize, - )]; - let condition_index = self.branch_to_feature_map[&( - pc, - if c { FUZZING_COMPARISON_TRUE_STATE } else { FUZZING_COMPARISON_FALSE_STATE }, - )]; + // Logarithm of the difference between LHS as RHS as the number of bits required to represent its value: + let diff_log = BigUint::from_bytes_be(&(b - a).to_be_bytes()).bits(); + + let approach_index = + self.branch_to_feature_map[&(pc, Self::log_range_state(diff_log as usize))]; + + let condition_index = self.branch_to_feature_map[&(pc, Self::branch_state(c))]; + self.trace[approach_index] += 1; self.trace[condition_index] += 1; } + BinaryFieldOp::Add + | BinaryFieldOp::Sub + | BinaryFieldOp::Mul + | BinaryFieldOp::Div + | BinaryFieldOp::IntegerDiv => {} } } @@ -105,15 +109,6 @@ impl FuzzingTrace { result: MemoryValue, ) { match op { - BinaryIntOp::Add - | BinaryIntOp::Sub - | BinaryIntOp::Mul - | BinaryIntOp::Div - | BinaryIntOp::And - | BinaryIntOp::Or - | BinaryIntOp::Xor - | BinaryIntOp::Shl - | BinaryIntOp::Shr => {} BinaryIntOp::Equals | BinaryIntOp::LessThan | BinaryIntOp::LessThanEquals => { let lhs_val = lhs.to_u128().expect("lhs is not an integer"); let rhs_val = rhs.to_u128().expect("rhs is not an integer"); @@ -122,19 +117,26 @@ impl FuzzingTrace { _ => return, }; - let approach_index = self.branch_to_feature_map[&( - pc, - FUZZING_COMPARISON_LOG_RANGE_START_STATE - + rhs_val.abs_diff(lhs_val).checked_ilog2().map_or_else(|| 0, |x| x + 1) - as usize, - )]; - let condition_index = self.branch_to_feature_map[&( - pc, - if c { FUZZING_COMPARISON_TRUE_STATE } else { FUZZING_COMPARISON_FALSE_STATE }, - )]; + let diff_log = + rhs_val.abs_diff(lhs_val).checked_ilog2().map_or_else(|| 0, |x| x + 1); + + let approach_index = + self.branch_to_feature_map[&(pc, Self::log_range_state(diff_log as usize))]; + + let condition_index = self.branch_to_feature_map[&(pc, Self::branch_state(c))]; + self.trace[approach_index] += 1; self.trace[condition_index] += 1; } + BinaryIntOp::Add + | BinaryIntOp::Sub + | BinaryIntOp::Mul + | BinaryIntOp::Div + | BinaryIntOp::And + | BinaryIntOp::Or + | BinaryIntOp::Xor + | BinaryIntOp::Shl + | BinaryIntOp::Shr => {} } } @@ -152,7 +154,7 @@ impl> VM<'_, F, B> { rhs: MemoryValue, result: MemoryValue, ) { - if let Some(trace) = &mut self.fuzzing_trace { + if let Some(ref mut trace) = self.fuzzing_trace { trace.record_binary_field_op_comparison(self.program_counter, op, lhs, rhs, result); } } @@ -165,21 +167,21 @@ impl> VM<'_, F, B> { rhs: MemoryValue, result: MemoryValue, ) { - if let Some(trace) = &mut self.fuzzing_trace { + if let Some(ref mut trace) = self.fuzzing_trace { trace.record_binary_int_op_comparison(self.program_counter, op, lhs, rhs, result); } } /// Mark the execution of a particular branch in the fuzzing trace pub(super) fn fuzzing_trace_branching(&mut self, destination: NextOpcodePositionOrState) { - if let Some(trace) = &mut self.fuzzing_trace { + if let Some(ref mut trace) = self.fuzzing_trace { trace.record_branch(self.program_counter, destination); } } /// Mark the execution of a conditional move in the fuzzing trace pub(super) fn fuzzing_trace_conditional_mov(&mut self, branch: bool) { - if let Some(trace) = &mut self.fuzzing_trace { + if let Some(ref mut trace) = self.fuzzing_trace { trace.record_conditional_mov(self.program_counter, branch); } } diff --git a/acvm-repo/brillig_vm/src/lib.rs b/acvm-repo/brillig_vm/src/lib.rs index e1bcfda739c..8d84f3d8227 100644 --- a/acvm-repo/brillig_vm/src/lib.rs +++ b/acvm-repo/brillig_vm/src/lib.rs @@ -19,7 +19,7 @@ use black_box::evaluate_black_box; // Re-export `brillig`. pub use acir::brillig; use memory::MemoryTypeError; -pub use memory::{MEMORY_ADDRESSING_BIT_SIZE, Memory, MemoryValue}; +pub use memory::{MEMORY_ADDRESSING_BIT_SIZE, Memory, MemoryValue, STACK_POINTER_ADDRESS}; pub use crate::fuzzing::BranchToFeatureMap; use crate::fuzzing::FuzzingTrace; @@ -81,65 +81,72 @@ pub enum VMStatus { /// and update the Brillig process. The VM can then be restarted to fully solve the previously /// unresolved foreign call as well as the remaining Brillig opcodes. ForeignCallWait { - /// Interpreted by simulator context + /// Interpreted by simulator context. function: String, - /// Input values - /// Each input is a list of values as an input can be either a single value or a memory pointer + /// Input values. + /// Each input can be either a single value or an array of values read from a memory pointer. inputs: Vec>, }, } -/// All samples for each opcode that was executed -pub type BrilligProfilingSamples = Vec; - -/// The position of an opcode that is currently being executed in the bytecode +/// The position of an opcode that is currently being executed in the bytecode. pub type OpcodePosition = usize; -/// The position of the next opcode that will be executed in the bytecode or an id of a specific state produced by the opcode +/// The position of the next opcode that will be executed in the bytecode, +/// or an id of a specific state produced by the opcode. pub type NextOpcodePositionOrState = usize; -/// A sample for an executed opcode +/// A sample for an executed opcode. #[derive(Debug, PartialEq, Eq, Clone)] pub struct BrilligProfilingSample { /// The call stack when processing a given opcode. pub call_stack: Vec, } +/// All samples for each opcode that was executed. +pub type BrilligProfilingSamples = Vec; + #[derive(Debug, PartialEq, Eq, Clone)] /// VM encapsulates the state of the Brillig VM during execution. pub struct VM<'a, F, B: BlackBoxFunctionSolver> { - /// Calldata to the brillig function + /// Calldata to the brillig function. calldata: Vec, - /// Instruction pointer + /// Instruction pointer. program_counter: usize, /// A counter maintained throughout a Brillig process that determines /// whether the caller has resolved the results of a [foreign call][Opcode::ForeignCall]. + /// + /// Incremented when the results of a foreign call have been processed and the output + /// values were written to memory. + /// + /// * When the counter is less than the length of the results, it indicates that we have + /// unprocessed responses returned from the external foreign call handler. foreign_call_counter: usize, - /// Represents the outputs of all foreign calls during a Brillig process - /// List is appended onto by the caller upon reaching a [VMStatus::ForeignCallWait] + /// Accumulates the outputs of all foreign calls during a Brillig process. + /// The list is appended onto by the caller upon reaching a [VMStatus::ForeignCallWait]. foreign_call_results: Vec>, - /// Executable opcodes + /// Executable opcodes. bytecode: &'a [Opcode], - /// Status of the VM + /// Status of the VM. status: VMStatus, - /// Memory of the VM + /// Memory of the VM. memory: Memory, - /// Call stack + /// Call stack. call_stack: Vec, - /// The solver for blackbox functions + /// The solver for blackbox functions. black_box_solver: &'a B, // Flag that determines whether we want to profile VM. profiling_active: bool, // Samples for profiling the VM execution. profiling_samples: BrilligProfilingSamples, - /// Fuzzing trace structure - /// If the field is `None` then fuzzing is inactive + /// Fuzzing trace structure. + /// If the field is `None` then fuzzing is inactive. fuzzing_trace: Option, } impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { - /// Constructs a new VM instance + /// Constructs a new VM instance. pub fn new( calldata: Vec, bytecode: &'a [Opcode], @@ -147,7 +154,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { profiling_active: bool, with_branch_to_feature_map: Option<&BranchToFeatureMap>, ) -> Self { - let fuzzing_trace = with_branch_to_feature_map.map(|map| FuzzingTrace::new(map.clone())); + let fuzzing_trace = with_branch_to_feature_map.cloned().map(FuzzingTrace::new); Self { calldata, @@ -179,9 +186,9 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { /// Updates the current status of the VM. /// Returns the given status. - fn status(&mut self, status: VMStatus) -> VMStatus { + fn status(&mut self, status: VMStatus) -> &VMStatus { self.status = status.clone(); - status + &self.status } pub fn get_status(&self) -> VMStatus { @@ -189,46 +196,45 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { } /// Sets the current status of the VM to Finished (completed execution). - fn finish(&mut self, return_data_offset: usize, return_data_size: usize) -> VMStatus { + fn finish(&mut self, return_data_offset: usize, return_data_size: usize) -> &VMStatus { self.status(VMStatus::Finished { return_data_offset, return_data_size }) } + /// Check whether the latest foreign call result is available yet. + fn has_unprocessed_foreign_call_result(&self) -> bool { + self.foreign_call_counter < self.foreign_call_results.len() + } + /// Provide the results of a Foreign Call to the VM /// and resume execution of the VM. pub fn resolve_foreign_call(&mut self, foreign_call_result: ForeignCallResult) { - if self.foreign_call_counter < self.foreign_call_results.len() { - panic!("No unresolved foreign calls"); + if self.has_unprocessed_foreign_call_result() { + panic!("No unresolved foreign calls; the previous results haven't been processed yet"); } self.foreign_call_results.push(foreign_call_result); self.status(VMStatus::InProgress); } - fn get_error_stack(&self) -> Vec { - let mut error_stack: Vec<_> = self.call_stack.clone(); - error_stack.push(self.program_counter); - error_stack - } - - /// Sets the current status of the VM to `fail`. - /// Indicating that the VM encountered a `Trap` Opcode - /// or an invalid state. - fn trap(&mut self, revert_data_offset: usize, revert_data_size: usize) -> VMStatus { + /// Sets the current status of the VM to `Failure`, + /// indicating that the VM encountered a `Trap` Opcode. + fn trap(&mut self, revert_data_offset: usize, revert_data_size: usize) -> &VMStatus { self.status(VMStatus::Failure { - call_stack: self.get_error_stack(), + call_stack: self.get_call_stack(), reason: FailureReason::Trap { revert_data_offset, revert_data_size }, - }); - self.status.clone() + }) } - fn fail(&mut self, message: String) -> VMStatus { + /// Sets the current status of the VM to `Failure`, + /// indicating that the VM encountered an invalid state. + fn fail(&mut self, message: String) -> &VMStatus { self.status(VMStatus::Failure { - call_stack: self.get_error_stack(), + call_stack: self.get_call_stack(), reason: FailureReason::RuntimeError { message }, - }); - self.status.clone() + }) } - /// Loop over the bytecode and update the program counter + /// Process opcodes in a loop until a status of `Finished`, + /// `Failure` or `ForeignCallWait` is encountered. pub fn process_opcodes(&mut self) -> VMStatus { while !matches!( self.process_opcode(), @@ -237,10 +243,16 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { self.status.clone() } + /// Read memory slots. + /// + /// Used by the debugger to inspect the contents of the memory. pub fn get_memory(&self) -> &[MemoryValue] { self.memory.values() } + /// Take all the contents of the memory, leaving it empty. + /// + /// Used only for testing purposes. pub fn take_memory(mut self) -> Memory { std::mem::take(&mut self.memory) } @@ -249,6 +261,9 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { self.foreign_call_counter } + /// Write a numeric value to direct memory slot. + /// + /// Used by the debugger to alter memory. pub fn write_memory_at(&mut self, ptr: usize, value: MemoryValue) { self.memory.write(MemoryAddress::direct(ptr), value); } @@ -256,10 +271,12 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { /// Returns the VM's current call stack, including the actual program /// counter in the last position of the returned vector. pub fn get_call_stack(&self) -> Vec { - self.call_stack.iter().copied().chain(std::iter::once(self.program_counter)).collect() + let mut call_stack = self.get_call_stack_no_current_counter(); + call_stack.push(self.program_counter); + call_stack } - /// Returns the VM's call stack but unlike [Self::get_call_stack] without the attaching + /// Returns the VM's call stack, but unlike [Self::get_call_stack] without the attaching /// the program counter in the last position of the returned vector. /// This is meant only for fetching the call stack after execution has completed. pub fn get_call_stack_no_current_counter(&self) -> Vec { @@ -267,7 +284,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { } /// Process a single opcode and modify the program counter. - pub fn process_opcode(&mut self) -> VMStatus { + pub fn process_opcode(&mut self) -> &VMStatus { if self.profiling_active { let call_stack: Vec = self.get_call_stack(); self.profiling_samples.push(BrilligProfilingSample { call_stack }); @@ -283,15 +300,15 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { /// Execute a single opcode: /// 1. Retrieve the current opcode using the program counter /// 2. Execute the opcode. - /// - For instance a binary 'result = lhs+rhs' opcode will read the VM memory at the lhs and rhs addresses, + /// - For instance a binary 'result = lhs+rhs' opcode will read the VM memory at the 'lhs' and 'rhs' addresses, /// compute the sum and write it to the 'result' memory address. /// 3. Update the program counter, usually by incrementing it. /// /// - Control flow opcodes jump around the bytecode by setting the program counter. - /// - Foreign call opcodes pause the VM until the foreign call results are available + /// - Foreign call opcodes pause the VM until the foreign call results are available. /// - Function call opcodes backup the current program counter into the call stack and jump to the function entry point. /// The stack frame for function calls is handled during codegen. - fn process_opcode_internal(&mut self) -> VMStatus { + fn process_opcode_internal(&mut self) -> &VMStatus { let opcode = &self.bytecode[self.program_counter]; match opcode { Opcode::BinaryFieldOp { op, lhs, rhs, destination: result } => { @@ -316,10 +333,10 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { self.increment_program_counter() } } - Opcode::Cast { destination: destination_address, source: source_address, bit_size } => { - let source_value = self.memory.read(*source_address); + Opcode::Cast { destination, source, bit_size } => { + let source_value = self.memory.read(*source); let casted_value = cast::cast(source_value, *bit_size); - self.memory.write(*destination_address, casted_value); + self.memory.write(*destination, casted_value); self.increment_program_counter() } Opcode::Jump { location: destination } => self.set_program_counter(*destination), @@ -327,12 +344,19 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { // Check if condition is true // We use 0 to mean false and any other value to mean true let condition_value = self.memory.read(*condition); - if condition_value.expect_u1().expect("condition value is not a boolean") { + let condition_value = match condition_value.expect_u1() { + Err(error) => { + return self.fail(format!("condition value is not a boolean: {error}")); + } + Ok(cond) => cond, + }; + if condition_value { self.fuzzing_trace_branching(*destination); - return self.set_program_counter(*destination); + self.set_program_counter(*destination) + } else { + self.fuzzing_trace_branching(self.program_counter + 1); + self.increment_program_counter() } - self.fuzzing_trace_branching(self.program_counter + 1); - self.increment_program_counter() } Opcode::CalldataCopy { destination_address, size_address, offset_address } => { let size = self.memory.read(*size_address).to_usize(); @@ -372,14 +396,18 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { Opcode::ConditionalMov { destination, source_a, source_b, condition } => { let condition_value = self.memory.read(*condition); - let condition_value_bool = - condition_value.expect_u1().expect("condition value is not a boolean"); - if condition_value_bool { + let condition_value = match condition_value.expect_u1() { + Err(error) => { + return self.fail(format!("condition value is not a boolean: {error}")); + } + Ok(cond) => cond, + }; + if condition_value { self.memory.write(*destination, self.memory.read(*source_a)); } else { self.memory.write(*destination, self.memory.read(*source_b)); } - self.fuzzing_trace_conditional_mov(condition_value_bool); + self.fuzzing_trace_conditional_mov(condition_value); self.increment_program_counter() } Opcode::Trap { revert_data } => { @@ -404,24 +432,25 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { self.finish(0, 0) } } - Opcode::Load { destination: destination_address, source_pointer } => { - // Convert our source_pointer to an address + Opcode::Load { destination, source_pointer } => { + // Convert the source_pointer to an address let source = self.memory.read_ref(*source_pointer); - // Use our usize source index to lookup the value in memory + // Use the source address to lookup the value in memory let value = self.memory.read(source); - self.memory.write(*destination_address, value); + self.memory.write(*destination, value); self.increment_program_counter() } Opcode::Store { destination_pointer, source: source_address } => { - // Convert our destination_pointer to an address + // Convert the destination_pointer to an address let destination = self.memory.read_ref(*destination_pointer); - // Use our usize destination index to set the value in memory + // Read the value at the source address let value = self.memory.read(*source_address); + // Use the destination address to set the value in memory self.memory.write(destination, value); self.increment_program_counter() } Opcode::Call { location } => { - // Push a return location + // Push the return location to the call stack. self.call_stack.push(self.program_counter); self.set_program_counter(*location) } @@ -431,16 +460,19 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { self.increment_program_counter() } Opcode::IndirectConst { destination_pointer, bit_size, value } => { - // Convert our destination_pointer to an address + // Convert the destination_pointer to an address let destination = self.memory.read_ref(*destination_pointer); - // Use our usize destination index to set the value in memory + // Use the destination address to set the value in memory self.memory.write(destination, MemoryValue::new_from_field(*value, *bit_size)); self.increment_program_counter() } Opcode::BlackBox(black_box_op) => { - match evaluate_black_box(black_box_op, self.black_box_solver, &mut self.memory) { - Ok(()) => self.increment_program_counter(), - Err(e) => self.fail(e.to_string()), + if let Err(e) = + evaluate_black_box(black_box_op, self.black_box_solver, &mut self.memory) + { + self.fail(e.to_string()) + } else { + self.increment_program_counter() } } } @@ -452,23 +484,23 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { } /// Increments the program counter by 1. - fn increment_program_counter(&mut self) -> VMStatus { + fn increment_program_counter(&mut self) -> &VMStatus { self.set_program_counter(self.program_counter + 1) } - /// Increments the program counter by `value`. + /// Sets the program counter to `value`. /// If the program counter no longer points to an opcode - /// in the bytecode, then the VMStatus reports halted. - fn set_program_counter(&mut self, value: usize) -> VMStatus { + /// in the bytecode, then the VMStatus reports `Finished`. + fn set_program_counter(&mut self, value: usize) -> &VMStatus { assert!(self.program_counter < self.bytecode.len()); self.program_counter = value; if self.program_counter >= self.bytecode.len() { self.status = VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }; } - self.status.clone() + &self.status } - /// Process a binary operation. + /// Process a binary field operation. /// This method will not modify the program counter. fn process_binary_field_op( &mut self, @@ -481,14 +513,12 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { let rhs_value = self.memory.read(rhs); let result_value = evaluate_binary_field_op(&op, lhs_value, rhs_value)?; - self.memory.write(result, result_value); - self.fuzzing_trace_binary_field_op_comparison(&op, lhs_value, rhs_value, result_value); Ok(()) } - /// Process a binary operation. + /// Process a binary integer operation. /// This method will not modify the program counter. fn process_binary_int_op( &mut self, @@ -507,6 +537,10 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver> VM<'a, F, B> { Ok(()) } + /// Process a unary negation operation. + /// + /// It returns `MemoryTypeError` if the value type does not match the type + /// indicated by `op_bit_size`. fn process_not( &mut self, source: MemoryAddress, diff --git a/acvm-repo/brillig_vm/src/memory.rs b/acvm-repo/brillig_vm/src/memory.rs index 05eb78c73c3..3410fa94b93 100644 --- a/acvm-repo/brillig_vm/src/memory.rs +++ b/acvm-repo/brillig_vm/src/memory.rs @@ -1,4 +1,4 @@ -//! Implementation of the VM's memory +//! Implementation of the VM's memory. use acir::{ AcirField, brillig::{BitSize, IntegerBitSize, MemoryAddress}, @@ -9,6 +9,11 @@ use acir::{ /// All memory pointers are interpreted as `u32` values, meaning the VM can directly address up to 2^32 memory slots. pub const MEMORY_ADDRESSING_BIT_SIZE: IntegerBitSize = IntegerBitSize::U32; +/// The current stack pointer is always in slot 0. +/// +/// It gets manipulated by opcodes laid down for calls by codegen. +pub const STACK_POINTER_ADDRESS: MemoryAddress = MemoryAddress::Direct(0); + /// A single typed value in the Brillig VM's memory. /// /// Memory in the VM is strongly typed and can represent either a native field element @@ -69,16 +74,21 @@ impl MemoryValue { } } + /// Expects a `U32` value and converts it into `usize`, otherwise panics. + /// + /// Primarily a convenience method for using values in memory operations as pointers, sizes and offsets. pub fn to_usize(&self) -> usize { match self { MemoryValue::U32(value) => (*value).try_into().unwrap(), - other => panic!("value is not typed as brillig usize: {other}"), + other => panic!("value is not typed as Brillig usize: {other}"), } } } impl MemoryValue { - /// Builds a memory value from a field element. + /// Builds a memory value from a field element, either field or integer type. + /// + /// If the bit size indicates an integer type, the value is downcast to fit into the specified size. pub fn new_from_field(value: F, bit_size: BitSize) -> Self { if let BitSize::Integer(bit_size) = bit_size { MemoryValue::new_integer(value.to_u128(), bit_size) @@ -87,7 +97,8 @@ impl MemoryValue { } } - /// Builds a memory value from a field element, checking that the value is within the bit size. + /// Builds a memory value from a field element, checking that the value is within the bit size, + /// otherwise returns `None`. pub fn new_checked(value: F, bit_size: BitSize) -> Option { if let BitSize::Integer(bit_size) = bit_size { if value.num_bits() > bit_size.into() { @@ -307,10 +318,18 @@ pub struct Memory { } impl Memory { + /// Read the value from slot 0. + /// + /// Panics if it's not a `U32`. fn get_stack_pointer(&self) -> usize { - self.read(MemoryAddress::Direct(0)).to_usize() + self.read(STACK_POINTER_ADDRESS).to_usize() } + /// Resolve an address to either: + /// * itself, if it's a direct address, or + /// * the current stack pointer plus the offset, if it's relative. + /// + /// Returns a memory slot index. fn resolve(&self, address: MemoryAddress) -> usize { match address { MemoryAddress::Direct(address) => address, @@ -318,34 +337,42 @@ impl Memory { } } - /// Gets the value at address + /// Reads the numeric value at the address. + /// + /// If the address is beyond the size of memory, a default value is returned. pub fn read(&self, address: MemoryAddress) -> MemoryValue { let resolved_addr = self.resolve(address); self.inner.get(resolved_addr).copied().unwrap_or_default() } + /// Reads the value at the address and returns it as a direct memory address, + /// without dereferencing the pointer itself to a numeric value. pub fn read_ref(&self, ptr: MemoryAddress) -> MemoryAddress { MemoryAddress::direct(self.read(ptr).to_usize()) } - pub fn read_slice(&self, addr: MemoryAddress, len: usize) -> &[MemoryValue] { + /// Read a contiguous slice of memory starting at `address`, up to `len` slots. + /// + /// Panics if the end index is beyond the size of the memory. + pub fn read_slice(&self, address: MemoryAddress, len: usize) -> &[MemoryValue] { // Allows to read a slice of uninitialized memory if the length is zero. // Ideally we'd be able to read uninitialized memory in general (as read does) // but that's not possible if we want to return a slice instead of owned data. if len == 0 { return &[]; } - let resolved_addr = self.resolve(addr); + let resolved_addr = self.resolve(address); &self.inner[resolved_addr..(resolved_addr + len)] } /// Sets the value at `address` to `value` pub fn write(&mut self, address: MemoryAddress, value: MemoryValue) { - let resolved_ptr = self.resolve(address); - self.resize_to_fit(resolved_ptr + 1); - self.inner[resolved_ptr] = value; + let resolved_addr = self.resolve(address); + self.resize_to_fit(resolved_addr + 1); + self.inner[resolved_addr] = value; } + /// Increase the size of memory fit `size` elements, or the current length, whichever is bigger. fn resize_to_fit(&mut self, size: usize) { // Calculate new memory size let new_size = std::cmp::max(self.inner.len(), size); @@ -355,9 +382,10 @@ impl Memory { /// Sets the values after `address` to `values` pub fn write_slice(&mut self, address: MemoryAddress, values: &[MemoryValue]) { - let resolved_address = self.resolve(address); - self.resize_to_fit(resolved_address + values.len()); - self.inner[resolved_address..(resolved_address + values.len())].copy_from_slice(values); + let resolved_addr = self.resolve(address); + let end_addr = resolved_addr + values.len(); + self.resize_to_fit(end_addr); + self.inner[resolved_addr..end_addr].copy_from_slice(values); } /// Returns the values of the memory diff --git a/acvm-repo/brillig_vm/tests/foreign_calls.rs b/acvm-repo/brillig_vm/tests/foreign_calls.rs index d5e7823485d..b9898d5a896 100644 --- a/acvm-repo/brillig_vm/tests/foreign_calls.rs +++ b/acvm-repo/brillig_vm/tests/foreign_calls.rs @@ -27,7 +27,7 @@ fn run_foreign_call_test( vm.resolve_foreign_call(ForeignCallResult { values: foreign_call_result }); let status = vm.process_opcode(); - assert_eq!(status, expected_final_status); + assert_eq!(*status, expected_final_status); let counter = vm.foreign_call_counter(); (vm.take_memory(), counter) } diff --git a/acvm-repo/brillig_vm/tests/mod.rs b/acvm-repo/brillig_vm/tests/mod.rs index fce4bc3b744..39766bb9222 100644 --- a/acvm-repo/brillig_vm/tests/mod.rs +++ b/acvm-repo/brillig_vm/tests/mod.rs @@ -37,7 +37,7 @@ fn add_single_step_smoke() { let mut vm = VM::new(calldata, &opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); // The address at index `2` should have the value of 3 since we had an // add opcode @@ -88,19 +88,19 @@ fn jmpif_opcode() { let mut vm = VM::new(calldata, &opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); let memory = vm.take_memory(); let output_cmp_value = memory.read(destination); @@ -267,15 +267,15 @@ fn cast_opcode() { let mut vm = VM::new(calldata, opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 1, return_data_size: 1 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 1, return_data_size: 1 }); let memory = vm.take_memory(); @@ -328,17 +328,17 @@ fn not_opcode() { let mut vm = VM::new(calldata, opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 1, return_data_size: 1 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 1, return_data_size: 1 }); let memory = vm.take_memory(); let MemoryValue::U128(negated_value) = memory.read(MemoryAddress::direct(1)) else { @@ -373,14 +373,14 @@ fn mov_opcode() { let mut vm = VM::new(calldata, opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); let memory = vm.take_memory(); @@ -439,19 +439,19 @@ fn cmov_opcode() { let mut vm = VM::new(calldata, opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); let memory = vm.take_memory(); @@ -538,38 +538,38 @@ fn cmp_binary_ops() { // Calldata copy let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); for _ in 0..calldata_size { let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); } // Equals let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let output_eq_value = vm.get_memory()[destination.unwrap_direct()]; assert_eq!(output_eq_value, true.into()); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let output_neq_value = vm.get_memory()[destination.unwrap_direct()]; assert_eq!(output_neq_value, false.into()); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let lt_value = vm.get_memory()[destination.unwrap_direct()]; assert_eq!(lt_value, true.into()); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); let lte_value = vm.get_memory()[destination.unwrap_direct()]; assert_eq!(lte_value, true.into()); @@ -666,10 +666,10 @@ fn iconst_opcode() { let mut vm = VM::new(vec![], opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); let memory = vm.take_memory(); @@ -921,7 +921,7 @@ fn relative_addressing() { vm.process_opcode(); vm.process_opcode(); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); + assert_eq!(*status, VMStatus::Finished { return_data_offset: 0, return_data_size: 0 }); let memory = vm.take_memory(); let output_value = memory.read(MemoryAddress::direct(1)); @@ -955,12 +955,12 @@ fn field_zero_division_regression() { let mut vm = VM::new(calldata, opcodes, &solver, false, None); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); - assert_eq!(status, VMStatus::InProgress); + assert_eq!(*status, VMStatus::InProgress); let status = vm.process_opcode(); assert_eq!( - status, + *status, VMStatus::Failure { reason: FailureReason::RuntimeError { message: "Attempted to divide by zero".into() }, call_stack: vec![2] diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index a912772ffc4..e920ce0da79 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -35,6 +35,7 @@ use self::{artifact::BrilligArtifact, debug_show::DebugToString, registers::Stac use acvm::{ AcirField, acir::brillig::{MemoryAddress, Opcode as BrilligOpcode}, + brillig_vm::STACK_POINTER_ADDRESS, }; use debug_show::DebugShow; @@ -45,15 +46,8 @@ use super::{BrilligOptions, FunctionId, GlobalSpace, ProcedureId}; /// memory has 2^32 memory slots. pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 32; -// Registers reserved in runtime for special purposes. -pub(crate) enum ReservedRegisters { - /// This register stores the stack pointer. All relative memory addresses are relative to this pointer. - StackPointer = 0, - /// This register stores the free memory pointer. Allocations must be done after this pointer. - FreeMemoryPointer = 1, - /// This register stores a 1_usize constant. - UsizeOne = 2, -} +/// Registers reserved in runtime for special purposes. +pub(crate) struct ReservedRegisters; impl ReservedRegisters { /// The number of reserved registers. These are allocated in the first memory positions. @@ -65,16 +59,19 @@ impl ReservedRegisters { Self::NUM_RESERVED_REGISTERS } + /// This register stores the stack pointer. All relative memory addresses are relative to this pointer. pub(crate) fn stack_pointer() -> MemoryAddress { - MemoryAddress::direct(ReservedRegisters::StackPointer as usize) + STACK_POINTER_ADDRESS } + /// This register stores the free memory pointer. Allocations must be done after this pointer. pub(crate) fn free_memory_pointer() -> MemoryAddress { - MemoryAddress::direct(ReservedRegisters::FreeMemoryPointer as usize) + MemoryAddress::direct(1) } + /// This register stores a 1_usize constant. pub(crate) fn usize_one() -> MemoryAddress { - MemoryAddress::direct(ReservedRegisters::UsizeOne as usize) + MemoryAddress::direct(2) } }