Skip to content

Commit

Permalink
fix(avm-simulator): hashing opcodes indirection (#5376)
Browse files Browse the repository at this point in the history 
**Situation for hashes before this PR**

The hash situation is quite peculiar. Noir defines its hashes as _black box functions_ in the [stdlib](https://github.com/AztecProtocol/aztec-packages/blob/25caf4d8050cc7446595fcb159f140e7fbee6767/noir/noir-repo/noir_stdlib/src/hash.nr). However, the signature for most of them is not really good for bytecode/avm. E.g., they take and/or return `u8`s instead of Fields, some of them are permutations, others have separators, etc.

Moreover, Brillig preprocessing changes the signature to accept vectors (variable size at runtime) vs arrays (size known at runtime) for some of the black box functions.

The AVM defines:
* [pedersen](https://github.com/AztecProtocol/aztec-packages/blob/25caf4d8050cc7446595fcb159f140e7fbee6767/avm-transpiler/src/transpile.rs#L880): it implements the blackbox function.
* the rest, are defined as [oracles](https://github.com/AztecProtocol/aztec-packages/blob/8a7606875fbb7d3eb15b6d8eaa7e297e1a8838ea/noir-projects/aztec-nr/aztec/src/avm/hash.nr). This means that if a contract uses the stdlib (blackbox) hashes for these functions, it will not currently work with the avm. The definition as oracles is to have a (likely temporary) better signature for the hashes.

Some of the indirections in our pedersen opcode were handled incorrectly (the transpiler was assuming an array, not a vector).

**After the PR**

I started implementing all of the hashing opcodes from the blackbox functions but this was quite difficult to quickly do in TS. So for the moment I'm just fixing the indirection problems.

I also restricted `getSlice` to fail if called on undefined areas, which uncovered a few other bugs in tests.
  • Loading branch information
@fcarreiro
fcarreiro authored Mar 25, 2024
1 parent 933145e commit a4b1ebc
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 84 deletions.
30 changes: 15 additions & 15 deletions avm-transpiler/src/transpile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,8 @@ fn handle_2_field_hash_instruction(
inputs: &[ValueOrArray],
) {
// handle field returns differently
let hash_offset_maybe = inputs[0];
let (hash_offset, hash_size) = match hash_offset_maybe {
let message_offset_maybe = inputs[0];
let (message_offset, message_size) = match message_offset_maybe {
ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0, size),
_ => panic!("Keccak | Sha256 address inputs destination should be a single value"),
};
Expand All @@ -669,16 +669,16 @@ fn handle_2_field_hash_instruction(

avm_instrs.push(AvmInstruction {
opcode,
indirect: Some(3), // 11 - addressing mode, indirect for input and output
indirect: Some(ZEROTH_OPERAND_INDIRECT | FIRST_OPERAND_INDIRECT),
operands: vec![
AvmOperand::U32 {
value: dest_offset as u32,
},
AvmOperand::U32 {
value: hash_offset as u32,
value: message_offset as u32,
},
AvmOperand::U32 {
value: hash_size as u32,
value: message_size as u32,
},
],
..Default::default()
Expand All @@ -701,8 +701,8 @@ fn handle_single_field_hash_instruction(
inputs: &[ValueOrArray],
) {
// handle field returns differently
let hash_offset_maybe = inputs[0];
let (hash_offset, hash_size) = match hash_offset_maybe {
let message_offset_maybe = inputs[0];
let (message_offset, message_size) = match message_offset_maybe {
ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0, size),
_ => panic!("Poseidon address inputs destination should be a single value"),
};
Expand All @@ -724,16 +724,16 @@ fn handle_single_field_hash_instruction(

avm_instrs.push(AvmInstruction {
opcode,
indirect: Some(1),
indirect: Some(FIRST_OPERAND_INDIRECT),
operands: vec![
AvmOperand::U32 {
value: dest_offset as u32,
},
AvmOperand::U32 {
value: hash_offset as u32,
value: message_offset as u32,
},
AvmOperand::U32 {
value: hash_size as u32,
value: message_size as u32,
},
],
..Default::default()
Expand Down Expand Up @@ -882,23 +882,23 @@ fn handle_black_box_function(avm_instrs: &mut Vec<AvmInstruction>, operation: &B
domain_separator: _,
output,
} => {
let hash_offset = inputs.pointer.0;
let hash_size = inputs.size.0;
let message_offset = inputs.pointer.0;
let message_size_offset = inputs.size.0;

let dest_offset = output.0;

avm_instrs.push(AvmInstruction {
opcode: AvmOpcode::PEDERSEN,
indirect: Some(1),
indirect: Some(FIRST_OPERAND_INDIRECT),
operands: vec![
AvmOperand::U32 {
value: dest_offset as u32,
},
AvmOperand::U32 {
value: hash_offset as u32,
value: message_offset as u32,
},
AvmOperand::U32 {
value: hash_size as u32,
value: message_size_offset as u32,
},
],
..Default::default()
Expand Down
8 changes: 4 additions & 4 deletions yarn-project/simulator/src/avm/avm_memory_types.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ describe('TaggedMemory', () => {
expect(mem.get(10)).toStrictEqual(new Field(5));
});

it(`Should getSlice beyond current size`, () => {
it(`Should fail getSlice on unset elements`, () => {
const mem = new TaggedMemory();
const val = [new Field(5), new Field(6)];

mem.setSlice(10, val);
mem.set(10, new Field(10));
mem.set(12, new Field(12));

expect(mem.getSlice(10, /*size=*/ 4)).toEqual([...val, undefined, undefined]);
expect(() => mem.getSlice(10, /*size=*/ 4)).toThrow(/size/);
});

it(`Should set and get slices`, () => {
Expand Down
1 change: 1 addition & 0 deletions yarn-project/simulator/src/avm/avm_memory_types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ export class TaggedMemory {
assert(offset + size < TaggedMemory.MAX_MEMORY_SIZE);
const value = this._mem.slice(offset, offset + size);
TaggedMemory.log(`getSlice(${offset}, ${size}) = ${value}`);
assert(value.length === size, `Expected slice of size ${size}, got ${value.length}.`);
return value;
}

Expand Down
12 changes: 6 additions & 6 deletions yarn-project/simulator/src/avm/opcodes/comparators.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('Comparators', () => {
new Eq(/*indirect=*/ 0, TypeTag.UINT32, /*aOffset=*/ 0, /*bOffset=*/ 3, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 3);
expect(actual).toEqual([new Uint8(0), new Uint8(0), new Uint8(1)]);
});

Expand All @@ -55,7 +55,7 @@ describe('Comparators', () => {
new Eq(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 3, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 3);
expect(actual).toEqual([new Uint8(0), new Uint8(0), new Uint8(1)]);
});

Expand Down Expand Up @@ -106,7 +106,7 @@ describe('Comparators', () => {
new Lt(/*indirect=*/ 0, TypeTag.UINT32, /*aOffset=*/ 0, /*bOffset=*/ 2, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 3);
expect(actual).toEqual([new Uint8(0), new Uint8(1), new Uint8(0)]);
});

Expand All @@ -119,7 +119,7 @@ describe('Comparators', () => {
new Lt(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 2, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 3);
expect(actual).toEqual([new Uint8(0), new Uint8(1), new Uint8(0)]);
});

Expand Down Expand Up @@ -170,7 +170,7 @@ describe('Comparators', () => {
new Lte(/*indirect=*/ 0, TypeTag.UINT32, /*aOffset=*/ 0, /*bOffset=*/ 2, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 3);
expect(actual).toEqual([new Uint8(1), new Uint8(1), new Uint8(0)]);
});

Expand All @@ -183,7 +183,7 @@ describe('Comparators', () => {
new Lte(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 2, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 3);
expect(actual).toEqual([new Uint8(1), new Uint8(1), new Uint8(0)]);
});

Expand Down
Loading

0 comments on commit a4b1ebc

Please sign in to comment.