From 03015b69cb29c029df79d9c736b33cd75fbd84c2 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Thu, 16 Jan 2025 16:05:51 +0000 Subject: [PATCH 01/10] Typos --- .../stdlib_circuit_builders/circuit_builder_base.hpp | 2 +- yarn-project/simulator/src/avm/opcodes/conversion.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/circuit_builder_base.hpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/circuit_builder_base.hpp index b2eedbd74e56..59c78b6ae4e6 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/circuit_builder_base.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/circuit_builder_base.hpp @@ -382,7 +382,7 @@ template struct CircuitSchemaInternal { * ComposerBase naming conventions: * - n = 5 gates (4 gates plus the 'zero' gate). * - variables <-- A.k.a. "witnesses". Indices of this variables vector are referred to as `witness_indices`. - * Example of varibales in this example (a 3,4,5 triangle): + * Example of variables in this example (a 3,4,5 triangle): * - variables = [ 0, 3, 4, 5, 9, 16, 25, 25] * - public_inputs = [6] <-- points to variables[6]. * diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.ts b/yarn-project/simulator/src/avm/opcodes/conversion.ts index 3699c6f75b1a..f935d1cb5e00 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.ts @@ -6,7 +6,7 @@ import { Addressing } from './addressing_mode.js'; import { Instruction } from './instruction.js'; export class ToRadixBE extends Instruction { - static type: string = 'TORADIXLE'; + static type: string = 'TORADIXBE'; static readonly opcode: Opcode = Opcode.TORADIXBE; // Informs (de)serialization. See Instruction.deserialize. From 5608770f8ced55e07356f47d7fb1d7843be512a2 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Mon, 20 Jan 2025 16:33:48 +0000 Subject: [PATCH 02/10] 11295: Refactor some error handling for BrilligVM Blackbox opcodes --- .../acir/src/circuit/black_box_functions.rs | 38 +++++ .../acvm-repo/brillig/src/black_box.rs | 158 ++++++++++++++++++ noir/noir-repo/acvm-repo/brillig/src/lib.rs | 2 +- .../acvm-repo/brillig_vm/src/black_box.rs | 78 ++++----- 4 files changed, 236 insertions(+), 40 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs b/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs index d0ec7d02201c..d792c9b88203 100644 --- a/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs +++ b/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs @@ -3,6 +3,7 @@ //! This makes certain zk-snark unfriendly computations cheaper than if they were //! implemented in more basic constraints. +use crate::brillig::BrilligBlackBoxFunc; use serde::{Deserialize, Serialize}; use strum_macros::EnumIter; @@ -221,6 +222,43 @@ impl BlackBoxFunc { } } +impl From for BrilligBlackBoxFunc { + fn from(func: BlackBoxFunc) -> Self { + match func { + BlackBoxFunc::AES128Encrypt => BrilligBlackBoxFunc::AES128Encrypt, + BlackBoxFunc::AND => { + unreachable!("BlackBoxFunc::AND does not have a Brillig counterpart") + } + BlackBoxFunc::XOR => { + unreachable!("BlackBoxFunc::XOR does not have a Brillig counterpart") + } + BlackBoxFunc::RANGE => { + unreachable!("BlackBoxFunc::RANGE does not have a Brillig counterpart") + } + BlackBoxFunc::Blake2s => BrilligBlackBoxFunc::Blake2s, + BlackBoxFunc::Blake3 => BrilligBlackBoxFunc::Blake3, + BlackBoxFunc::EcdsaSecp256k1 => BrilligBlackBoxFunc::EcdsaSecp256k1, + BlackBoxFunc::EcdsaSecp256r1 => BrilligBlackBoxFunc::EcdsaSecp256r1, + BlackBoxFunc::MultiScalarMul => BrilligBlackBoxFunc::MultiScalarMul, + BlackBoxFunc::Keccakf1600 => BrilligBlackBoxFunc::Keccakf1600, + BlackBoxFunc::RecursiveAggregation => { + unreachable!( + "BlackBoxFunc::RecursiveAggregation does not have a Brillig counterpart" + ) + } + BlackBoxFunc::EmbeddedCurveAdd => BrilligBlackBoxFunc::EmbeddedCurveAdd, + BlackBoxFunc::BigIntAdd => BrilligBlackBoxFunc::BigIntAdd, + BlackBoxFunc::BigIntSub => BrilligBlackBoxFunc::BigIntSub, + BlackBoxFunc::BigIntMul => BrilligBlackBoxFunc::BigIntMul, + BlackBoxFunc::BigIntDiv => BrilligBlackBoxFunc::BigIntDiv, + BlackBoxFunc::BigIntFromLeBytes => BrilligBlackBoxFunc::BigIntFromLeBytes, + BlackBoxFunc::BigIntToLeBytes => BrilligBlackBoxFunc::BigIntToLeBytes, + BlackBoxFunc::Poseidon2Permutation => BrilligBlackBoxFunc::Poseidon2Permutation, + BlackBoxFunc::Sha256Compression => BrilligBlackBoxFunc::Sha256Compression, + } + } +} + #[cfg(test)] mod tests { use strum::IntoEnumIterator; diff --git a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs index be9ba20ed499..9972fa41973a 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs @@ -107,3 +107,161 @@ pub enum BlackBoxOp { output_bits: MemoryAddress, }, } + +/// Enum listing Brillig black box functions. There is one-to-one correspondence with the previous enum BlackBoxOp. +#[allow(clippy::upper_case_acronyms)] +#[derive(Clone, Debug, Hash, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum BrilligBlackBoxFunc { + /// Ciphers (encrypts) the provided plaintext using AES128 in CBC mode, + /// padding the input using PKCS#7. + /// - inputs: byte array `[u8; N]` + /// - iv: initialization vector `[u8; 16]` + /// - key: user key `[u8; 16]` + /// - outputs: byte vector `[u8]` of length `input.len() + (16 - input.len() % 16)` + AES128Encrypt, + + /// Computes the Blake2s hash of the inputs, as specified in + /// https://tools.ietf.org/html/rfc7693 + /// - inputs are a byte array, i.e a vector of (witness, 8) + /// - output is a byte array of length 32, i.e. an array of 32 + /// (witness, 8), constrained to be the blake2s of the inputs. + Blake2s, + + /// Computes the Blake3 hash of the inputs + /// - inputs are a byte array, i.e a vector of (witness, 8) + /// - output is a byte array of length 32, i.e an array of 32 + /// (witness, 8), constrained to be the blake3 of the inputs. + Blake3, + + /// Keccak Permutation function of width 1600 + /// - inputs: An array of 25 64-bit Keccak lanes that represent a keccak sponge of 1600 bits + /// - outputs: The result of a keccak f1600 permutation on the input state. Also an array of 25 Keccak lanes. + Keccakf1600, + + /// Verifies a ECDSA signature over the secp256k1 curve. + /// - inputs: + /// - x coordinate of public key as 32 bytes + /// - y coordinate of public key as 32 bytes + /// - the signature, as a 64 bytes array + /// - the hash of the message, as a vector of bytes + /// - output: 0 for failure and 1 for success + EcdsaSecp256k1, + + /// Verifies a ECDSA signature over the secp256r1 curve. + /// + /// Same as EcdsaSecp256k1, but done over another curve. + EcdsaSecp256r1, + + /// Multiple scalar multiplication (MSM) with a variable base/input point + /// (P) of the embedded curve. An MSM multiplies the points and scalars and + /// sums the results. + /// - input: + /// points (witness, N) a vector of x and y coordinates of input + /// points `[x1, y1, x2, y2,...]`. + /// scalars (witness, N) a vector of low and high limbs of input + /// scalars `[s1_low, s1_high, s2_low, s2_high, ...]`. (witness, N) + /// For Barretenberg, they must both be less than 128 bits. + /// - output: + /// a tuple of `x` and `y` coordinates of output. + /// Points computed as `s_low*P+s_high*2^{128}*P` + /// + /// Because the Grumpkin scalar field is bigger than the ACIR field, we + /// provide 2 ACIR fields representing the low and high parts of the Grumpkin + /// scalar $a$: `a=low+high*2^{128}`, with `low, high < 2^{128}` + MultiScalarMul, + + /// Addition over the embedded curve on which the witness is defined + /// The opcode makes the following assumptions but does not enforce them because + /// it is more efficient to do it only when required. For instance, adding two + /// points that are on the curve it guarantee to give a point on the curve. + /// + /// It assumes that the points are on the curve. + /// If the inputs are the same witnesses index, it will perform a doubling, + /// If not, it assumes that the points' x-coordinates are not equal. + /// It also assumes neither point is the infinity point. + EmbeddedCurveAdd, + + /// BigInt addition + BigIntAdd, + + /// BigInt subtraction + BigIntSub, + + /// BigInt multiplication + BigIntMul, + + /// BigInt division + BigIntDiv, + + /// BigInt from le bytes + BigIntFromLeBytes, + + /// BigInt to le bytes + BigIntToLeBytes, + + /// Permutation function of Poseidon2 + Poseidon2Permutation, + + /// SHA256 compression function + /// - input: [(witness, 32); 16] + /// - state: [(witness, 32); 8] + /// - output: [(witness, 32); 8] + Sha256Compression, + + /// Big-endian radix decomposition + ToRadix, +} + +impl std::fmt::Display for BrilligBlackBoxFunc { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.name()) + } +} + +impl From for BrilligBlackBoxFunc { + fn from(op: BlackBoxOp) -> Self { + match op { + BlackBoxOp::AES128Encrypt { .. } => BrilligBlackBoxFunc::AES128Encrypt, + BlackBoxOp::Blake2s { .. } => BrilligBlackBoxFunc::Blake2s, + BlackBoxOp::Blake3 { .. } => BrilligBlackBoxFunc::Blake3, + BlackBoxOp::Keccakf1600 { .. } => BrilligBlackBoxFunc::Keccakf1600, + BlackBoxOp::EcdsaSecp256k1 { .. } => BrilligBlackBoxFunc::EcdsaSecp256k1, + BlackBoxOp::EcdsaSecp256r1 { .. } => BrilligBlackBoxFunc::EcdsaSecp256r1, + BlackBoxOp::MultiScalarMul { .. } => BrilligBlackBoxFunc::MultiScalarMul, + BlackBoxOp::EmbeddedCurveAdd { .. } => BrilligBlackBoxFunc::EmbeddedCurveAdd, + BlackBoxOp::BigIntAdd { .. } => BrilligBlackBoxFunc::BigIntAdd, + BlackBoxOp::BigIntSub { .. } => BrilligBlackBoxFunc::BigIntSub, + BlackBoxOp::BigIntMul { .. } => BrilligBlackBoxFunc::BigIntMul, + BlackBoxOp::BigIntDiv { .. } => BrilligBlackBoxFunc::BigIntDiv, + BlackBoxOp::BigIntFromLeBytes { .. } => BrilligBlackBoxFunc::BigIntFromLeBytes, + BlackBoxOp::BigIntToLeBytes { .. } => BrilligBlackBoxFunc::BigIntToLeBytes, + BlackBoxOp::Poseidon2Permutation { .. } => BrilligBlackBoxFunc::Poseidon2Permutation, + BlackBoxOp::Sha256Compression { .. } => BrilligBlackBoxFunc::Sha256Compression, + BlackBoxOp::ToRadix { .. } => BrilligBlackBoxFunc::ToRadix, + } + } +} + +impl BrilligBlackBoxFunc { + pub fn name(&self) -> &'static str { + match self { + BrilligBlackBoxFunc::AES128Encrypt => "aes128_encrypt", + BrilligBlackBoxFunc::Blake2s => "blake2s", + BrilligBlackBoxFunc::Blake3 => "blake3", + BrilligBlackBoxFunc::EcdsaSecp256k1 => "ecdsa_secp256k1", + BrilligBlackBoxFunc::MultiScalarMul => "multi_scalar_mul", + BrilligBlackBoxFunc::EmbeddedCurveAdd => "embedded_curve_add", + BrilligBlackBoxFunc::Keccakf1600 => "keccakf1600", + BrilligBlackBoxFunc::EcdsaSecp256r1 => "ecdsa_secp256r1", + BrilligBlackBoxFunc::BigIntAdd => "bigint_add", + BrilligBlackBoxFunc::BigIntSub => "bigint_sub", + BrilligBlackBoxFunc::BigIntMul => "bigint_mul", + BrilligBlackBoxFunc::BigIntDiv => "bigint_div", + BrilligBlackBoxFunc::BigIntFromLeBytes => "bigint_from_le_bytes", + BrilligBlackBoxFunc::BigIntToLeBytes => "bigint_to_le_bytes", + BrilligBlackBoxFunc::Poseidon2Permutation => "poseidon2_permutation", + BrilligBlackBoxFunc::Sha256Compression => "sha256_compression", + BrilligBlackBoxFunc::ToRadix => "to_radix", + } + } +} diff --git a/noir/noir-repo/acvm-repo/brillig/src/lib.rs b/noir/noir-repo/acvm-repo/brillig/src/lib.rs index cf31ff79996b..9636f6c3960f 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/lib.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/lib.rs @@ -13,7 +13,7 @@ mod black_box; mod foreign_call; mod opcodes; -pub use black_box::BlackBoxOp; +pub use black_box::{BlackBoxOp, BrilligBlackBoxFunc}; pub use foreign_call::{ForeignCallParam, ForeignCallResult}; pub use opcodes::{ BinaryFieldOp, BinaryIntOp, HeapArray, HeapValueType, HeapVector, MemoryAddress, ValueOrArray, diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs index 9ebbbd3f0871..de6bbd8fa26f 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs @@ -1,4 +1,4 @@ -use acir::brillig::{BlackBoxOp, HeapArray, HeapVector, IntegerBitSize}; +use acir::brillig::{BlackBoxOp, BrilligBlackBoxFunc, HeapArray, HeapVector, IntegerBitSize}; use acir::{AcirField, BlackBoxFunc}; use acvm_blackbox_solver::{ aes128_encrypt, blake2s, blake3, ecdsa_secp256k1_verify, ecdsa_secp256r1_verify, keccakf1600, @@ -6,10 +6,27 @@ use acvm_blackbox_solver::{ }; use num_bigint::BigUint; use num_traits::Zero; +use thiserror::Error; use crate::memory::MemoryValue; use crate::Memory; +#[derive(Clone, PartialEq, Eq, Debug, Error)] +pub(crate) enum BrilligBlackBoxResolutionError { + #[error("failed to solve brillig blackbox function: {0}, reason: {1}")] + Failed(BrilligBlackBoxFunc, String), +} + +impl From for BrilligBlackBoxResolutionError { + fn from(err: BlackBoxResolutionError) -> Self { + match err { + BlackBoxResolutionError::Failed(func, string) => { + BrilligBlackBoxResolutionError::Failed(func.into(), string) + } + } + } +} + fn read_heap_vector<'a, F: AcirField>( memory: &'a Memory, vector: &HeapVector, @@ -45,19 +62,23 @@ pub(crate) fn evaluate_black_box solver: &Solver, memory: &mut Memory, bigint_solver: &mut BrilligBigIntSolver, -) -> Result<(), BlackBoxResolutionError> { +) -> Result<(), BrilligBlackBoxResolutionError> { match op { BlackBoxOp::AES128Encrypt { inputs, iv, key, outputs } => { - let bb_func = black_box_function_from_op(op); - let inputs = to_u8_vec(read_heap_vector(memory, inputs)); let iv: [u8; 16] = to_u8_vec(read_heap_array(memory, iv)).try_into().map_err(|_| { - BlackBoxResolutionError::Failed(bb_func, "Invalid iv length".to_string()) + BrilligBlackBoxResolutionError::Failed( + (*op).into(), + "Invalid iv length".to_string(), + ) })?; let key: [u8; 16] = to_u8_vec(read_heap_array(memory, key)).try_into().map_err(|_| { - BlackBoxResolutionError::Failed(bb_func, "Invalid key length".to_string()) + BrilligBlackBoxResolutionError::Failed( + (*op).into(), + "Invalid key length".to_string(), + ) })?; let ciphertext = aes128_encrypt(&inputs, iv, key)?; @@ -105,25 +126,26 @@ pub(crate) fn evaluate_black_box signature, result: result_address, } => { - let bb_func = black_box_function_from_op(op); - let public_key_x: [u8; 32] = to_u8_vec(read_heap_array(memory, public_key_x)).try_into().map_err(|_| { - BlackBoxResolutionError::Failed( - bb_func, + BrilligBlackBoxResolutionError::Failed( + (*op).into(), "Invalid public key x length".to_string(), ) })?; let public_key_y: [u8; 32] = to_u8_vec(read_heap_array(memory, public_key_y)).try_into().map_err(|_| { - BlackBoxResolutionError::Failed( - bb_func, + BrilligBlackBoxResolutionError::Failed( + (*op).into(), "Invalid public key y length".to_string(), ) })?; let signature: [u8; 64] = to_u8_vec(read_heap_array(memory, signature)).try_into().map_err(|_| { - BlackBoxResolutionError::Failed(bb_func, "Invalid signature length".to_string()) + BrilligBlackBoxResolutionError::Failed( + (*op).into(), + "Invalid signature length".to_string(), + ) })?; let hashed_msg = to_u8_vec(read_heap_vector(memory, hashed_msg)); @@ -284,8 +306,8 @@ pub(crate) fn evaluate_black_box let mut message = [0; 16]; let inputs = read_heap_array(memory, input); if inputs.len() != 16 { - return Err(BlackBoxResolutionError::Failed( - BlackBoxFunc::Sha256Compression, + return Err(BrilligBlackBoxResolutionError::Failed( + BlackBoxFunc::Sha256Compression.into(), format!("Expected 16 inputs but encountered {}", &inputs.len()), )); } @@ -295,8 +317,8 @@ pub(crate) fn evaluate_black_box let mut state = [0; 8]; let values = read_heap_array(memory, hash_values); if values.len() != 8 { - return Err(BlackBoxResolutionError::Failed( - BlackBoxFunc::Sha256Compression, + return Err(BrilligBlackBoxResolutionError::Failed( + BlackBoxFunc::Sha256Compression.into(), format!("Expected 8 values but encountered {}", &values.len()), )); } @@ -348,25 +370,3 @@ pub(crate) fn evaluate_black_box } } } - -fn black_box_function_from_op(op: &BlackBoxOp) -> BlackBoxFunc { - match op { - BlackBoxOp::AES128Encrypt { .. } => BlackBoxFunc::AES128Encrypt, - BlackBoxOp::Blake2s { .. } => BlackBoxFunc::Blake2s, - BlackBoxOp::Blake3 { .. } => BlackBoxFunc::Blake3, - BlackBoxOp::Keccakf1600 { .. } => BlackBoxFunc::Keccakf1600, - BlackBoxOp::EcdsaSecp256k1 { .. } => BlackBoxFunc::EcdsaSecp256k1, - BlackBoxOp::EcdsaSecp256r1 { .. } => BlackBoxFunc::EcdsaSecp256r1, - BlackBoxOp::MultiScalarMul { .. } => BlackBoxFunc::MultiScalarMul, - BlackBoxOp::EmbeddedCurveAdd { .. } => BlackBoxFunc::EmbeddedCurveAdd, - BlackBoxOp::BigIntAdd { .. } => BlackBoxFunc::BigIntAdd, - BlackBoxOp::BigIntSub { .. } => BlackBoxFunc::BigIntSub, - BlackBoxOp::BigIntMul { .. } => BlackBoxFunc::BigIntMul, - BlackBoxOp::BigIntDiv { .. } => BlackBoxFunc::BigIntDiv, - BlackBoxOp::BigIntFromLeBytes { .. } => BlackBoxFunc::BigIntFromLeBytes, - BlackBoxOp::BigIntToLeBytes { .. } => BlackBoxFunc::BigIntToLeBytes, - BlackBoxOp::Poseidon2Permutation { .. } => BlackBoxFunc::Poseidon2Permutation, - BlackBoxOp::Sha256Compression { .. } => BlackBoxFunc::Sha256Compression, - BlackBoxOp::ToRadix { .. } => unreachable!("ToRadix is not an ACIR BlackBoxFunc"), - } -} From 35bf700df324ff324c72aa3276bbe648f4e95feb Mon Sep 17 00:00:00 2001 From: jeanmon Date: Mon, 20 Jan 2025 17:03:39 +0000 Subject: [PATCH 03/10] 11295: More granular error handling for toRadix in BrilligVM --- .../acvm-repo/brillig_vm/src/black_box.rs | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs index de6bbd8fa26f..f9d87020be8a 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs @@ -5,7 +5,7 @@ use acvm_blackbox_solver::{ sha256_compression, BigIntSolverWithId, BlackBoxFunctionSolver, BlackBoxResolutionError, }; use num_bigint::BigUint; -use num_traits::Zero; +use num_traits::{ToPrimitive, Zero}; use thiserror::Error; use crate::memory::MemoryValue; @@ -307,7 +307,7 @@ pub(crate) fn evaluate_black_box let inputs = read_heap_array(memory, input); if inputs.len() != 16 { return Err(BrilligBlackBoxResolutionError::Failed( - BlackBoxFunc::Sha256Compression.into(), + BrilligBlackBoxFunc::Sha256Compression, format!("Expected 16 inputs but encountered {}", &inputs.len()), )); } @@ -318,7 +318,7 @@ pub(crate) fn evaluate_black_box let values = read_heap_array(memory, hash_values); if values.len() != 8 { return Err(BrilligBlackBoxResolutionError::Failed( - BlackBoxFunc::Sha256Compression.into(), + BrilligBlackBoxFunc::Sha256Compression, format!("Expected 8 values but encountered {}", &values.len()), )); } @@ -338,7 +338,12 @@ pub(crate) fn evaluate_black_box .read(*radix) .expect_integer_with_bit_size(IntegerBitSize::U32) .expect("ToRadix opcode's radix bit size does not match expected bit size 32"); - let num_limbs = memory.read(*num_limbs).to_usize(); + let num_limbs = memory + .read(*num_limbs) + .expect_integer_with_bit_size(IntegerBitSize::U32) + .expect("ToRadix opcode's number of limbs does not match expected bit size 32") + .to_usize() + .expect("usize type is not of bit size 32"); let output_bits = !memory .read(*output_bits) .expect_integer_with_bit_size(IntegerBitSize::U1) @@ -348,6 +353,27 @@ pub(crate) fn evaluate_black_box let mut input = BigUint::from_bytes_be(&input.to_be_bytes()); let radix = BigUint::from_bytes_be(&radix.to_be_bytes()); + if radix < BigUint::from(2u32) || radix > BigUint::from(256u32) { + return Err(BrilligBlackBoxResolutionError::Failed( + BrilligBlackBoxFunc::ToRadix, + format!("Radix out of the valid range [2,256]. Value: {}", radix), + )); + } + + if num_limbs < 1 && input != BigUint::from(0u32) { + return Err(BrilligBlackBoxResolutionError::Failed( + BrilligBlackBoxFunc::ToRadix, + format!(" Input value {} is not zero but number of limbs is zero.", input), + )); + } + + if output_bits && radix != BigUint::from(2u32) { + return Err(BrilligBlackBoxResolutionError::Failed( + BrilligBlackBoxFunc::ToRadix, + format!("Radix {} is not equal to two in bit mode.", radix), + )); + } + let mut limbs: Vec> = vec![MemoryValue::default(); num_limbs]; for i in (0..num_limbs).rev() { From 1010b9f218c71c12ca7389a1041c27d9862bf7e1 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 21 Jan 2025 08:44:14 +0000 Subject: [PATCH 04/10] 11295: more error handling for AVM simulator and TS tests --- .../acvm-repo/brillig_vm/src/black_box.rs | 6 +- yarn-project/simulator/src/avm/errors.ts | 10 +++ .../src/avm/opcodes/conversion.test.ts | 69 +++++++++++++++++++ .../simulator/src/avm/opcodes/conversion.ts | 19 +++-- 4 files changed, 96 insertions(+), 8 deletions(-) diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs index f9d87020be8a..30edf60d268c 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs @@ -343,7 +343,7 @@ pub(crate) fn evaluate_black_box .expect_integer_with_bit_size(IntegerBitSize::U32) .expect("ToRadix opcode's number of limbs does not match expected bit size 32") .to_usize() - .expect("usize type is not of bit size 32"); + .unwrap(); // Will not panic as 32 bits must fit into usize type. let output_bits = !memory .read(*output_bits) .expect_integer_with_bit_size(IntegerBitSize::U1) @@ -363,14 +363,14 @@ pub(crate) fn evaluate_black_box if num_limbs < 1 && input != BigUint::from(0u32) { return Err(BrilligBlackBoxResolutionError::Failed( BrilligBlackBoxFunc::ToRadix, - format!(" Input value {} is not zero but number of limbs is zero.", input), + format!("Input value {} is not zero but number of limbs is zero.", input), )); } if output_bits && radix != BigUint::from(2u32) { return Err(BrilligBlackBoxResolutionError::Failed( BrilligBlackBoxFunc::ToRadix, - format!("Radix {} is not equal to two in bit mode.", radix), + format!("Radix {} is not equal to 2 and bit mode is activated.", radix), )); } diff --git a/yarn-project/simulator/src/avm/errors.ts b/yarn-project/simulator/src/avm/errors.ts index 64007b356294..9fdd03059604 100644 --- a/yarn-project/simulator/src/avm/errors.ts +++ b/yarn-project/simulator/src/avm/errors.ts @@ -148,6 +148,16 @@ export class MSMPointNotOnCurveError extends AvmExecutionError { } } +/** + * Error is thrown when some inputs of ToRadixBE are not valid. + */ +export class InvalidToRadixInputsError extends AvmExecutionError { + constructor(errorString: string) { + super(errorString); + this.name = 'InvalidToRadixInputsError'; + } +} + /** * Error is thrown when a static call attempts to alter some state */ diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.test.ts b/yarn-project/simulator/src/avm/opcodes/conversion.test.ts index 873ab29db5b4..940dda8be0b4 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.test.ts @@ -1,5 +1,6 @@ import { type AvmContext } from '../avm_context.js'; import { Field, Uint1, type Uint8, Uint32 } from '../avm_memory_types.js'; +import { InvalidToRadixInputsError } from '../errors.js'; import { initContext } from '../fixtures/index.js'; import { Addressing, AddressingMode } from './addressing_mode.js'; import { ToRadixBE } from './conversion.js'; @@ -150,5 +151,73 @@ describe('Conversion Opcodes', () => { expect(resultBuffer.readUInt8(i)).toEqual(expectedResults[2 * i] * 16 + expectedResults[2 * i + 1]); } }); + + test.each([0, 1, 257] as const)('Should throw an error for radix equal to %s', async radix => { + const radixOffset = 1; + const numLimbsOffset = 100; + const outputBitsOffset = 200; + context.machineState.memory.set(radixOffset, new Uint32(radix)); + context.machineState.memory.set(numLimbsOffset, new Uint32(10)); //the first 10 bits + context.machineState.memory.set(outputBitsOffset, new Uint1(1)); // true, output as bits + + await expect( + new ToRadixBE( + 0 /*indirect*/, + 0 /*srcOffset*/, + radixOffset, + numLimbsOffset, + outputBitsOffset, + 20 /*dstOffset*/, + ).execute(context), + ).rejects.toThrow(InvalidToRadixInputsError); + }); + + test.each([1, 2, 256, 98263423541] as const)( + 'Should throw an error for non-zero input %s when number of limbs is zero', + async arg => { + const srcOffset = 0; + const radixOffset = 1; + const numLimbsOffset = 100; + const outputBitsOffset = 200; + context.machineState.memory.set(srcOffset, new Field(arg)); + context.machineState.memory.set(radixOffset, new Uint32(16)); + context.machineState.memory.set(numLimbsOffset, new Uint32(0)); // 0 number of limbs + context.machineState.memory.set(outputBitsOffset, new Uint1(0)); // false, output as bytes + + await expect( + new ToRadixBE( + 0 /*indirect*/, + srcOffset, + radixOffset, + numLimbsOffset, + outputBitsOffset, + 20 /*dstOffset*/, + ).execute(context), + ).rejects.toThrow(InvalidToRadixInputsError); + }, + ); + + test.each([3, 4, 256] as const)( + 'Should throw an error for radix %s not equal to 2 when bit mode is activated', + async radix => { + const radixOffset = 1; + const numLimbsOffset = 100; + const outputBitsOffset = 200; + context.machineState.memory.set(radixOffset, new Uint32(radix)); + context.machineState.memory.set(numLimbsOffset, new Uint32(4)); // 4 first bytes + context.machineState.memory.set(outputBitsOffset, new Uint1(1)); // true, output as bit + + await expect( + new ToRadixBE( + 0 /*indirect*/, + 0 /*srcOffset*/, + radixOffset, + numLimbsOffset, + outputBitsOffset, + 20 /*dstOffset*/, + ).execute(context), + ).rejects.toThrow(InvalidToRadixInputsError); + }, + ); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.ts b/yarn-project/simulator/src/avm/opcodes/conversion.ts index f935d1cb5e00..630dde154d26 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.ts @@ -1,6 +1,6 @@ import { type AvmContext } from '../avm_context.js'; import { TypeTag, Uint1, Uint8 } from '../avm_memory_types.js'; -import { InstructionExecutionError } from '../errors.js'; +import { InstructionExecutionError, InvalidToRadixInputsError } from '../errors.js'; import { Opcode, OperandType } from '../serialization/instruction_serialization.js'; import { Addressing } from './addressing_mode.js'; import { Instruction } from './instruction.js'; @@ -49,12 +49,21 @@ export class ToRadixBE extends Instruction { let value: bigint = memory.get(srcOffset).toBigInt(); const radix: bigint = memory.get(radixOffset).toBigInt(); - if (numLimbs < 1) { - throw new InstructionExecutionError(`ToRadixBE instruction's numLimbs should be > 0 (was ${numLimbs})`); + + if (radix < 2 || radix > 256) { + throw new InvalidToRadixInputsError(`ToRadixBE instruction's radix should be in range [2,256] (was ${radix}).`); } - if (radix > 256) { - throw new InstructionExecutionError(`ToRadixBE instruction's radix should be <= 256 (was ${radix})`); + + if (numLimbs < 1 && value != BigInt(0)) { + throw new InvalidToRadixInputsError( + `ToRadixBE instruction's input value is not zero (was ${value}) but numLimbs zero.`, + ); } + + if (outputBits != 0 && radix != BigInt(2)) { + throw new InvalidToRadixInputsError(`Radix ${radix} is not equal to 2 and bit mode is activated.`); + } + const radixBN: bigint = BigInt(radix); const limbArray = new Array(numLimbs); From cef33ce9f7953bcde2b3fe0b1b21ed61f606e866 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 21 Jan 2025 10:09:22 +0000 Subject: [PATCH 05/10] 11295: error handling in witness generation of cpp circuit --- .../cpp/src/barretenberg/vm/avm/trace/errors.hpp | 2 +- .../cpp/src/barretenberg/vm/avm/trace/helper.cpp | 4 ++-- barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp | 9 ++++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp index e6613bd7c589..535031969c06 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp @@ -18,7 +18,7 @@ enum class AvmError : uint32_t { PARSING_ERROR, ENV_VAR_UNKNOWN, CONTRACT_INST_MEM_UNKNOWN, - RADIX_OUT_OF_BOUNDS, + INVALID_TORADIXBE_INPUTS, DUPLICATE_NULLIFIER, SIDE_EFFECT_LIMIT_REACHED, OUT_OF_GAS, diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp index c36904e66873..e54ba422811c 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp @@ -129,8 +129,8 @@ std::string to_name(AvmError error) return "ENVIRONMENT VARIABLE UNKNOWN"; case AvmError::CONTRACT_INST_MEM_UNKNOWN: return "CONTRACT INSTANCE MEMBER UNKNOWN"; - case AvmError::RADIX_OUT_OF_BOUNDS: - return "RADIX OUT OF BOUNDS"; + case AvmError::INVALID_TORADIXBE_INPUTS: + return "INVALID TO_RADIX_BE INPUTS"; case AvmError::DUPLICATE_NULLIFIER: return "DUPLICATE NULLIFIER"; case AvmError::SIDE_EFFECT_LIMIT_REACHED: diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp index c1cba7fe195a..49a89b364acb 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp @@ -5019,9 +5019,12 @@ AvmError AvmTraceBuilder::op_to_radix_be(uint16_t indirect, // uint32_t radix = static_cast(read_radix.val); uint32_t radix = static_cast(read_radix); - bool radix_out_of_bounds = radix > 256; - if (is_ok(error) && radix_out_of_bounds) { - error = AvmError::RADIX_OUT_OF_BOUNDS; + const bool radix_out_of_range = radix < 2 || radix > 256; + const bool zero_limb_input_non_zero = num_limbs == 0 && input != FF(0); + const bool bit_mode_radix_not_two = output_bits > 0 && radix != 2; + + if (is_ok(error) && (radix_out_of_range || zero_limb_input_non_zero || bit_mode_radix_not_two)) { + error = AvmError::INVALID_TORADIXBE_INPUTS; } // In case of an error, we do not perform the computation. From 6013824dfa787ed0d72d2ec2c6917cadfbcce9c0 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 21 Jan 2025 11:19:29 +0000 Subject: [PATCH 06/10] 11295: address review feedback --- yarn-project/simulator/src/avm/opcodes/conversion.test.ts | 6 +++--- yarn-project/simulator/src/avm/opcodes/conversion.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.test.ts b/yarn-project/simulator/src/avm/opcodes/conversion.test.ts index 940dda8be0b4..a7f1c5658219 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.test.ts @@ -152,7 +152,7 @@ describe('Conversion Opcodes', () => { } }); - test.each([0, 1, 257] as const)('Should throw an error for radix equal to %s', async radix => { + it.each([0, 1, 257])('Should throw an error for radix equal to %s', async radix => { const radixOffset = 1; const numLimbsOffset = 100; const outputBitsOffset = 200; @@ -172,7 +172,7 @@ describe('Conversion Opcodes', () => { ).rejects.toThrow(InvalidToRadixInputsError); }); - test.each([1, 2, 256, 98263423541] as const)( + it.each([1, 2, 256, 98263423541])( 'Should throw an error for non-zero input %s when number of limbs is zero', async arg => { const srcOffset = 0; @@ -197,7 +197,7 @@ describe('Conversion Opcodes', () => { }, ); - test.each([3, 4, 256] as const)( + it.each([3, 4, 256])( 'Should throw an error for radix %s not equal to 2 when bit mode is activated', async radix => { const radixOffset = 1; diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.ts b/yarn-project/simulator/src/avm/opcodes/conversion.ts index 630dde154d26..a7f486ca06e5 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.ts @@ -54,13 +54,13 @@ export class ToRadixBE extends Instruction { throw new InvalidToRadixInputsError(`ToRadixBE instruction's radix should be in range [2,256] (was ${radix}).`); } - if (numLimbs < 1 && value != BigInt(0)) { + if (numLimbs < 1 && value != BigInt(0n)) { throw new InvalidToRadixInputsError( `ToRadixBE instruction's input value is not zero (was ${value}) but numLimbs zero.`, ); } - if (outputBits != 0 && radix != BigInt(2)) { + if (outputBits != 0 && radix != BigInt(2n)) { throw new InvalidToRadixInputsError(`Radix ${radix} is not equal to 2 and bit mode is activated.`); } From 24b8309e641469619fbb7b0f96425dc4a3e4ce13 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 21 Jan 2025 12:10:41 +0000 Subject: [PATCH 07/10] 11295: TS superfluous import --- yarn-project/simulator/src/avm/opcodes/conversion.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/yarn-project/simulator/src/avm/opcodes/conversion.ts b/yarn-project/simulator/src/avm/opcodes/conversion.ts index a7f486ca06e5..60de7a8db080 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.ts @@ -1,6 +1,6 @@ import { type AvmContext } from '../avm_context.js'; import { TypeTag, Uint1, Uint8 } from '../avm_memory_types.js'; -import { InstructionExecutionError, InvalidToRadixInputsError } from '../errors.js'; +import { InvalidToRadixInputsError } from '../errors.js'; import { Opcode, OperandType } from '../serialization/instruction_serialization.js'; import { Addressing } from './addressing_mode.js'; import { Instruction } from './instruction.js'; From 84d13d1cf9083eaec3ee1afdce234dd25d894849 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 21 Jan 2025 13:40:14 +0000 Subject: [PATCH 08/10] 11295: Address review feedback for brillig vm code --- .../acvm-repo/brillig/src/black_box.rs | 72 ++++++++----------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs index 9972fa41973a..7c636f57aea6 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs @@ -114,71 +114,59 @@ pub enum BlackBoxOp { pub enum BrilligBlackBoxFunc { /// Ciphers (encrypts) the provided plaintext using AES128 in CBC mode, /// padding the input using PKCS#7. - /// - inputs: byte array `[u8; N]` - /// - iv: initialization vector `[u8; 16]` - /// - key: user key `[u8; 16]` - /// - outputs: byte vector `[u8]` of length `input.len() + (16 - input.len() % 16)` + /// - inputs: HeapVector of `u8` + /// - iv: initialization vector of type HeapArray `[u8; 16]` + /// - key: HeapArray `[u8; 16]` + /// - outputs: HeapVector of `u8` of length `inputs.len() + (16 - inputs.len() % 16)` AES128Encrypt, - /// Computes the Blake2s hash of the inputs, as specified in + /// Computes the Blake2s hash of the message, as specified in /// https://tools.ietf.org/html/rfc7693 - /// - inputs are a byte array, i.e a vector of (witness, 8) - /// - output is a byte array of length 32, i.e. an array of 32 - /// (witness, 8), constrained to be the blake2s of the inputs. + /// - message: HeapVector of `u8` + /// - output: HeapArray of `u8` of length 32 Blake2s, - /// Computes the Blake3 hash of the inputs - /// - inputs are a byte array, i.e a vector of (witness, 8) - /// - output is a byte array of length 32, i.e an array of 32 - /// (witness, 8), constrained to be the blake3 of the inputs. + /// Computes the Blake3 hash of the message. + /// - message: HeapVector of `u8` + /// - output: HeapArray of `u8` of length 32 Blake3, /// Keccak Permutation function of width 1600 - /// - inputs: An array of 25 64-bit Keccak lanes that represent a keccak sponge of 1600 bits - /// - outputs: The result of a keccak f1600 permutation on the input state. Also an array of 25 Keccak lanes. + /// - input: HeapArray of `u8` representing 25 64-bit Keccak lanes that represent a keccak sponge of 1600 bits + /// - output: HeapArray of `u8` containing the result of a keccak f1600 permutation on the input state. Also an array of 25 Keccak lanes. Keccakf1600, - /// Verifies a ECDSA signature over the secp256k1 curve. + /// Verifies an ECDSA signature over the secp256k1 curve. /// - inputs: - /// - x coordinate of public key as 32 bytes - /// - y coordinate of public key as 32 bytes - /// - the signature, as a 64 bytes array - /// - the hash of the message, as a vector of bytes - /// - output: 0 for failure and 1 for success + /// - x coordinate of public key as 32 bytes (HeapArray of `u8`) + /// - y coordinate of public key as 32 bytes (HeapArray of `u8`) + /// - the signature, as a 64 bytes array (HeapArray of `u8`) + /// - the hash of the message, as a vector of bytes (HeapVector of `u8`) + /// - result: 0 for failure and 1 for success EcdsaSecp256k1, - /// Verifies a ECDSA signature over the secp256r1 curve. + /// Verifies an ECDSA signature over the secp256r1 curve. /// /// Same as EcdsaSecp256k1, but done over another curve. EcdsaSecp256r1, /// Multiple scalar multiplication (MSM) with a variable base/input point - /// (P) of the embedded curve. An MSM multiplies the points and scalars and + /// (P) of the embedded (Grumpkin) curve. An MSM multiplies the points and scalars and /// sums the results. /// - input: - /// points (witness, N) a vector of x and y coordinates of input - /// points `[x1, y1, x2, y2,...]`. - /// scalars (witness, N) a vector of low and high limbs of input - /// scalars `[s1_low, s1_high, s2_low, s2_high, ...]`. (witness, N) - /// For Barretenberg, they must both be less than 128 bits. - /// - output: - /// a tuple of `x` and `y` coordinates of output. - /// Points computed as `s_low*P+s_high*2^{128}*P` + /// points: HeapVector of x and y coordinates (and infinity point flag) of the input + /// points `[x1, y1, x2, y2,...]`. + /// scalars: HeapVector of low and high limbs of input + /// scalars `[s1_low, s1_high, s2_low, s2_high, ...]`. (witness, N) + /// - outputs: HeapArray containing a tuple of `x` and `y` coordinates + /// (and infinity point flag) of output. /// /// Because the Grumpkin scalar field is bigger than the ACIR field, we /// provide 2 ACIR fields representing the low and high parts of the Grumpkin /// scalar $a$: `a=low+high*2^{128}`, with `low, high < 2^{128}` MultiScalarMul, - /// Addition over the embedded curve on which the witness is defined - /// The opcode makes the following assumptions but does not enforce them because - /// it is more efficient to do it only when required. For instance, adding two - /// points that are on the curve it guarantee to give a point on the curve. - /// - /// It assumes that the points are on the curve. - /// If the inputs are the same witnesses index, it will perform a doubling, - /// If not, it assumes that the points' x-coordinates are not equal. - /// It also assumes neither point is the infinity point. + /// Addition over the embedded (Grumpkin) curve. EmbeddedCurveAdd, /// BigInt addition @@ -203,9 +191,9 @@ pub enum BrilligBlackBoxFunc { Poseidon2Permutation, /// SHA256 compression function - /// - input: [(witness, 32); 16] - /// - state: [(witness, 32); 8] - /// - output: [(witness, 32); 8] + /// - input: HeapArray of `u32` of length 16 + /// - state (hash_values): HeapArray of `u32` of length 8 + /// - output: HeapArray of `u32` of length 8 Sha256Compression, /// Big-endian radix decomposition From 7da9d0c0769581901d157479c1e4c371fa5a9ac1 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 22 Jan 2025 08:47:23 +0000 Subject: [PATCH 09/10] 11295: Revert noir repo changes related to toRadix error handling --- .../acir/src/circuit/black_box_functions.rs | 38 ----- .../acvm-repo/brillig/src/black_box.rs | 146 ------------------ noir/noir-repo/acvm-repo/brillig/src/lib.rs | 2 +- .../acvm-repo/brillig_vm/src/black_box.rs | 108 +++++-------- 4 files changed, 42 insertions(+), 252 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs b/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs index d792c9b88203..d0ec7d02201c 100644 --- a/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs +++ b/noir/noir-repo/acvm-repo/acir/src/circuit/black_box_functions.rs @@ -3,7 +3,6 @@ //! This makes certain zk-snark unfriendly computations cheaper than if they were //! implemented in more basic constraints. -use crate::brillig::BrilligBlackBoxFunc; use serde::{Deserialize, Serialize}; use strum_macros::EnumIter; @@ -222,43 +221,6 @@ impl BlackBoxFunc { } } -impl From for BrilligBlackBoxFunc { - fn from(func: BlackBoxFunc) -> Self { - match func { - BlackBoxFunc::AES128Encrypt => BrilligBlackBoxFunc::AES128Encrypt, - BlackBoxFunc::AND => { - unreachable!("BlackBoxFunc::AND does not have a Brillig counterpart") - } - BlackBoxFunc::XOR => { - unreachable!("BlackBoxFunc::XOR does not have a Brillig counterpart") - } - BlackBoxFunc::RANGE => { - unreachable!("BlackBoxFunc::RANGE does not have a Brillig counterpart") - } - BlackBoxFunc::Blake2s => BrilligBlackBoxFunc::Blake2s, - BlackBoxFunc::Blake3 => BrilligBlackBoxFunc::Blake3, - BlackBoxFunc::EcdsaSecp256k1 => BrilligBlackBoxFunc::EcdsaSecp256k1, - BlackBoxFunc::EcdsaSecp256r1 => BrilligBlackBoxFunc::EcdsaSecp256r1, - BlackBoxFunc::MultiScalarMul => BrilligBlackBoxFunc::MultiScalarMul, - BlackBoxFunc::Keccakf1600 => BrilligBlackBoxFunc::Keccakf1600, - BlackBoxFunc::RecursiveAggregation => { - unreachable!( - "BlackBoxFunc::RecursiveAggregation does not have a Brillig counterpart" - ) - } - BlackBoxFunc::EmbeddedCurveAdd => BrilligBlackBoxFunc::EmbeddedCurveAdd, - BlackBoxFunc::BigIntAdd => BrilligBlackBoxFunc::BigIntAdd, - BlackBoxFunc::BigIntSub => BrilligBlackBoxFunc::BigIntSub, - BlackBoxFunc::BigIntMul => BrilligBlackBoxFunc::BigIntMul, - BlackBoxFunc::BigIntDiv => BrilligBlackBoxFunc::BigIntDiv, - BlackBoxFunc::BigIntFromLeBytes => BrilligBlackBoxFunc::BigIntFromLeBytes, - BlackBoxFunc::BigIntToLeBytes => BrilligBlackBoxFunc::BigIntToLeBytes, - BlackBoxFunc::Poseidon2Permutation => BrilligBlackBoxFunc::Poseidon2Permutation, - BlackBoxFunc::Sha256Compression => BrilligBlackBoxFunc::Sha256Compression, - } - } -} - #[cfg(test)] mod tests { use strum::IntoEnumIterator; diff --git a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs index 7c636f57aea6..be9ba20ed499 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/black_box.rs @@ -107,149 +107,3 @@ pub enum BlackBoxOp { output_bits: MemoryAddress, }, } - -/// Enum listing Brillig black box functions. There is one-to-one correspondence with the previous enum BlackBoxOp. -#[allow(clippy::upper_case_acronyms)] -#[derive(Clone, Debug, Hash, Copy, PartialEq, Eq, Serialize, Deserialize)] -pub enum BrilligBlackBoxFunc { - /// Ciphers (encrypts) the provided plaintext using AES128 in CBC mode, - /// padding the input using PKCS#7. - /// - inputs: HeapVector of `u8` - /// - iv: initialization vector of type HeapArray `[u8; 16]` - /// - key: HeapArray `[u8; 16]` - /// - outputs: HeapVector of `u8` of length `inputs.len() + (16 - inputs.len() % 16)` - AES128Encrypt, - - /// Computes the Blake2s hash of the message, as specified in - /// https://tools.ietf.org/html/rfc7693 - /// - message: HeapVector of `u8` - /// - output: HeapArray of `u8` of length 32 - Blake2s, - - /// Computes the Blake3 hash of the message. - /// - message: HeapVector of `u8` - /// - output: HeapArray of `u8` of length 32 - Blake3, - - /// Keccak Permutation function of width 1600 - /// - input: HeapArray of `u8` representing 25 64-bit Keccak lanes that represent a keccak sponge of 1600 bits - /// - output: HeapArray of `u8` containing the result of a keccak f1600 permutation on the input state. Also an array of 25 Keccak lanes. - Keccakf1600, - - /// Verifies an ECDSA signature over the secp256k1 curve. - /// - inputs: - /// - x coordinate of public key as 32 bytes (HeapArray of `u8`) - /// - y coordinate of public key as 32 bytes (HeapArray of `u8`) - /// - the signature, as a 64 bytes array (HeapArray of `u8`) - /// - the hash of the message, as a vector of bytes (HeapVector of `u8`) - /// - result: 0 for failure and 1 for success - EcdsaSecp256k1, - - /// Verifies an ECDSA signature over the secp256r1 curve. - /// - /// Same as EcdsaSecp256k1, but done over another curve. - EcdsaSecp256r1, - - /// Multiple scalar multiplication (MSM) with a variable base/input point - /// (P) of the embedded (Grumpkin) curve. An MSM multiplies the points and scalars and - /// sums the results. - /// - input: - /// points: HeapVector of x and y coordinates (and infinity point flag) of the input - /// points `[x1, y1, x2, y2,...]`. - /// scalars: HeapVector of low and high limbs of input - /// scalars `[s1_low, s1_high, s2_low, s2_high, ...]`. (witness, N) - /// - outputs: HeapArray containing a tuple of `x` and `y` coordinates - /// (and infinity point flag) of output. - /// - /// Because the Grumpkin scalar field is bigger than the ACIR field, we - /// provide 2 ACIR fields representing the low and high parts of the Grumpkin - /// scalar $a$: `a=low+high*2^{128}`, with `low, high < 2^{128}` - MultiScalarMul, - - /// Addition over the embedded (Grumpkin) curve. - EmbeddedCurveAdd, - - /// BigInt addition - BigIntAdd, - - /// BigInt subtraction - BigIntSub, - - /// BigInt multiplication - BigIntMul, - - /// BigInt division - BigIntDiv, - - /// BigInt from le bytes - BigIntFromLeBytes, - - /// BigInt to le bytes - BigIntToLeBytes, - - /// Permutation function of Poseidon2 - Poseidon2Permutation, - - /// SHA256 compression function - /// - input: HeapArray of `u32` of length 16 - /// - state (hash_values): HeapArray of `u32` of length 8 - /// - output: HeapArray of `u32` of length 8 - Sha256Compression, - - /// Big-endian radix decomposition - ToRadix, -} - -impl std::fmt::Display for BrilligBlackBoxFunc { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.name()) - } -} - -impl From for BrilligBlackBoxFunc { - fn from(op: BlackBoxOp) -> Self { - match op { - BlackBoxOp::AES128Encrypt { .. } => BrilligBlackBoxFunc::AES128Encrypt, - BlackBoxOp::Blake2s { .. } => BrilligBlackBoxFunc::Blake2s, - BlackBoxOp::Blake3 { .. } => BrilligBlackBoxFunc::Blake3, - BlackBoxOp::Keccakf1600 { .. } => BrilligBlackBoxFunc::Keccakf1600, - BlackBoxOp::EcdsaSecp256k1 { .. } => BrilligBlackBoxFunc::EcdsaSecp256k1, - BlackBoxOp::EcdsaSecp256r1 { .. } => BrilligBlackBoxFunc::EcdsaSecp256r1, - BlackBoxOp::MultiScalarMul { .. } => BrilligBlackBoxFunc::MultiScalarMul, - BlackBoxOp::EmbeddedCurveAdd { .. } => BrilligBlackBoxFunc::EmbeddedCurveAdd, - BlackBoxOp::BigIntAdd { .. } => BrilligBlackBoxFunc::BigIntAdd, - BlackBoxOp::BigIntSub { .. } => BrilligBlackBoxFunc::BigIntSub, - BlackBoxOp::BigIntMul { .. } => BrilligBlackBoxFunc::BigIntMul, - BlackBoxOp::BigIntDiv { .. } => BrilligBlackBoxFunc::BigIntDiv, - BlackBoxOp::BigIntFromLeBytes { .. } => BrilligBlackBoxFunc::BigIntFromLeBytes, - BlackBoxOp::BigIntToLeBytes { .. } => BrilligBlackBoxFunc::BigIntToLeBytes, - BlackBoxOp::Poseidon2Permutation { .. } => BrilligBlackBoxFunc::Poseidon2Permutation, - BlackBoxOp::Sha256Compression { .. } => BrilligBlackBoxFunc::Sha256Compression, - BlackBoxOp::ToRadix { .. } => BrilligBlackBoxFunc::ToRadix, - } - } -} - -impl BrilligBlackBoxFunc { - pub fn name(&self) -> &'static str { - match self { - BrilligBlackBoxFunc::AES128Encrypt => "aes128_encrypt", - BrilligBlackBoxFunc::Blake2s => "blake2s", - BrilligBlackBoxFunc::Blake3 => "blake3", - BrilligBlackBoxFunc::EcdsaSecp256k1 => "ecdsa_secp256k1", - BrilligBlackBoxFunc::MultiScalarMul => "multi_scalar_mul", - BrilligBlackBoxFunc::EmbeddedCurveAdd => "embedded_curve_add", - BrilligBlackBoxFunc::Keccakf1600 => "keccakf1600", - BrilligBlackBoxFunc::EcdsaSecp256r1 => "ecdsa_secp256r1", - BrilligBlackBoxFunc::BigIntAdd => "bigint_add", - BrilligBlackBoxFunc::BigIntSub => "bigint_sub", - BrilligBlackBoxFunc::BigIntMul => "bigint_mul", - BrilligBlackBoxFunc::BigIntDiv => "bigint_div", - BrilligBlackBoxFunc::BigIntFromLeBytes => "bigint_from_le_bytes", - BrilligBlackBoxFunc::BigIntToLeBytes => "bigint_to_le_bytes", - BrilligBlackBoxFunc::Poseidon2Permutation => "poseidon2_permutation", - BrilligBlackBoxFunc::Sha256Compression => "sha256_compression", - BrilligBlackBoxFunc::ToRadix => "to_radix", - } - } -} diff --git a/noir/noir-repo/acvm-repo/brillig/src/lib.rs b/noir/noir-repo/acvm-repo/brillig/src/lib.rs index 9636f6c3960f..cf31ff79996b 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/lib.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/lib.rs @@ -13,7 +13,7 @@ mod black_box; mod foreign_call; mod opcodes; -pub use black_box::{BlackBoxOp, BrilligBlackBoxFunc}; +pub use black_box::BlackBoxOp; pub use foreign_call::{ForeignCallParam, ForeignCallResult}; pub use opcodes::{ BinaryFieldOp, BinaryIntOp, HeapArray, HeapValueType, HeapVector, MemoryAddress, ValueOrArray, diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs index 30edf60d268c..9ebbbd3f0871 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs @@ -1,32 +1,15 @@ -use acir::brillig::{BlackBoxOp, BrilligBlackBoxFunc, HeapArray, HeapVector, IntegerBitSize}; +use acir::brillig::{BlackBoxOp, HeapArray, HeapVector, IntegerBitSize}; use acir::{AcirField, BlackBoxFunc}; use acvm_blackbox_solver::{ aes128_encrypt, blake2s, blake3, ecdsa_secp256k1_verify, ecdsa_secp256r1_verify, keccakf1600, sha256_compression, BigIntSolverWithId, BlackBoxFunctionSolver, BlackBoxResolutionError, }; use num_bigint::BigUint; -use num_traits::{ToPrimitive, Zero}; -use thiserror::Error; +use num_traits::Zero; use crate::memory::MemoryValue; use crate::Memory; -#[derive(Clone, PartialEq, Eq, Debug, Error)] -pub(crate) enum BrilligBlackBoxResolutionError { - #[error("failed to solve brillig blackbox function: {0}, reason: {1}")] - Failed(BrilligBlackBoxFunc, String), -} - -impl From for BrilligBlackBoxResolutionError { - fn from(err: BlackBoxResolutionError) -> Self { - match err { - BlackBoxResolutionError::Failed(func, string) => { - BrilligBlackBoxResolutionError::Failed(func.into(), string) - } - } - } -} - fn read_heap_vector<'a, F: AcirField>( memory: &'a Memory, vector: &HeapVector, @@ -62,23 +45,19 @@ pub(crate) fn evaluate_black_box solver: &Solver, memory: &mut Memory, bigint_solver: &mut BrilligBigIntSolver, -) -> Result<(), BrilligBlackBoxResolutionError> { +) -> Result<(), BlackBoxResolutionError> { match op { BlackBoxOp::AES128Encrypt { inputs, iv, key, outputs } => { + let bb_func = black_box_function_from_op(op); + let inputs = to_u8_vec(read_heap_vector(memory, inputs)); let iv: [u8; 16] = to_u8_vec(read_heap_array(memory, iv)).try_into().map_err(|_| { - BrilligBlackBoxResolutionError::Failed( - (*op).into(), - "Invalid iv length".to_string(), - ) + BlackBoxResolutionError::Failed(bb_func, "Invalid iv length".to_string()) })?; let key: [u8; 16] = to_u8_vec(read_heap_array(memory, key)).try_into().map_err(|_| { - BrilligBlackBoxResolutionError::Failed( - (*op).into(), - "Invalid key length".to_string(), - ) + BlackBoxResolutionError::Failed(bb_func, "Invalid key length".to_string()) })?; let ciphertext = aes128_encrypt(&inputs, iv, key)?; @@ -126,26 +105,25 @@ pub(crate) fn evaluate_black_box signature, result: result_address, } => { + let bb_func = black_box_function_from_op(op); + let public_key_x: [u8; 32] = to_u8_vec(read_heap_array(memory, public_key_x)).try_into().map_err(|_| { - BrilligBlackBoxResolutionError::Failed( - (*op).into(), + BlackBoxResolutionError::Failed( + bb_func, "Invalid public key x length".to_string(), ) })?; let public_key_y: [u8; 32] = to_u8_vec(read_heap_array(memory, public_key_y)).try_into().map_err(|_| { - BrilligBlackBoxResolutionError::Failed( - (*op).into(), + BlackBoxResolutionError::Failed( + bb_func, "Invalid public key y length".to_string(), ) })?; let signature: [u8; 64] = to_u8_vec(read_heap_array(memory, signature)).try_into().map_err(|_| { - BrilligBlackBoxResolutionError::Failed( - (*op).into(), - "Invalid signature length".to_string(), - ) + BlackBoxResolutionError::Failed(bb_func, "Invalid signature length".to_string()) })?; let hashed_msg = to_u8_vec(read_heap_vector(memory, hashed_msg)); @@ -306,8 +284,8 @@ pub(crate) fn evaluate_black_box let mut message = [0; 16]; let inputs = read_heap_array(memory, input); if inputs.len() != 16 { - return Err(BrilligBlackBoxResolutionError::Failed( - BrilligBlackBoxFunc::Sha256Compression, + return Err(BlackBoxResolutionError::Failed( + BlackBoxFunc::Sha256Compression, format!("Expected 16 inputs but encountered {}", &inputs.len()), )); } @@ -317,8 +295,8 @@ pub(crate) fn evaluate_black_box let mut state = [0; 8]; let values = read_heap_array(memory, hash_values); if values.len() != 8 { - return Err(BrilligBlackBoxResolutionError::Failed( - BrilligBlackBoxFunc::Sha256Compression, + return Err(BlackBoxResolutionError::Failed( + BlackBoxFunc::Sha256Compression, format!("Expected 8 values but encountered {}", &values.len()), )); } @@ -338,12 +316,7 @@ pub(crate) fn evaluate_black_box .read(*radix) .expect_integer_with_bit_size(IntegerBitSize::U32) .expect("ToRadix opcode's radix bit size does not match expected bit size 32"); - let num_limbs = memory - .read(*num_limbs) - .expect_integer_with_bit_size(IntegerBitSize::U32) - .expect("ToRadix opcode's number of limbs does not match expected bit size 32") - .to_usize() - .unwrap(); // Will not panic as 32 bits must fit into usize type. + let num_limbs = memory.read(*num_limbs).to_usize(); let output_bits = !memory .read(*output_bits) .expect_integer_with_bit_size(IntegerBitSize::U1) @@ -353,27 +326,6 @@ pub(crate) fn evaluate_black_box let mut input = BigUint::from_bytes_be(&input.to_be_bytes()); let radix = BigUint::from_bytes_be(&radix.to_be_bytes()); - if radix < BigUint::from(2u32) || radix > BigUint::from(256u32) { - return Err(BrilligBlackBoxResolutionError::Failed( - BrilligBlackBoxFunc::ToRadix, - format!("Radix out of the valid range [2,256]. Value: {}", radix), - )); - } - - if num_limbs < 1 && input != BigUint::from(0u32) { - return Err(BrilligBlackBoxResolutionError::Failed( - BrilligBlackBoxFunc::ToRadix, - format!("Input value {} is not zero but number of limbs is zero.", input), - )); - } - - if output_bits && radix != BigUint::from(2u32) { - return Err(BrilligBlackBoxResolutionError::Failed( - BrilligBlackBoxFunc::ToRadix, - format!("Radix {} is not equal to 2 and bit mode is activated.", radix), - )); - } - let mut limbs: Vec> = vec![MemoryValue::default(); num_limbs]; for i in (0..num_limbs).rev() { @@ -396,3 +348,25 @@ pub(crate) fn evaluate_black_box } } } + +fn black_box_function_from_op(op: &BlackBoxOp) -> BlackBoxFunc { + match op { + BlackBoxOp::AES128Encrypt { .. } => BlackBoxFunc::AES128Encrypt, + BlackBoxOp::Blake2s { .. } => BlackBoxFunc::Blake2s, + BlackBoxOp::Blake3 { .. } => BlackBoxFunc::Blake3, + BlackBoxOp::Keccakf1600 { .. } => BlackBoxFunc::Keccakf1600, + BlackBoxOp::EcdsaSecp256k1 { .. } => BlackBoxFunc::EcdsaSecp256k1, + BlackBoxOp::EcdsaSecp256r1 { .. } => BlackBoxFunc::EcdsaSecp256r1, + BlackBoxOp::MultiScalarMul { .. } => BlackBoxFunc::MultiScalarMul, + BlackBoxOp::EmbeddedCurveAdd { .. } => BlackBoxFunc::EmbeddedCurveAdd, + BlackBoxOp::BigIntAdd { .. } => BlackBoxFunc::BigIntAdd, + BlackBoxOp::BigIntSub { .. } => BlackBoxFunc::BigIntSub, + BlackBoxOp::BigIntMul { .. } => BlackBoxFunc::BigIntMul, + BlackBoxOp::BigIntDiv { .. } => BlackBoxFunc::BigIntDiv, + BlackBoxOp::BigIntFromLeBytes { .. } => BlackBoxFunc::BigIntFromLeBytes, + BlackBoxOp::BigIntToLeBytes { .. } => BlackBoxFunc::BigIntToLeBytes, + BlackBoxOp::Poseidon2Permutation { .. } => BlackBoxFunc::Poseidon2Permutation, + BlackBoxOp::Sha256Compression { .. } => BlackBoxFunc::Sha256Compression, + BlackBoxOp::ToRadix { .. } => unreachable!("ToRadix is not an ACIR BlackBoxFunc"), + } +} From b2d279bd91e8a12c9021d23349972d9aac771555 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 22 Jan 2025 09:04:10 +0000 Subject: [PATCH 10/10] 11295: briligvm assertions to safeguard invalid inputs in toradix --- .../acvm-repo/brillig_vm/src/black_box.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs index 9ebbbd3f0871..bc8b1f3c230b 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs @@ -328,6 +328,24 @@ pub(crate) fn evaluate_black_box let mut limbs: Vec> = vec![MemoryValue::default(); num_limbs]; + assert!( + radix >= BigUint::from(2u32) && radix <= BigUint::from(256u32), + "Radix out of the valid range [2,256]. Value: {}", + radix + ); + + assert!( + num_limbs >= 1 || input == BigUint::from(0u32), + "Input value {} is not zero but number of limbs is zero.", + input + ); + + assert!( + !output_bits || radix == BigUint::from(2u32), + "Radix {} is not equal to 2 and bit mode is activated.", + radix + ); + for i in (0..num_limbs).rev() { let limb = &input % &radix; if output_bits {