-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(avm): hashing to simulator #4527
Changes from 7 commits
72a499f
4140bd2
9acb8ff
352de03
0794ec3
07408d4
b3fce3c
35307f2
9ab35ec
cb63cfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
use acvm::acir::brillig::Opcode as BrilligOpcode; | ||
use acvm::acir::circuit::brillig::Brillig; | ||
|
||
use acvm::brillig_vm::brillig::{BinaryFieldOp, BinaryIntOp, MemoryAddress, Value, ValueOrArray}; | ||
use acvm::brillig_vm::brillig::{ | ||
BinaryFieldOp, BinaryIntOp, BlackBoxOp, HeapArray, MemoryAddress, Value, ValueOrArray, | ||
}; | ||
|
||
use crate::instructions::{ | ||
AvmInstruction, AvmOperand, AvmTypeTag, ALL_DIRECT, FIRST_OPERAND_INDIRECT, | ||
|
@@ -93,7 +95,6 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec<u8> { | |
value: destination.to_usize() as u32, | ||
}, | ||
], | ||
..Default::default() | ||
}); | ||
} | ||
BrilligOpcode::CalldataCopy { destination_address, size, offset } => { | ||
|
@@ -200,9 +201,13 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec<u8> { | |
..Default::default() | ||
}); | ||
}, | ||
BrilligOpcode::Cast { destination, source, bit_size } => { | ||
avm_instrs.push(emit_cast(source.to_usize() as u32, destination.to_usize() as u32, tag_from_bit_size(*bit_size))); | ||
} | ||
Comment on lines
+204
to
+206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't support cast before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope |
||
BrilligOpcode::ForeignCall { function, destinations, inputs, destination_value_types:_, input_value_types:_ } => { | ||
handle_foreign_call(&mut avm_instrs, function, destinations, inputs); | ||
}, | ||
BrilligOpcode::BlackBox(operation) => handle_black_box_function(&mut avm_instrs, operation), | ||
_ => panic!( | ||
"Transpiler doesn't know how to process {:?} brillig instruction", | ||
brillig_instr | ||
|
@@ -228,9 +233,121 @@ fn handle_foreign_call( | |
function: &String, | ||
destinations: &Vec<ValueOrArray>, | ||
inputs: &Vec<ValueOrArray>, | ||
) { | ||
match function.as_str() { | ||
"keccak256" | "sha256" => { | ||
handle_2_field_hash_instruction(avm_instrs, function, destinations, inputs) | ||
} | ||
"poseidon" => handle_field_hash_instruction(avm_instrs, function, destinations, inputs), | ||
_ => handle_getter_instruction(avm_instrs, function, destinations, inputs), | ||
} | ||
} | ||
|
||
fn handle_2_field_hash_instruction( | ||
avm_instrs: &mut Vec<AvmInstruction>, | ||
function: &String, | ||
destinations: &[ValueOrArray], | ||
inputs: &[ValueOrArray], | ||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be name Also this is not trivial and could use a meaningful There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. roger that |
||
// handle field returns differently | ||
let hash_offset_maybe = inputs[0]; | ||
println!("hash_offset_maybe: {:?}", hash_offset_maybe); | ||
let (hash_offset, hash_size) = match hash_offset_maybe { | ||
ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0, size), | ||
_ => panic!("Keccak | Sha256 address inputs destination should be a single value"), | ||
}; | ||
|
||
assert!(destinations.len() == 1); | ||
let dest_offset_maybe = destinations[0]; | ||
let dest_offset = match dest_offset_maybe { | ||
ValueOrArray::HeapArray(HeapArray { pointer, size }) => { | ||
assert!(size == 2); | ||
pointer.0 | ||
} | ||
_ => panic!("Keccak | Poseidon address destination should be a single value"), | ||
}; | ||
|
||
let opcode = match function.as_str() { | ||
"keccak256" => AvmOpcode::KECCAK, | ||
"sha256" => AvmOpcode::SHA256, | ||
_ => panic!( | ||
"Transpiler doesn't know how to process ForeignCall function {:?}", | ||
function | ||
), | ||
}; | ||
|
||
avm_instrs.push(AvmInstruction { | ||
opcode, | ||
indirect: Some(3), // 11 - addressing mode, indirect for input and output | ||
operands: vec![ | ||
AvmOperand::U32 { | ||
value: dest_offset as u32, | ||
}, | ||
AvmOperand::U32 { | ||
value: hash_offset as u32, | ||
}, | ||
AvmOperand::U32 { | ||
value: hash_size as u32, | ||
}, | ||
], | ||
..Default::default() | ||
}); | ||
} | ||
|
||
fn handle_field_hash_instruction( | ||
avm_instrs: &mut Vec<AvmInstruction>, | ||
function: &String, | ||
destinations: &[ValueOrArray], | ||
inputs: &[ValueOrArray], | ||
) { | ||
// handle field returns differently | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as other comment: Also this is not trivial and could use a meaningful |
||
let hash_offset_maybe = inputs[0]; | ||
let (hash_offset, hash_size) = match hash_offset_maybe { | ||
ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0, size), | ||
_ => panic!("Poseidon address inputs destination should be a single value"), | ||
}; | ||
|
||
assert!(destinations.len() == 1); | ||
let dest_offset_maybe = destinations[0]; | ||
let dest_offset = match dest_offset_maybe { | ||
ValueOrArray::MemoryAddress(dest_offset) => dest_offset.0, | ||
_ => panic!("Poseidon address destination should be a single value"), | ||
}; | ||
|
||
let opcode = match function.as_str() { | ||
"poseidon" => AvmOpcode::POSEIDON, | ||
_ => panic!( | ||
"Transpiler doesn't know how to process ForeignCall function {:?}", | ||
function | ||
), | ||
}; | ||
|
||
avm_instrs.push(AvmInstruction { | ||
opcode, | ||
indirect: Some(1), | ||
operands: vec![ | ||
AvmOperand::U32 { | ||
value: dest_offset as u32, | ||
}, | ||
AvmOperand::U32 { | ||
value: hash_offset as u32, | ||
}, | ||
AvmOperand::U32 { | ||
value: hash_size as u32, | ||
}, | ||
], | ||
..Default::default() | ||
}); | ||
} | ||
|
||
fn handle_getter_instruction( | ||
avm_instrs: &mut Vec<AvmInstruction>, | ||
function: &String, | ||
destinations: &Vec<ValueOrArray>, | ||
inputs: &Vec<ValueOrArray>, | ||
) { | ||
Comment on lines
+370
to
375
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment, especially explaining what a getter is |
||
// For the foreign calls we want to handle, we do not want inputs, as they are getters | ||
assert!(inputs.len() == 0); | ||
assert!(inputs.is_empty()); | ||
assert!(destinations.len() == 1); | ||
let dest_offset_maybe = destinations[0]; | ||
let dest_offset = match dest_offset_maybe { | ||
|
@@ -257,15 +374,14 @@ fn handle_foreign_call( | |
function | ||
), | ||
}; | ||
|
||
avm_instrs.push(AvmInstruction { | ||
opcode, | ||
indirect: Some(ALL_DIRECT), | ||
operands: vec![AvmOperand::U32 { | ||
value: dest_offset as u32, | ||
}], | ||
..Default::default() | ||
}); | ||
}) | ||
} | ||
|
||
/// Handles Brillig's CONST opcode. | ||
|
@@ -316,7 +432,7 @@ fn emit_set(tag: AvmTypeTag, dest: u32, value: u128) -> AvmInstruction { | |
AvmTypeTag::UINT64 => AvmOperand::U64 { | ||
value: value as u64, | ||
}, | ||
AvmTypeTag::UINT128 => AvmOperand::U128 { value: value }, | ||
AvmTypeTag::UINT128 => AvmOperand::U128 { value }, | ||
_ => panic!("Invalid type tag {:?} for set", tag), | ||
}, | ||
// dest offset | ||
|
@@ -342,7 +458,7 @@ fn emit_cast(source: u32, destination: u32, dst_tag: AvmTypeTag) -> AvmInstructi | |
fn emit_mov(indirect: Option<u8>, source: u32, dest: u32) -> AvmInstruction { | ||
AvmInstruction { | ||
opcode: AvmOpcode::MOV, | ||
indirect: indirect, | ||
indirect, | ||
operands: vec![ | ||
AvmOperand::U32 { value: source }, | ||
AvmOperand::U32 { value: dest }, | ||
|
@@ -351,6 +467,42 @@ fn emit_mov(indirect: Option<u8>, source: u32, dest: u32) -> AvmInstruction { | |
} | ||
} | ||
|
||
fn handle_black_box_function(avm_instrs: &mut Vec<AvmInstruction>, operation: &BlackBoxOp) { | ||
match operation { | ||
BlackBoxOp::PedersenHash { | ||
inputs, | ||
domain_separator: _, | ||
Comment on lines
+499
to
+503
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as other functions (comment, rename?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is clearer though just by name. Simple short comment is probably fine. |
||
output, | ||
} => { | ||
let hash_offset = inputs.pointer.0; | ||
let hash_size = inputs.size.0; | ||
|
||
let dest_offset = output.0; | ||
|
||
avm_instrs.push(AvmInstruction { | ||
opcode: AvmOpcode::PEDERSEN, | ||
indirect: Some(1), | ||
operands: vec![ | ||
AvmOperand::U32 { | ||
value: dest_offset as u32, | ||
}, | ||
AvmOperand::U32 { | ||
value: hash_offset as u32, | ||
}, | ||
AvmOperand::U32 { | ||
value: hash_size as u32, | ||
}, | ||
], | ||
..Default::default() | ||
}); | ||
} | ||
_ => panic!( | ||
"Transpiler doesn't know how to process BlackBoxOp {:?}", | ||
operation | ||
), | ||
} | ||
} | ||
|
||
/// Compute an array that maps each Brillig pc to an AVM pc. | ||
/// This must be done before transpiling to properly transpile jump destinations. | ||
/// This is necessary for two reasons: | ||
|
@@ -367,12 +519,7 @@ fn map_brillig_pcs_to_avm_pcs(initial_offset: usize, brillig: &Brillig) -> Vec<u | |
pc_map[0] = initial_offset; | ||
for i in 0..brillig.bytecode.len() - 1 { | ||
let num_avm_instrs_for_this_brillig_instr = match &brillig.bytecode[i] { | ||
BrilligOpcode::Load { .. } => 2, | ||
BrilligOpcode::Store { .. } => 2, | ||
Comment on lines
-370
to
-371
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this a bug where these were left as 2 👀 oof that would've been a time killer debugging that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably took an hour to find lmao, the avm got stuck in a loop and i couldnt work out why it was reverting |
||
BrilligOpcode::Const { bit_size, .. } => match bit_size { | ||
254 => 2, // Field. | ||
_ => 1, | ||
}, | ||
BrilligOpcode::Const { bit_size: 254, .. } => 2, | ||
_ => 1, | ||
}; | ||
// next Brillig pc will map to an AVM pc offset by the | ||
|
@@ -384,6 +531,7 @@ fn map_brillig_pcs_to_avm_pcs(initial_offset: usize, brillig: &Brillig) -> Vec<u | |
|
||
fn tag_from_bit_size(bit_size: u32) -> AvmTypeTag { | ||
match bit_size { | ||
1 => AvmTypeTag::UINT8, // temp workaround | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. brillig did not like me not having this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine. I do worry with temporary workarounds like this that some Noir code just won't work right in the AVM and we'll spend time debugging only to realize this. Link this comment to @sirasistant's avm wishlist that mentions u1 support |
||
8 => AvmTypeTag::UINT8, | ||
16 => AvmTypeTag::UINT16, | ||
32 => AvmTypeTag::UINT32, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
mod context; | ||
mod hash; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
#[oracle(keccak256)] | ||
pub fn keccak256<N>(input: [Field; N]) -> [Field; 2] {} | ||
|
||
#[oracle(poseidon)] | ||
pub fn poseidon<N>(input: [Field; N]) -> Field {} | ||
|
||
#[oracle(sha256)] | ||
pub fn sha256<N>(input: [Field; N]) -> [Field; 2] {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,16 @@ | ||
// | ||
|
||
contract AvmTest { | ||
// Libs | ||
use dep::aztec::protocol_types::address::{AztecAddress, EthAddress}; | ||
|
||
// avm lib | ||
use dep::aztec::avm::context::AvmContext; | ||
use dep::aztec::avm::{ | ||
context::AvmContext, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Special opcodes for these using the oracles, this way we dont have to deal with the current refactor of these ( for now )- where just the permutation is being put in a gadget in hash.nr |
||
hash::{ | ||
keccak256, | ||
poseidon, | ||
sha256 | ||
} | ||
}; | ||
|
||
#[aztec(private)] | ||
fn constructor() {} | ||
|
@@ -51,9 +56,32 @@ contract AvmTest { | |
200 as Field | ||
} | ||
|
||
/************************************************************************ | ||
* AvmContext functions | ||
************************************************************************/ | ||
// /************************************************************************ | ||
// * Hashing functions | ||
// ************************************************************************/ | ||
#[aztec(public-vm)] | ||
fn keccak_hash(data: [Field; 3]) -> pub [Field; 2] { | ||
keccak256(data) | ||
} | ||
|
||
#[aztec(public-vm)] | ||
fn poseidon_hash(data: [Field; 3]) -> pub Field { | ||
poseidon(data) | ||
} | ||
|
||
#[aztec(public-vm)] | ||
fn sha256_hash(data: [Field; 3]) -> pub [Field; 2] { | ||
sha256(data) | ||
} | ||
|
||
#[aztec(public-vm)] | ||
fn pedersen_hash(data: [Field; 3]) -> pub Field { | ||
dep::std::hash::pedersen_hash(data) | ||
} | ||
|
||
// /************************************************************************ | ||
// * AvmContext functions | ||
// ************************************************************************/ | ||
#[aztec(public-vm)] | ||
fn getAddress() -> pub AztecAddress { | ||
context.address() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo fmt changes