Skip to content

Commit

Permalink
chore: more granular error handling for toradixBE (#11378)
Browse files Browse the repository at this point in the history
Resolves #11295
  • Loading branch information
jeanmon authored Jan 22, 2025
1 parent b1cb502 commit 64f4052
Show file tree
Hide file tree
Showing 8 changed files with 122 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ template <typename FF> 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].
*
Expand Down
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/helper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
9 changes: 6 additions & 3 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5019,9 +5019,12 @@ AvmError AvmTraceBuilder::op_to_radix_be(uint16_t indirect,
// uint32_t radix = static_cast<uint32_t>(read_radix.val);
uint32_t radix = static_cast<uint32_t>(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.
Expand Down
18 changes: 18 additions & 0 deletions noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,24 @@ pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>

let mut limbs: Vec<MemoryValue<F>> = 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 {
Expand Down
10 changes: 10 additions & 0 deletions yarn-project/simulator/src/avm/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
69 changes: 69 additions & 0 deletions yarn-project/simulator/src/avm/opcodes/conversion.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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);
},
);
});
});
21 changes: 15 additions & 6 deletions yarn-project/simulator/src/avm/opcodes/conversion.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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);

Expand Down

1 comment on commit 64f4052

@AztecBot
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 64f4052 Previous: fe34b05 Ratio
wasmconstruct_proof_ultrahonk_power_of_2/20 14718.318547 ms/iter 13536.199834000001 ms/iter 1.09

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

Please sign in to comment.