From 64f4052d498496724ec56b207ca0f89c3fe87ac8 Mon Sep 17 00:00:00 2001 From: Jean M <132435771+jeanmon@users.noreply.github.com> Date: Wed, 22 Jan 2025 17:47:37 +0100 Subject: [PATCH] chore: more granular error handling for toradixBE (#11378) Resolves #11295 --- .../circuit_builder_base.hpp | 2 +- .../src/barretenberg/vm/avm/trace/errors.hpp | 2 +- .../src/barretenberg/vm/avm/trace/helper.cpp | 4 +- .../src/barretenberg/vm/avm/trace/trace.cpp | 9 ++- .../acvm-repo/brillig_vm/src/black_box.rs | 18 +++++ yarn-project/simulator/src/avm/errors.ts | 10 +++ .../src/avm/opcodes/conversion.test.ts | 69 +++++++++++++++++++ .../simulator/src/avm/opcodes/conversion.ts | 21 ++++-- 8 files changed, 122 insertions(+), 13 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 8587fcf98b5..0065e1953c6 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 @@ -384,7 +384,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/barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp index e6613bd7c58..535031969c0 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 021122383a9..0e4a7efb54a 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp @@ -135,8 +135,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 c1cba7fe195..49a89b364ac 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. 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 9ebbbd3f087..bc8b1f3c230 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 { diff --git a/yarn-project/simulator/src/avm/errors.ts b/yarn-project/simulator/src/avm/errors.ts index 64007b35629..9fdd0305960 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 873ab29db5b..a7f1c565821 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]); } }); + + 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; + 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); + }); + + 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; + 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); + }, + ); + + 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; + 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 3699c6f75b1..60de7a8db08 100644 --- a/yarn-project/simulator/src/avm/opcodes/conversion.ts +++ b/yarn-project/simulator/src/avm/opcodes/conversion.ts @@ -1,12 +1,12 @@ import { type AvmContext } from '../avm_context.js'; import { TypeTag, Uint1, Uint8 } from '../avm_memory_types.js'; -import { InstructionExecutionError } 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'; 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. @@ -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(0n)) { + throw new InvalidToRadixInputsError( + `ToRadixBE instruction's input value is not zero (was ${value}) but numLimbs zero.`, + ); } + + if (outputBits != 0 && radix != BigInt(2n)) { + 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);