Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat: Solve Keccak opcodes #247

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions acvm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ acir.workspace = true
stdlib.workspace = true

sha2 = "0.9.3"
sha3 = "0.10.7"
crc32fast = "1.3.2"
k256 = { version = "0.7.2", features = [
"ecdsa",
Expand Down
11 changes: 9 additions & 2 deletions acvm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ use acir::{
native_types::{Expression, Witness},
BlackBoxFunc,
};
use pwg::{block::Blocks, directives::solve_directives};
use pwg::{
black_box_functions::solve_black_box_function, block::Blocks, directives::solve_directives,
};
use std::collections::BTreeMap;
use thiserror::Error;

Expand Down Expand Up @@ -122,7 +124,12 @@ pub trait PartialWitnessGenerator {
unassigned_witness.0,
)))
} else {
self.solve_black_box_function_call(initial_witness, bb_func)
let status = solve_black_box_function(initial_witness, bb_func);
if matches!(status, Err(OpcodeResolutionError::OpcodeNotSolvable(_))) {
self.solve_black_box_function_call(initial_witness, bb_func)
} else {
status
}
Comment on lines +127 to +132
Copy link
Member

@TomAFrench TomAFrench Apr 28, 2023

Choose a reason for hiding this comment

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

This change is unrelated to supporting keccak opcodes and has significant knock on effects for all backends so shouldn't be included in this PR.

This change is breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is a breaking change, what is broken?

Copy link
Member

Choose a reason for hiding this comment

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

It's changing the contract between backends and ACVM.

Previously ACVM provided reference methods for executing black box functions to the backends however backends were in charge of solving the black box function and returning the result. After this change this is still the case for all other opcodes but we'll never send keccak256 in particular to the backend.

This split between keccak256 and all the other functions doesn't make sense and we shouldn't split responsibility for black box function execution between the backend and the ACVM. Until we have a rust implementation of all black box functions then we should allow the backend to handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in fact this is not breaking anything.
Solving Keccak in ACVM does make sense, as well as all other 'blackbox'. I will move the solving into ACVM for all but pedersen, which will still be handled by the backend until we have a rust implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change would mean that a lot of code in aztec_backend would become dead code without us actually realizing it. The backend has a match on all black box functions but those code paths would never be executed.

This could lead to confusing bugs between Noir and the backend and it seems hard to communicate to backend implementors.

I'm torn on where this should happen because I don't want aztec_backend to implement keccak itself 😬 I'd probably be fine making a separate PR for this change, just so we can mark it breaking and have a clear CHANGELOG entry. @TomAFrench thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course we would remove the dead code as long as it is integrated in ACVM, so I don't see this as an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course we would remove the dead code as long as it is integrated in ACVM, so I don't see this as an issue.

This assumes that we are in control of all the backends, but we need to develop as though there are other backends that need to integrate ACVM and they will be confused without accurate documentation & changelog entries.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could land something like #257 before this and then it wouldn't be an issue.

}
}
Opcode::Directive(directive) => solve_directives(initial_witness, directive),
Expand Down
1 change: 1 addition & 0 deletions acvm/src/pwg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub mod arithmetic;
// Directives
pub mod directives;
// black box functions
pub mod black_box_functions;
pub mod block;
pub mod hash;
pub mod logic;
Expand Down
34 changes: 34 additions & 0 deletions acvm/src/pwg/black_box_functions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use std::collections::BTreeMap;

use acir::{circuit::opcodes::BlackBoxFuncCall, native_types::Witness, BlackBoxFunc, FieldElement};

use crate::{OpcodeNotSolvable, OpcodeResolution, OpcodeResolutionError};

use super::hash;

pub fn solve_black_box_function(
initial_witness: &mut BTreeMap<Witness, FieldElement>,
func_call: &BlackBoxFuncCall,
) -> Result<OpcodeResolution, OpcodeResolutionError> {
match func_call.name {
BlackBoxFunc::AES
| BlackBoxFunc::AND
| BlackBoxFunc::XOR
| BlackBoxFunc::RANGE
| BlackBoxFunc::SHA256
| BlackBoxFunc::Blake2s
| BlackBoxFunc::ComputeMerkleRoot
| BlackBoxFunc::SchnorrVerify
| BlackBoxFunc::Pedersen
| BlackBoxFunc::HashToField128Security
| BlackBoxFunc::EcdsaSecp256k1
| BlackBoxFunc::FixedBaseScalarMul => {
Err(OpcodeResolutionError::OpcodeNotSolvable(OpcodeNotSolvable::MissingAssignment(0)))
}
//self.solve_black_box_function_call(initial_witness, func_call),
BlackBoxFunc::Keccak256 => {
hash::keccak256(initial_witness, func_call)?;
Ok(OpcodeResolution::Solved)
}
}
}
41 changes: 41 additions & 0 deletions acvm/src/pwg/hash.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use acir::{circuit::opcodes::BlackBoxFuncCall, native_types::Witness, FieldElement};
use blake2::{Blake2s, Digest};
use sha2::Sha256;
use sha3::Keccak256;
use std::collections::BTreeMap;

use crate::{OpcodeResolution, OpcodeResolutionError};
Expand Down Expand Up @@ -72,3 +73,43 @@ fn generic_hash_256<D: Digest>(
let result = hasher.finalize().as_slice().try_into().unwrap();
Ok(result)
}

pub fn keccak256(
initial_witness: &mut BTreeMap<Witness, FieldElement>,
gadget_call: &BlackBoxFuncCall,
) -> Result<OpcodeResolution, OpcodeResolutionError> {
generic_sha3::<Keccak256>(initial_witness, gadget_call)?;
Ok(OpcodeResolution::Solved)
}

fn generic_sha3<D: sha3::Digest>(
Copy link
Member

Choose a reason for hiding this comment

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

This is the same Digest trait as used by generic_hash_256 so we don't need a duplicate implementation ( you may need to adjust versions of dependencies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not the same because the two crates do not use the same version.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, you'll need to adjust the dependency version for the blake2 crate to match.

initial_witness: &mut BTreeMap<Witness, FieldElement>,
gadget_call: &BlackBoxFuncCall,
) -> Result<(), OpcodeResolutionError> {
let mut hasher = D::new();

// For each input in the vector of inputs, check if we have their witness assignments (Can do this outside of match, since they all have inputs)
for input_index in gadget_call.inputs.iter() {
let witness = &input_index.witness;
let num_bits = input_index.num_bits;

let witness_assignment = initial_witness.get(witness);
let assignment = match witness_assignment {
None => panic!("cannot find witness assignment for {witness:?}"),
Some(assignment) => assignment,
};

let bytes = assignment.fetch_nearest_bytes(num_bits as usize);
hasher.update(bytes);
}
let result = hasher.finalize();
assert_eq!(result.len(), 32);
for i in 0..32 {
insert_value(
&gadget_call.outputs[i],
FieldElement::from_be_bytes_reduce(&[result[i]]),
initial_witness,
)?;
}
Ok(())
}