From 51a098e25599a00ed5e2f9c798aa81fb8b697ca2 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 12 Jun 2023 05:59:51 +0000 Subject: [PATCH 01/18] chore!: remove deprecated `Oracle` opcode --- acir/src/circuit/mod.rs | 20 +------ acir/src/circuit/opcodes.rs | 8 --- acir/src/circuit/opcodes/oracle_data.rs | 41 -------------- acvm/src/compiler/transformers/fallback.rs | 4 +- acvm/src/pwg/mod.rs | 65 +++------------------- acvm/src/pwg/oracle.rs | 44 --------------- 6 files changed, 12 insertions(+), 170 deletions(-) delete mode 100644 acir/src/circuit/opcodes/oracle_data.rs delete mode 100644 acvm/src/pwg/oracle.rs diff --git a/acir/src/circuit/mod.rs b/acir/src/circuit/mod.rs index f45d7cc2e..90b7bbea2 100644 --- a/acir/src/circuit/mod.rs +++ b/acir/src/circuit/mod.rs @@ -112,10 +112,10 @@ mod tests { use std::collections::BTreeSet; use super::{ - opcodes::{BlackBoxFuncCall, FunctionInput, OracleData}, + opcodes::{BlackBoxFuncCall, FunctionInput}, Circuit, Opcode, PublicInputs, }; - use crate::native_types::{Expression, Witness}; + use crate::native_types::Witness; use acir_field::FieldElement; fn directive_opcode() -> Opcode { @@ -136,25 +136,12 @@ mod tests { input: FunctionInput { witness: Witness(1), num_bits: 8 }, }) } - fn oracle_opcode() -> Opcode { - Opcode::Oracle(OracleData { - name: String::from("oracle-name"), - inputs: vec![Expression { - mul_terms: vec![(FieldElement::from(123u128), Witness(1), Witness(2))], - linear_combinations: vec![(FieldElement::from(456u128), Witness(34))], - q_c: FieldElement::from(12345678u128), - }], - input_values: vec![], - outputs: vec![Witness(1), Witness(2), Witness(3)], - output_values: vec![], - }) - } #[test] fn serialization_roundtrip() { let circuit = Circuit { current_witness_index: 5, - opcodes: vec![and_opcode(), range_opcode(), oracle_opcode(), directive_opcode()], + opcodes: vec![and_opcode(), range_opcode(), directive_opcode()], public_parameters: PublicInputs(BTreeSet::from_iter(vec![Witness(2), Witness(12)])), return_values: PublicInputs(BTreeSet::from_iter(vec![Witness(4), Witness(12)])), }; @@ -182,7 +169,6 @@ mod tests { }), range_opcode(), and_opcode(), - oracle_opcode(), ], public_parameters: PublicInputs(BTreeSet::from_iter(vec![Witness(2)])), return_values: PublicInputs(BTreeSet::from_iter(vec![Witness(2)])), diff --git a/acir/src/circuit/opcodes.rs b/acir/src/circuit/opcodes.rs index 59688c628..1e285254e 100644 --- a/acir/src/circuit/opcodes.rs +++ b/acir/src/circuit/opcodes.rs @@ -7,11 +7,9 @@ use serde::{Deserialize, Serialize}; mod black_box_function_call; mod block; -mod oracle_data; pub use black_box_function_call::{BlackBoxFuncCall, FunctionInput}; pub use block::{BlockId, MemOp, MemoryBlock}; -pub use oracle_data::OracleData; #[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum Opcode { @@ -37,7 +35,6 @@ pub enum Opcode { // TODO(#319): Review this comment and generalize it to be useful for other backends. // RAM is required for acvm-backend-barretenberg as dynamic memory implementation in Barretenberg requires an initialization phase and can only handle constant values for operations. RAM(MemoryBlock), - Oracle(OracleData), Brillig(Brillig), } @@ -52,7 +49,6 @@ impl Opcode { Opcode::Block(_) => "block", Opcode::RAM(_) => "ram", Opcode::ROM(_) => "rom", - Opcode::Oracle(data) => &data.name, Opcode::Brillig(_) => "brillig", } } @@ -150,10 +146,6 @@ impl std::fmt::Display for Opcode { write!(f, "RAM ")?; write!(f, "(id: {}, len: {}) ", block.id.0, block.trace.len()) } - Opcode::Oracle(data) => { - write!(f, "ORACLE: ")?; - write!(f, "{data}") - } Opcode::Brillig(brillig) => { write!(f, "BRILLIG: ")?; writeln!(f, "inputs: {:?}", brillig.inputs)?; diff --git a/acir/src/circuit/opcodes/oracle_data.rs b/acir/src/circuit/opcodes/oracle_data.rs deleted file mode 100644 index b2843cc1f..000000000 --- a/acir/src/circuit/opcodes/oracle_data.rs +++ /dev/null @@ -1,41 +0,0 @@ -use crate::native_types::{Expression, Witness}; -use acir_field::FieldElement; -use serde::{Deserialize, Serialize}; - -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] -pub struct OracleData { - /// Name of the oracle - pub name: String, - /// Inputs - pub inputs: Vec, - /// Input values - they are progressively computed by the pwg - pub input_values: Vec, - /// Output witness - pub outputs: Vec, - /// Output values - they are computed by the (external) oracle once the input_values are known - pub output_values: Vec, -} - -impl std::fmt::Display for OracleData { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "ORACLE: {}", self.name)?; - let solved = if self.input_values.len() == self.inputs.len() { "solved" } else { "" }; - - if !self.inputs.is_empty() { - write!( - f, - "Inputs: _{}..._{}{solved}", - self.inputs.first().unwrap(), - self.inputs.last().unwrap() - )?; - } - - let solved = if self.output_values.len() == self.outputs.len() { "solved" } else { "" }; - write!( - f, - "Outputs: _{}..._{}{solved}", - self.outputs.first().unwrap().witness_index(), - self.outputs.last().unwrap().witness_index() - ) - } -} diff --git a/acvm/src/compiler/transformers/fallback.rs b/acvm/src/compiler/transformers/fallback.rs index ed5fe7a82..c1ad43061 100644 --- a/acvm/src/compiler/transformers/fallback.rs +++ b/acvm/src/compiler/transformers/fallback.rs @@ -32,10 +32,8 @@ impl FallbackTransformer { | Opcode::Brillig(_) | Opcode::Block(_) | Opcode::ROM(_) - | Opcode::RAM(_) - | Opcode::Oracle { .. } => { + | Opcode::RAM(_) => { // directive, arithmetic expression or blocks are handled by acvm - // The oracle opcode is assumed to be supported. acir_supported_opcodes.push(opcode); continue; } diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 31da6fa31..64a073601 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -3,16 +3,12 @@ use crate::{Language, PartialWitnessGenerator}; use acir::{ brillig_vm::ForeignCallResult, - circuit::brillig::Brillig, - circuit::opcodes::{Opcode, OracleData}, + circuit::{brillig::Brillig, Opcode}, native_types::{Expression, Witness, WitnessMap}, BlackBoxFunc, FieldElement, }; -use self::{ - arithmetic::ArithmeticSolver, brillig::BrilligSolver, directives::solve_directives, - oracle::OracleSolver, -}; +use self::{arithmetic::ArithmeticSolver, brillig::BrilligSolver, directives::solve_directives}; use thiserror::Error; @@ -25,7 +21,6 @@ mod directives; // black box functions mod blackbox; mod block; -mod oracle; // Re-export `Blocks` so that it can be passed to `pwg::solve` pub use block::Blocks; @@ -45,7 +40,6 @@ pub enum PartialWitnessGeneratorStatus { /// /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the remaining opcodes. RequiresOracleData { - required_oracle_data: Vec, unsolved_opcodes: Vec, unresolved_brillig_calls: Vec, }, @@ -103,14 +97,12 @@ pub fn solve( mut opcode_to_solve: Vec, ) -> Result { let mut unresolved_opcodes: Vec = Vec::new(); - let mut unresolved_oracles: Vec = Vec::new(); let mut unresolved_brillig_calls: Vec = Vec::new(); - while !opcode_to_solve.is_empty() || !unresolved_oracles.is_empty() { + while !opcode_to_solve.is_empty() { unresolved_opcodes.clear(); let mut stalled = true; let mut opcode_not_solvable = None; for opcode in &opcode_to_solve { - let mut solved_oracle_data = None; let mut solved_brillig_data = None; let resolution = match opcode { Opcode::Arithmetic(expr) => ArithmeticSolver::solve(initial_witness, expr), @@ -121,12 +113,6 @@ pub fn solve( Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { blocks.solve(block.id, &block.trace, initial_witness) } - Opcode::Oracle(data) => { - let mut data_clone = data.clone(); - let result = OracleSolver::solve(initial_witness, &mut data_clone)?; - solved_oracle_data = Some(data_clone); - Ok(result) - } Opcode::Brillig(brillig) => { let mut brillig_clone = brillig.clone(); let result = BrilligSolver::solve(initial_witness, &mut brillig_clone); @@ -140,12 +126,7 @@ pub fn solve( } Ok(OpcodeResolution::InProgress) => { stalled = false; - // InProgress Oracles must be externally re-solved - if let Some(oracle) = solved_oracle_data { - unresolved_oracles.push(oracle); - } else { - unresolved_opcodes.push(opcode.clone()); - } + unresolved_opcodes.push(opcode.clone()); } Ok(OpcodeResolution::InProgressBrillig(oracle_wait_info)) => { stalled = false; @@ -167,9 +148,7 @@ pub fn solve( // We push those opcodes not solvable to the back as // it could be because the opcodes are out of order, i.e. this assignment // relies on a later opcodes' results - if let Some(oracle_data) = solved_oracle_data { - unresolved_opcodes.push(Opcode::Oracle(oracle_data)); - } else if let Some(brillig) = solved_brillig_data { + if let Some(brillig) = solved_brillig_data { unresolved_opcodes.push(Opcode::Brillig(brillig)); } else { unresolved_opcodes.push(opcode.clone()); @@ -182,9 +161,8 @@ pub fn solve( } } // We have oracles that must be externally resolved - if !unresolved_oracles.is_empty() || !unresolved_brillig_calls.is_empty() { + if !unresolved_brillig_calls.is_empty() { return Ok(PartialWitnessGeneratorStatus::RequiresOracleData { - required_oracle_data: unresolved_oracles, unsolved_opcodes: unresolved_opcodes, unresolved_brillig_calls, }); @@ -314,7 +292,7 @@ mod tests { circuit::{ brillig::{Brillig, BrilligInputs, BrilligOutputs}, directives::Directive, - opcodes::{FunctionInput, OracleData}, + opcodes::FunctionInput, Opcode, }, native_types::{Expression, Witness, WitnessMap}, @@ -376,17 +354,6 @@ mod tests { let w_z = Witness(4); let w_z_inverse = Witness(5); let opcodes = vec![ - Opcode::Oracle(OracleData { - name: "invert".into(), - inputs: vec![Expression { - mul_terms: vec![], - linear_combinations: vec![(fe_1, w_x), (fe_1, w_y)], - q_c: fe_0, - }], - input_values: vec![], - outputs: vec![w_oracle], - output_values: vec![], - }), Opcode::Arithmetic(Expression { mul_terms: vec![], linear_combinations: vec![(fe_1, w_x), (fe_1, w_y), (-fe_1, w_z)], @@ -414,23 +381,7 @@ mod tests { .into(); let mut blocks = Blocks::default(); let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresOracleData { mut required_oracle_data, unsolved_opcodes, .. } = solver_status else { - panic!("Should require oracle data") - }; - assert!(unsolved_opcodes.is_empty(), "oracle should be removed"); - assert_eq!(required_oracle_data.len(), 1, "should have an oracle request"); - let mut oracle_data = required_oracle_data.remove(0); - - assert_eq!(oracle_data.input_values.len(), 1, "Should have solved a single input"); - - // Filling data request and continue solving - oracle_data.output_values = vec![oracle_data.input_values.last().unwrap().inverse()]; - let mut next_opcodes_for_solving = vec![Opcode::Oracle(oracle_data)]; - next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); - let solver_status = - pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should be solvable"); + .expect("should be solvable"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } diff --git a/acvm/src/pwg/oracle.rs b/acvm/src/pwg/oracle.rs deleted file mode 100644 index e6db9221f..000000000 --- a/acvm/src/pwg/oracle.rs +++ /dev/null @@ -1,44 +0,0 @@ -use acir::{circuit::opcodes::OracleData, native_types::WitnessMap}; - -use super::{arithmetic::ArithmeticSolver, insert_value}; -use super::{OpcodeNotSolvable, OpcodeResolution, OpcodeResolutionError}; - -pub(super) struct OracleSolver; - -impl OracleSolver { - /// Derives the rest of the witness based on the initial low level variables - pub(super) fn solve( - initial_witness: &mut WitnessMap, - data: &mut OracleData, - ) -> Result { - // Set input values - for input in data.inputs.iter().skip(data.input_values.len()) { - let solve = ArithmeticSolver::evaluate(input, initial_witness); - if let Some(value) = solve.to_const() { - data.input_values.push(value); - } else { - break; - } - } - - // If all of the inputs to the oracle have assignments - if data.input_values.len() == data.inputs.len() { - if data.output_values.len() == data.outputs.len() { - for (out, value) in data.outputs.iter().zip(data.output_values.iter()) { - insert_value(out, *value, initial_witness)?; - } - Ok(OpcodeResolution::Solved) - } else { - // Missing output values - Ok(OpcodeResolution::InProgress) - } - } else { - Ok(OpcodeResolution::Stalled(OpcodeNotSolvable::ExpressionHasTooManyUnknowns( - data.inputs - .last() - .expect("Infallible: cannot reach this point if no inputs") - .clone(), - ))) - } - } -} From 502109e1943a068c3c001b510caa9dc9ff22ff36 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 12 Jun 2023 16:47:11 +0000 Subject: [PATCH 02/18] chore: rename `PartialWitnessGeneratorStatus::RequiresOracleData` to `RequiresForeignCall` --- acvm/src/pwg/mod.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 64a073601..087f49fc7 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -30,16 +30,12 @@ pub enum PartialWitnessGeneratorStatus { /// All opcodes have been solved. Solved, - /// The `PartialWitnessGenerator` has encountered a request for [oracle data][Opcode::Oracle] or a Brillig [foreign call][acir::brillig_vm::Opcode::ForeignCall]. + /// The `PartialWitnessGenerator` has encountered a request for a Brillig [foreign call][acir::brillig_vm::Opcode::ForeignCall] + /// to retrieve information from outside of the ACVM. + /// The result of the foreign call is inserted into the `Brillig` opcode which made the call using [`UnresolvedBrilligCall::resolve`]. /// - /// Both of these opcodes require information from outside of the ACVM to be inserted before restarting execution. - /// [`Opcode::Oracle`] and [`Opcode::Brillig`] opcodes require the return values to be inserted slightly differently. - /// `Oracle` opcodes expect their return values to be written directly into the witness map whereas a `Brillig` foreign call - /// result is inserted into the `Brillig` opcode which made the call using [`UnresolvedBrilligCall::resolve`]. - /// (Note: this means that the updated opcode must then be passed back into the ACVM to be processed further.) - /// - /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the remaining opcodes. - RequiresOracleData { + /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the new set of opcodes. + RequiresForeignCall { unsolved_opcodes: Vec, unresolved_brillig_calls: Vec, }, @@ -162,7 +158,7 @@ pub fn solve( } // We have oracles that must be externally resolved if !unresolved_brillig_calls.is_empty() { - return Ok(PartialWitnessGeneratorStatus::RequiresOracleData { + return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes: unresolved_opcodes, unresolved_brillig_calls, }); @@ -472,7 +468,7 @@ mod tests { // use the partial witness generation solver with our acir program let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) .expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresOracleData { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -606,7 +602,7 @@ mod tests { // use the partial witness generation solver with our acir program let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) .expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresOracleData { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -630,7 +626,7 @@ mod tests { let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) .expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresOracleData { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; From 15eec9d6ea84b62ca00aa4c6bec557f564b47e6b Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 13 Jun 2023 05:49:29 +0100 Subject: [PATCH 03/18] Update acvm/src/pwg/mod.rs --- acvm/src/pwg/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 087f49fc7..915a8c50d 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -156,7 +156,7 @@ pub fn solve( Err(err) => return Err(err), } } - // We have oracles that must be externally resolved + // We have foreign calls that must be externally resolved if !unresolved_brillig_calls.is_empty() { return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes: unresolved_opcodes, From 7cf72b414592555eb1cf66481aec6bf23a17012a Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 14 Jun 2023 04:15:38 +0000 Subject: [PATCH 04/18] chore: cleanup unwanted test case --- acvm/src/pwg/mod.rs | 46 --------------------------------------------- 1 file changed, 46 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 915a8c50d..8bf5c6ca7 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -335,52 +335,6 @@ mod tests { } } - #[test] - fn inversion_oracle_equivalence() { - // Opcodes below describe the following: - // fn main(x : Field, y : pub Field) { - // let z = x + y; - // constrain 1/z == Oracle("inverse", x + y); - // } - let fe_0 = FieldElement::zero(); - let fe_1 = FieldElement::one(); - let w_x = Witness(1); - let w_y = Witness(2); - let w_oracle = Witness(3); - let w_z = Witness(4); - let w_z_inverse = Witness(5); - let opcodes = vec![ - Opcode::Arithmetic(Expression { - mul_terms: vec![], - linear_combinations: vec![(fe_1, w_x), (fe_1, w_y), (-fe_1, w_z)], - q_c: fe_0, - }), - Opcode::Directive(Directive::Invert { x: w_z, result: w_z_inverse }), - Opcode::Arithmetic(Expression { - mul_terms: vec![(fe_1, w_z, w_z_inverse)], - linear_combinations: vec![], - q_c: -fe_1, - }), - Opcode::Arithmetic(Expression { - mul_terms: vec![], - linear_combinations: vec![(-fe_1, w_oracle), (fe_1, w_z_inverse)], - q_c: fe_0, - }), - ]; - - let backend = StubbedPwg; - - let mut witness_assignments = BTreeMap::from([ - (Witness(1), FieldElement::from(2u128)), - (Witness(2), FieldElement::from(3u128)), - ]) - .into(); - let mut blocks = Blocks::default(); - let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should be solvable"); - assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); - } - #[test] fn inversion_brillig_oracle_equivalence() { // Opcodes below describe the following: From f241b6ceb45ba2a9493d4a9e12736f826421629e Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 14 Jun 2023 04:18:46 +0000 Subject: [PATCH 05/18] chore: replace expect strings --- acvm/src/pwg/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 8bf5c6ca7..6b0b2b6a0 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -421,9 +421,9 @@ mod tests { let mut blocks = Blocks::default(); // use the partial witness generation solver with our acir program let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on oracle"); + .expect("should stall on brillig call"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require oracle data") + panic!("Should require foreign call resolution") }; assert_eq!(unsolved_opcodes.len(), 0, "brillig should have been removed"); @@ -445,7 +445,7 @@ mod tests { // After filling data request, continue solving let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should not stall on oracle"); + .expect("should not stall on brillig call"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -555,9 +555,9 @@ mod tests { let mut blocks = Blocks::default(); // use the partial witness generation solver with our acir program let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on oracle"); + .expect("should stall on brillig call"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require oracle data") + panic!("Should require foreign call resolution") }; assert_eq!(unsolved_opcodes.len(), 0, "brillig should have been removed"); @@ -579,9 +579,9 @@ mod tests { // After filling data request, continue solving let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should stall on oracle"); + .expect("should stall on brillig call"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require oracle data") + panic!("Should require foreign call resolution") }; assert!(unsolved_opcodes.is_empty(), "should be fully solved"); @@ -605,7 +605,7 @@ mod tests { // After filling data request, continue solving let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should not stall on oracle"); + .expect("should not stall on brillig call"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -690,7 +690,7 @@ mod tests { .into(); let mut blocks = Blocks::default(); let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should not stall on oracle"); + .expect("should not stall on brillig call"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } } From f8072997613ccee3bab9eeaeaf45fabdd94f7b3f Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 16 Jun 2023 07:41:23 +0000 Subject: [PATCH 06/18] feat: attach backend implementation to `ACVM` struct --- acvm/src/pwg/mod.rs | 215 +++++++++++++++++++++++--------------------- 1 file changed, 114 insertions(+), 101 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 1688da5d7..303d27b89 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -85,84 +85,94 @@ pub enum OpcodeResolutionError { BrilligFunctionFailed(String), } -/// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. -pub fn solve( - backend: &impl PartialWitnessGenerator, - initial_witness: &mut WitnessMap, - blocks: &mut Blocks, - mut opcode_to_solve: Vec, -) -> Result { - let mut unresolved_opcodes: Vec = Vec::new(); - let mut unresolved_brillig_calls: Vec = Vec::new(); - while !opcode_to_solve.is_empty() { - unresolved_opcodes.clear(); - let mut stalled = true; - let mut opcode_not_solvable = None; - for opcode in &opcode_to_solve { - let resolution = match opcode { - Opcode::Arithmetic(expr) => ArithmeticSolver::solve(initial_witness, expr), - Opcode::BlackBoxFuncCall(bb_func) => { - blackbox::solve(backend, initial_witness, bb_func) - } - Opcode::Directive(directive) => solve_directives(initial_witness, directive), - Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { - blocks.solve(block.id, &block.trace, initial_witness) - } - Opcode::Brillig(brillig) => BrilligSolver::solve(initial_witness, brillig), - }; - match resolution { - Ok(OpcodeResolution::Solved) => { - stalled = false; - } - Ok(OpcodeResolution::InProgress) => { - stalled = false; - unresolved_opcodes.push(opcode.clone()); - } - Ok(OpcodeResolution::InProgressBrillig(oracle_wait_info)) => { - stalled = false; - // InProgressBrillig Oracles must be externally re-solved - let brillig = match opcode { - Opcode::Brillig(brillig) => brillig.clone(), - _ => unreachable!("Brillig resolution for non brillig opcode"), - }; - unresolved_brillig_calls.push(UnresolvedBrilligCall { - brillig, - foreign_call_wait_info: oracle_wait_info, - }) - } - Ok(OpcodeResolution::Stalled(not_solvable)) => { - if opcode_not_solvable.is_none() { - // we keep track of the first unsolvable opcode - opcode_not_solvable = Some(not_solvable); +pub struct ACVM { + backend: B, +} + +impl ACVM { + pub fn new(backend: B) -> Self { + ACVM { backend } + } + + /// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. + pub fn solve( + &self, + initial_witness: &mut WitnessMap, + blocks: &mut Blocks, + mut opcode_to_solve: Vec, + ) -> Result { + let mut unresolved_opcodes: Vec = Vec::new(); + let mut unresolved_brillig_calls: Vec = Vec::new(); + while !opcode_to_solve.is_empty() { + unresolved_opcodes.clear(); + let mut stalled = true; + let mut opcode_not_solvable = None; + for opcode in &opcode_to_solve { + let resolution = match opcode { + Opcode::Arithmetic(expr) => ArithmeticSolver::solve(initial_witness, expr), + Opcode::BlackBoxFuncCall(bb_func) => { + blackbox::solve(&self.backend, initial_witness, bb_func) } - // We push those opcodes not solvable to the back as - // it could be because the opcodes are out of order, i.e. this assignment - // relies on a later opcodes' results - unresolved_opcodes.push(opcode.clone()); - } - Err(OpcodeResolutionError::OpcodeNotSolvable(_)) => { - unreachable!("ICE - Result should have been converted to GateResolution") + Opcode::Directive(directive) => solve_directives(initial_witness, directive), + Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { + blocks.solve(block.id, &block.trace, initial_witness) + } + Opcode::Brillig(brillig) => BrilligSolver::solve(initial_witness, brillig), + }; + match resolution { + Ok(OpcodeResolution::Solved) => { + stalled = false; + } + Ok(OpcodeResolution::InProgress) => { + stalled = false; + unresolved_opcodes.push(opcode.clone()); + } + Ok(OpcodeResolution::InProgressBrillig(oracle_wait_info)) => { + stalled = false; + // InProgressBrillig Oracles must be externally re-solved + let brillig = match opcode { + Opcode::Brillig(brillig) => brillig.clone(), + _ => unreachable!("Brillig resolution for non brillig opcode"), + }; + unresolved_brillig_calls.push(UnresolvedBrilligCall { + brillig, + foreign_call_wait_info: oracle_wait_info, + }) + } + Ok(OpcodeResolution::Stalled(not_solvable)) => { + if opcode_not_solvable.is_none() { + // we keep track of the first unsolvable opcode + opcode_not_solvable = Some(not_solvable); + } + // We push those opcodes not solvable to the back as + // it could be because the opcodes are out of order, i.e. this assignment + // relies on a later opcodes' results + unresolved_opcodes.push(opcode.clone()); + } + Err(OpcodeResolutionError::OpcodeNotSolvable(_)) => { + unreachable!("ICE - Result should have been converted to GateResolution") + } + Err(err) => return Err(err), } - Err(err) => return Err(err), } + // We have oracles that must be externally resolved + if !unresolved_brillig_calls.is_empty() { + return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall { + unsolved_opcodes: unresolved_opcodes, + unresolved_brillig_calls, + }); + } + // We are stalled because of an opcode being bad + if stalled && !unresolved_opcodes.is_empty() { + return Err(OpcodeResolutionError::OpcodeNotSolvable( + opcode_not_solvable + .expect("infallible: cannot be stalled and None at the same time"), + )); + } + std::mem::swap(&mut opcode_to_solve, &mut unresolved_opcodes); } - // We have foreign calls that must be externally resolved - if !unresolved_brillig_calls.is_empty() { - return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall { - unsolved_opcodes: unresolved_opcodes, - unresolved_brillig_calls, - }); - } - // We are stalled because of an opcode being bad - if stalled && !unresolved_opcodes.is_empty() { - return Err(OpcodeResolutionError::OpcodeNotSolvable( - opcode_not_solvable - .expect("infallible: cannot be stalled and None at the same time"), - )); - } - std::mem::swap(&mut opcode_to_solve, &mut unresolved_opcodes); + Ok(PartialWitnessGeneratorStatus::Solved) } - Ok(PartialWitnessGeneratorStatus::Solved) } // Returns the concrete value for a particular witness @@ -286,7 +296,7 @@ mod tests { }; use crate::{ - pwg::{self, block::Blocks, OpcodeResolution, PartialWitnessGeneratorStatus}, + pwg::{block::Blocks, OpcodeResolution, PartialWitnessGeneratorStatus, ACVM}, OpcodeResolutionError, PartialWitnessGenerator, }; @@ -401,19 +411,20 @@ mod tests { }), ]; - let backend = StubbedPwg; - let mut witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), ]) .into(); + + let acvm = ACVM::new(StubbedPwg); let mut blocks = Blocks::default(); // use the partial witness generation solver with our acir program - let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on brillig call"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require foreign call resolution") + let solver_status = acvm + .solve(&mut witness_assignments, &mut blocks, opcodes) + .expect("should stall on oracle"); + let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + panic!("Should require oracle data") }; assert_eq!(unsolved_opcodes.len(), 0, "brillig should have been removed"); @@ -433,9 +444,9 @@ mod tests { let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = - pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should not stall on brillig call"); + let solver_status = acvm + .solve(&mut witness_assignments, &mut blocks, next_opcodes_for_solving) + .expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -533,8 +544,6 @@ mod tests { }), ]; - let backend = StubbedPwg; - let mut witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), @@ -542,12 +551,15 @@ mod tests { (Witness(9), FieldElement::from(10u128)), ]) .into(); + + let acvm = ACVM::new(StubbedPwg); let mut blocks = Blocks::default(); // use the partial witness generation solver with our acir program - let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on brillig call"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require foreign call resolution") + let solver_status = acvm + .solve(&mut witness_assignments, &mut blocks, opcodes) + .expect("should stall on oracle"); + let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + panic!("Should require oracle data") }; assert_eq!(unsolved_opcodes.len(), 0, "brillig should have been removed"); @@ -568,11 +580,11 @@ mod tests { let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = - pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should stall on brillig call"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require foreign call resolution") + let solver_status = acvm + .solve(&mut witness_assignments, &mut blocks, next_opcodes_for_solving) + .expect("should stall on oracle"); + let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + panic!("Should require oracle data") }; assert!(unsolved_opcodes.is_empty(), "should be fully solved"); @@ -595,9 +607,9 @@ mod tests { next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = - pwg::solve(&backend, &mut witness_assignments, &mut blocks, next_opcodes_for_solving) - .expect("should not stall on brillig call"); + let solver_status = acvm + .solve(&mut witness_assignments, &mut blocks, next_opcodes_for_solving) + .expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -673,16 +685,17 @@ mod tests { }), ]; - let backend = StubbedPwg; - let mut witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), ]) .into(); + + let acvm = ACVM::new(StubbedPwg); let mut blocks = Blocks::default(); - let solver_status = pwg::solve(&backend, &mut witness_assignments, &mut blocks, opcodes) - .expect("should not stall on brillig call"); + let solver_status = acvm + .solve(&mut witness_assignments, &mut blocks, opcodes) + .expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } } From 90a84397d2872eda0068713584e234428666431a Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 16 Jun 2023 07:45:16 +0000 Subject: [PATCH 07/18] feat: track state of `Blocks` internally inside ACVM --- acvm/src/pwg/mod.rs | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 303d27b89..4cf0c3482 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -87,18 +87,18 @@ pub enum OpcodeResolutionError { pub struct ACVM { backend: B, + blocks: Blocks, } impl ACVM { pub fn new(backend: B) -> Self { - ACVM { backend } + ACVM { backend, blocks: Blocks::default() } } /// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. pub fn solve( - &self, + &mut self, initial_witness: &mut WitnessMap, - blocks: &mut Blocks, mut opcode_to_solve: Vec, ) -> Result { let mut unresolved_opcodes: Vec = Vec::new(); @@ -115,7 +115,7 @@ impl ACVM { } Opcode::Directive(directive) => solve_directives(initial_witness, directive), Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { - blocks.solve(block.id, &block.trace, initial_witness) + self.blocks.solve(block.id, &block.trace, initial_witness) } Opcode::Brillig(brillig) => BrilligSolver::solve(initial_witness, brillig), }; @@ -296,7 +296,7 @@ mod tests { }; use crate::{ - pwg::{block::Blocks, OpcodeResolution, PartialWitnessGeneratorStatus, ACVM}, + pwg::{OpcodeResolution, PartialWitnessGeneratorStatus, ACVM}, OpcodeResolutionError, PartialWitnessGenerator, }; @@ -417,12 +417,11 @@ mod tests { ]) .into(); - let acvm = ACVM::new(StubbedPwg); - let mut blocks = Blocks::default(); + let mut acvm = ACVM::new(StubbedPwg); + // use the partial witness generation solver with our acir program - let solver_status = acvm - .solve(&mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on oracle"); + let solver_status = + acvm.solve(&mut witness_assignments, opcodes).expect("should stall on oracle"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -445,7 +444,7 @@ mod tests { next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving let solver_status = acvm - .solve(&mut witness_assignments, &mut blocks, next_opcodes_for_solving) + .solve(&mut witness_assignments, next_opcodes_for_solving) .expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -552,12 +551,10 @@ mod tests { ]) .into(); - let acvm = ACVM::new(StubbedPwg); - let mut blocks = Blocks::default(); + let mut acvm = ACVM::new(StubbedPwg); // use the partial witness generation solver with our acir program - let solver_status = acvm - .solve(&mut witness_assignments, &mut blocks, opcodes) - .expect("should stall on oracle"); + let solver_status = + acvm.solve(&mut witness_assignments, opcodes).expect("should stall on oracle"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -581,7 +578,7 @@ mod tests { next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving let solver_status = acvm - .solve(&mut witness_assignments, &mut blocks, next_opcodes_for_solving) + .solve(&mut witness_assignments, next_opcodes_for_solving) .expect("should stall on oracle"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") @@ -608,7 +605,7 @@ mod tests { // After filling data request, continue solving let solver_status = acvm - .solve(&mut witness_assignments, &mut blocks, next_opcodes_for_solving) + .solve(&mut witness_assignments, next_opcodes_for_solving) .expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -691,11 +688,9 @@ mod tests { ]) .into(); - let acvm = ACVM::new(StubbedPwg); - let mut blocks = Blocks::default(); - let solver_status = acvm - .solve(&mut witness_assignments, &mut blocks, opcodes) - .expect("should not stall on oracle"); + let mut acvm = ACVM::new(StubbedPwg); + let solver_status = + acvm.solve(&mut witness_assignments, opcodes).expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } } From 9b3a4d81efd10f9adae1ab6efb2a20760e854a10 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 16 Jun 2023 07:56:51 +0000 Subject: [PATCH 08/18] feat: pass ownership over witness map to ACVM --- acvm/src/pwg/mod.rs | 59 ++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 4cf0c3482..c5350a414 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -88,17 +88,23 @@ pub enum OpcodeResolutionError { pub struct ACVM { backend: B, blocks: Blocks, + + witness_map: WitnessMap, } impl ACVM { - pub fn new(backend: B) -> Self { - ACVM { backend, blocks: Blocks::default() } + pub fn new(backend: B, initial_witness: WitnessMap) -> Self { + ACVM { backend, blocks: Blocks::default(), witness_map: initial_witness } + } + + /// Finalize the ACVM execution, returning the resulting [`WitnessMap`]. + pub fn finalize(self) -> WitnessMap { + self.witness_map } /// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. pub fn solve( &mut self, - initial_witness: &mut WitnessMap, mut opcode_to_solve: Vec, ) -> Result { let mut unresolved_opcodes: Vec = Vec::new(); @@ -109,13 +115,17 @@ impl ACVM { let mut opcode_not_solvable = None; for opcode in &opcode_to_solve { let resolution = match opcode { - Opcode::Arithmetic(expr) => ArithmeticSolver::solve(initial_witness, expr), + Opcode::Arithmetic(expr) => { + ArithmeticSolver::solve(&mut self.witness_map, expr) + } Opcode::BlackBoxFuncCall(bb_func) => { - blackbox::solve(&self.backend, initial_witness, bb_func) + blackbox::solve(&self.backend, &mut self.witness_map, bb_func) + } + Opcode::Directive(directive) => { + solve_directives(&mut self.witness_map, directive) } - Opcode::Directive(directive) => solve_directives(initial_witness, directive), Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { - self.blocks.solve(block.id, &block.trace, initial_witness) + self.blocks.solve(block.id, &block.trace, &mut self.witness_map) } Opcode::Brillig(brillig) => BrilligSolver::solve(initial_witness, brillig), }; @@ -411,17 +421,16 @@ mod tests { }), ]; - let mut witness_assignments = BTreeMap::from([ + let witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), ]) .into(); - let mut acvm = ACVM::new(StubbedPwg); + let mut acvm = ACVM::new(StubbedPwg, witness_assignments); // use the partial witness generation solver with our acir program - let solver_status = - acvm.solve(&mut witness_assignments, opcodes).expect("should stall on oracle"); + let solver_status = acvm.solve(opcodes).expect("should stall on oracle"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -443,9 +452,8 @@ mod tests { let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = acvm - .solve(&mut witness_assignments, next_opcodes_for_solving) - .expect("should not stall on oracle"); + let solver_status = + acvm.solve(next_opcodes_for_solving).expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -543,7 +551,7 @@ mod tests { }), ]; - let mut witness_assignments = BTreeMap::from([ + let witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), (Witness(8), FieldElement::from(5u128)), @@ -551,10 +559,9 @@ mod tests { ]) .into(); - let mut acvm = ACVM::new(StubbedPwg); + let mut acvm = ACVM::new(StubbedPwg, witness_assignments); // use the partial witness generation solver with our acir program - let solver_status = - acvm.solve(&mut witness_assignments, opcodes).expect("should stall on oracle"); + let solver_status = acvm.solve(opcodes).expect("should stall on oracle"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -577,9 +584,7 @@ mod tests { let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = acvm - .solve(&mut witness_assignments, next_opcodes_for_solving) - .expect("should stall on oracle"); + let solver_status = acvm.solve(next_opcodes_for_solving).expect("should stall on oracle"); let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { panic!("Should require oracle data") }; @@ -604,9 +609,8 @@ mod tests { next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = acvm - .solve(&mut witness_assignments, next_opcodes_for_solving) - .expect("should not stall on oracle"); + let solver_status = + acvm.solve(next_opcodes_for_solving).expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -682,15 +686,14 @@ mod tests { }), ]; - let mut witness_assignments = BTreeMap::from([ + let witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), ]) .into(); - let mut acvm = ACVM::new(StubbedPwg); - let solver_status = - acvm.solve(&mut witness_assignments, opcodes).expect("should not stall on oracle"); + let mut acvm = ACVM::new(StubbedPwg, witness_assignments); + let solver_status = acvm.solve(opcodes).expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } } From e19745d190e2fe20dd78db62928893d2c7d20783 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 16 Jun 2023 08:44:10 +0000 Subject: [PATCH 09/18] chore: maintain list of unsolved opcodes internally --- acvm/src/pwg/mod.rs | 78 +++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 41 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index c5350a414..e6f86a3b2 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -34,11 +34,8 @@ pub enum PartialWitnessGeneratorStatus { /// to retrieve information from outside of the ACVM. /// The result of the foreign call is inserted into the `Brillig` opcode which made the call using [`UnresolvedBrilligCall::resolve`]. /// - /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the new set of opcodes. - RequiresForeignCall { - unsolved_opcodes: Vec, - unresolved_brillig_calls: Vec, - }, + /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the remaining opcodes. + RequiresForeignCall { unresolved_brillig_calls: Vec }, } #[derive(Debug, PartialEq)] @@ -88,13 +85,14 @@ pub enum OpcodeResolutionError { pub struct ACVM { backend: B, blocks: Blocks, + opcodes: Vec, witness_map: WitnessMap, } impl ACVM { - pub fn new(backend: B, initial_witness: WitnessMap) -> Self { - ACVM { backend, blocks: Blocks::default(), witness_map: initial_witness } + pub fn new(backend: B, opcodes: Vec, initial_witness: WitnessMap) -> Self { + ACVM { backend, blocks: Blocks::default(), opcodes, witness_map: initial_witness } } /// Finalize the ACVM execution, returning the resulting [`WitnessMap`]. @@ -102,18 +100,19 @@ impl ACVM { self.witness_map } + pub fn resolve_brillig_foreign_call(&mut self, foreign_call: Brillig) { + self.opcodes.insert(0, Opcode::Brillig(foreign_call)); + } + /// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. - pub fn solve( - &mut self, - mut opcode_to_solve: Vec, - ) -> Result { + pub fn solve(&mut self) -> Result { let mut unresolved_opcodes: Vec = Vec::new(); let mut unresolved_brillig_calls: Vec = Vec::new(); - while !opcode_to_solve.is_empty() { + while !self.opcodes.is_empty() { unresolved_opcodes.clear(); let mut stalled = true; let mut opcode_not_solvable = None; - for opcode in &opcode_to_solve { + for opcode in &self.opcodes { let resolution = match opcode { Opcode::Arithmetic(expr) => { ArithmeticSolver::solve(&mut self.witness_map, expr) @@ -127,7 +126,9 @@ impl ACVM { Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { self.blocks.solve(block.id, &block.trace, &mut self.witness_map) } - Opcode::Brillig(brillig) => BrilligSolver::solve(initial_witness, brillig), + Opcode::Brillig(brillig) => { + BrilligSolver::solve(&mut self.witness_map, brillig) + } }; match resolution { Ok(OpcodeResolution::Solved) => { @@ -165,21 +166,22 @@ impl ACVM { Err(err) => return Err(err), } } + + std::mem::swap(&mut self.opcodes, &mut unresolved_opcodes); + // We have oracles that must be externally resolved if !unresolved_brillig_calls.is_empty() { return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall { - unsolved_opcodes: unresolved_opcodes, unresolved_brillig_calls, }); } // We are stalled because of an opcode being bad - if stalled && !unresolved_opcodes.is_empty() { + if stalled && !self.opcodes.is_empty() { return Err(OpcodeResolutionError::OpcodeNotSolvable( opcode_not_solvable .expect("infallible: cannot be stalled and None at the same time"), )); } - std::mem::swap(&mut opcode_to_solve, &mut unresolved_opcodes); } Ok(PartialWitnessGeneratorStatus::Solved) } @@ -427,15 +429,15 @@ mod tests { ]) .into(); - let mut acvm = ACVM::new(StubbedPwg, witness_assignments); + let mut acvm = ACVM::new(StubbedPwg, opcodes, witness_assignments); // use the partial witness generation solver with our acir program - let solver_status = acvm.solve(opcodes).expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + let solver_status = acvm.solve().expect("should stall on oracle"); + let PartialWitnessGeneratorStatus::RequiresForeignCall { mut unresolved_brillig_calls } = solver_status else { panic!("Should require oracle data") }; - assert_eq!(unsolved_opcodes.len(), 0, "brillig should have been removed"); + assert_eq!(acvm.opcodes.len(), 0, "brillig should have been removed"); assert_eq!(unresolved_brillig_calls.len(), 1, "should have a brillig oracle request"); let foreign_call = unresolved_brillig_calls.remove(0); @@ -449,11 +451,9 @@ mod tests { Value::from(foreign_call.foreign_call_wait_info.inputs[0][0].to_field().inverse()); // Alter Brillig oracle opcode with foreign call resolution let brillig: Brillig = foreign_call.resolve(foreign_call_result.into()); - let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; - next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); + acvm.resolve_brillig_foreign_call(brillig); // After filling data request, continue solving - let solver_status = - acvm.solve(next_opcodes_for_solving).expect("should not stall on oracle"); + let solver_status = acvm.solve().expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -559,14 +559,14 @@ mod tests { ]) .into(); - let mut acvm = ACVM::new(StubbedPwg, witness_assignments); + let mut acvm = ACVM::new(StubbedPwg, opcodes, witness_assignments); // use the partial witness generation solver with our acir program - let solver_status = acvm.solve(opcodes).expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + let solver_status = acvm.solve().expect("should stall on oracle"); + let PartialWitnessGeneratorStatus::RequiresForeignCall { mut unresolved_brillig_calls } = solver_status else { panic!("Should require oracle data") }; - assert_eq!(unsolved_opcodes.len(), 0, "brillig should have been removed"); + assert_eq!(acvm.opcodes.len(), 0, "brillig should have been removed"); assert_eq!(unresolved_brillig_calls.len(), 1, "should have a brillig oracle request"); let foreign_call = unresolved_brillig_calls.remove(0); @@ -580,16 +580,15 @@ mod tests { foreign_call.foreign_call_wait_info.inputs[0][0].to_field().inverse(); // Alter Brillig oracle opcode let brillig: Brillig = foreign_call.resolve(Value::from(x_plus_y_inverse).into()); + acvm.resolve_brillig_foreign_call(brillig); - let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; - next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); // After filling data request, continue solving - let solver_status = acvm.solve(next_opcodes_for_solving).expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { unsolved_opcodes, mut unresolved_brillig_calls, .. } = solver_status else { + let solver_status = acvm.solve().expect("should stall on oracle"); + let PartialWitnessGeneratorStatus::RequiresForeignCall { mut unresolved_brillig_calls } = solver_status else { panic!("Should require oracle data") }; - assert!(unsolved_opcodes.is_empty(), "should be fully solved"); + assert!(acvm.opcodes.is_empty(), "should be fully solved"); assert_eq!(unresolved_brillig_calls.len(), 1, "should have no unresolved oracles"); let foreign_call = unresolved_brillig_calls.remove(0); @@ -604,13 +603,10 @@ mod tests { assert_ne!(x_plus_y_inverse, i_plus_j_inverse); // Alter Brillig oracle opcode let brillig = foreign_call.resolve(Value::from(i_plus_j_inverse).into()); - - let mut next_opcodes_for_solving = vec![Opcode::Brillig(brillig)]; - next_opcodes_for_solving.extend_from_slice(&unsolved_opcodes[..]); + acvm.resolve_brillig_foreign_call(brillig); // After filling data request, continue solving - let solver_status = - acvm.solve(next_opcodes_for_solving).expect("should not stall on oracle"); + let solver_status = acvm.solve().expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } @@ -692,8 +688,8 @@ mod tests { ]) .into(); - let mut acvm = ACVM::new(StubbedPwg, witness_assignments); - let solver_status = acvm.solve(opcodes).expect("should not stall on oracle"); + let mut acvm = ACVM::new(StubbedPwg, opcodes, witness_assignments); + let solver_status = acvm.solve().expect("should not stall on oracle"); assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } } From 5a809ece35edec3be2f4b813e6e9d3f6a063ddf6 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 08:55:22 +0000 Subject: [PATCH 10/18] feat: track pending foreign calls inside ACVM struct --- acvm/src/pwg/mod.rs | 36 ++++++++++++++++++++------ acvm/tests/solver.rs | 60 +++++++++++++++++++++----------------------- 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 0efb87b69..abcd5d31b 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -36,7 +36,7 @@ pub enum PartialWitnessGeneratorStatus { /// The result of the foreign call is inserted into the `Brillig` opcode which made the call using [`UnresolvedBrilligCall::resolve`]. /// /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the remaining opcodes. - RequiresForeignCall { unresolved_brillig_calls: Vec }, + RequiresForeignCall, } #[derive(Debug, PartialEq)] @@ -86,14 +86,26 @@ pub enum OpcodeResolutionError { pub struct ACVM { backend: B, blocks: Blocks, + /// A list of opcodes which are to be executed by the ACVM. + /// + /// Note that this doesn't include any opcodes which are waiting on a pending foreign call. opcodes: Vec, witness_map: WitnessMap, + + /// A list of foreign calls which must be resolved before the ACVM can resume execution. + pending_foreign_calls: Vec, } impl ACVM { pub fn new(backend: B, opcodes: Vec, initial_witness: WitnessMap) -> Self { - ACVM { backend, blocks: Blocks::default(), opcodes, witness_map: initial_witness } + ACVM { + backend, + blocks: Blocks::default(), + opcodes, + witness_map: initial_witness, + pending_foreign_calls: Vec::new(), + } } /// Finalize the ACVM execution, returning the resulting [`WitnessMap`]. @@ -101,12 +113,24 @@ impl ACVM { self.witness_map } - pub fn resolve_brillig_foreign_call(&mut self, foreign_call: Brillig) { - self.opcodes.insert(0, Opcode::Brillig(foreign_call)); + /// Return a reference to the arguments for the next pending foreign call, if one exists. + pub fn get_pending_foreign_call(&self) -> Option<&ForeignCallWaitInfo> { + self.pending_foreign_calls.first().map(|foreign_call| &foreign_call.foreign_call_wait_info) + } + + /// Resolves a pending foreign call using a result calculated outside of the ACVM. + pub fn resolve_pending_foreign_call(&mut self, foreign_call_result: ForeignCallResult) { + // Remove the first foreign call and inject the result to create a new opcode. + let foreign_call = self.pending_foreign_calls.remove(0); + let resolved_brillig = foreign_call.resolve(foreign_call_result); + + // Mark this opcode to be executed next. + self.opcodes.insert(0, Opcode::Brillig(resolved_brillig)); } /// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. pub fn solve(&mut self) -> Result { + // TODO: Prevent execution with outstanding foreign calls? let mut unresolved_opcodes: Vec = Vec::new(); let mut unresolved_brillig_calls: Vec = Vec::new(); while !self.opcodes.is_empty() { @@ -172,9 +196,7 @@ impl ACVM { // We have oracles that must be externally resolved if !unresolved_brillig_calls.is_empty() { - return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall { - unresolved_brillig_calls, - }); + return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall); } // We are stalled because of an opcode being bad if stalled && !self.opcodes.is_empty() { diff --git a/acvm/tests/solver.rs b/acvm/tests/solver.rs index 886b1694f..b2b134790 100644 --- a/acvm/tests/solver.rs +++ b/acvm/tests/solver.rs @@ -15,7 +15,7 @@ use acir::{ use acvm::{ pwg::{ ForeignCallWaitInfo, OpcodeResolution, OpcodeResolutionError, - PartialWitnessGeneratorStatus, UnresolvedBrilligCall, ACVM, + PartialWitnessGeneratorStatus, ACVM, }, PartialWitnessGenerator, }; @@ -142,23 +142,22 @@ fn inversion_brillig_oracle_equivalence() { let mut acvm = ACVM::new(StubbedPwg, opcodes, witness_assignments); // use the partial witness generation solver with our acir program let solver_status = acvm.solve().expect("should stall on brillig call"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require oracle data") - }; + assert_eq!( + solver_status, + PartialWitnessGeneratorStatus::RequiresForeignCall, + "Should require oracle data" + ); assert_eq!(acvm.opcodes.len(), 0, "brillig should have been removed"); - assert_eq!(unresolved_brillig_calls.len(), 1, "should have a brillig foreign call request"); - let foreign_call = unresolved_brillig_calls.remove(0); - let foreign_call_wait_info: &ForeignCallWaitInfo = &foreign_call.foreign_call_wait_info; + let foreign_call_wait_info: &ForeignCallWaitInfo = + acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); assert_eq!(foreign_call_wait_info.inputs.len(), 1, "Should be waiting for a single input"); // As caller of VM, need to resolve foreign calls - let foreign_call_result = - Value::from(foreign_call.foreign_call_wait_info.inputs[0][0].to_field().inverse()); + let foreign_call_result = Value::from(foreign_call_wait_info.inputs[0][0].to_field().inverse()); // Alter Brillig oracle opcode with foreign call resolution - let brillig: Brillig = foreign_call.resolve(foreign_call_result.into()); - acvm.resolve_brillig_foreign_call(brillig); + acvm.resolve_pending_foreign_call(foreign_call_result.into()); // After filling data request, continue solving let solver_status = acvm.solve().expect("should not stall on brillig call"); @@ -273,41 +272,40 @@ fn double_inversion_brillig_oracle() { // use the partial witness generation solver with our acir program let solver_status = acvm.solve().expect("should stall on oracle"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require foreign call response") - }; - + assert_eq!( + solver_status, + PartialWitnessGeneratorStatus::RequiresForeignCall, + "Should require oracle data" + ); assert_eq!(acvm.opcodes.len(), 0, "brillig should have been removed"); - assert_eq!(unresolved_brillig_calls.len(), 1, "should have a brillig foreign call request"); - let foreign_call = unresolved_brillig_calls.remove(0); - let foreign_call_wait_info: &ForeignCallWaitInfo = &foreign_call.foreign_call_wait_info; + let foreign_call_wait_info: &ForeignCallWaitInfo = + acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); assert_eq!(foreign_call_wait_info.inputs.len(), 1, "Should be waiting for a single input"); - let x_plus_y_inverse = foreign_call.foreign_call_wait_info.inputs[0][0].to_field().inverse(); + let x_plus_y_inverse = Value::from(foreign_call_wait_info.inputs[0][0].to_field().inverse()); // Resolve Brillig foreign call - let brillig: Brillig = foreign_call.resolve(Value::from(x_plus_y_inverse).into()); - acvm.resolve_brillig_foreign_call(brillig); + acvm.resolve_pending_foreign_call(x_plus_y_inverse.into()); // After filling data request, continue solving let solver_status = acvm.solve().expect("should stall on brillig call"); - let PartialWitnessGeneratorStatus::RequiresForeignCall { mut unresolved_brillig_calls, .. } = solver_status else { - panic!("Should require foreign call data") - }; - + assert_eq!( + solver_status, + PartialWitnessGeneratorStatus::RequiresForeignCall, + "Should require oracle data" + ); assert!(acvm.opcodes.is_empty(), "should be fully solved"); - assert_eq!(unresolved_brillig_calls.len(), 1, "should have an unresolved foreign call"); - let foreign_call: UnresolvedBrilligCall = unresolved_brillig_calls.remove(0); - let foreign_call_wait_info: &ForeignCallWaitInfo = &foreign_call.foreign_call_wait_info; + let foreign_call_wait_info = + acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); assert_eq!(foreign_call_wait_info.inputs.len(), 1, "Should be waiting for a single input"); - let i_plus_j_inverse = foreign_call_wait_info.inputs[0][0].to_field().inverse(); + let i_plus_j_inverse = Value::from(foreign_call_wait_info.inputs[0][0].to_field().inverse()); assert_ne!(x_plus_y_inverse, i_plus_j_inverse); + // Alter Brillig oracle opcode - let brillig = foreign_call.resolve(Value::from(i_plus_j_inverse).into()); - acvm.resolve_brillig_foreign_call(brillig); + acvm.resolve_pending_foreign_call(i_plus_j_inverse.into()); // After filling data request, continue solving let solver_status = acvm.solve().expect("should not stall on brillig call"); From ae4ffaf5f188f39662b2dccb8cf64316c9e53e8f Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 09:02:04 +0000 Subject: [PATCH 11/18] chore: add temporary method to fix integration test --- acvm/src/pwg/mod.rs | 5 +++++ acvm/tests/solver.rs | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index abcd5d31b..7ac7c8f2a 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -108,6 +108,11 @@ impl ACVM { } } + // necessary to fix integration tests. Should replace with a better VM status model. + pub fn remaining_opcodes_len(&self) -> usize { + self.opcodes.len() + } + /// Finalize the ACVM execution, returning the resulting [`WitnessMap`]. pub fn finalize(self) -> WitnessMap { self.witness_map diff --git a/acvm/tests/solver.rs b/acvm/tests/solver.rs index b2b134790..2e192ba29 100644 --- a/acvm/tests/solver.rs +++ b/acvm/tests/solver.rs @@ -148,7 +148,7 @@ fn inversion_brillig_oracle_equivalence() { PartialWitnessGeneratorStatus::RequiresForeignCall, "Should require oracle data" ); - assert_eq!(acvm.opcodes.len(), 0, "brillig should have been removed"); + assert_eq!(acvm.remaining_opcodes_len(), 0, "brillig should have been removed"); let foreign_call_wait_info: &ForeignCallWaitInfo = acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); @@ -277,7 +277,7 @@ fn double_inversion_brillig_oracle() { PartialWitnessGeneratorStatus::RequiresForeignCall, "Should require oracle data" ); - assert_eq!(acvm.opcodes.len(), 0, "brillig should have been removed"); + assert_eq!(acvm.remaining_opcodes_len(), 0, "brillig should have been removed"); let foreign_call_wait_info: &ForeignCallWaitInfo = acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); @@ -295,7 +295,7 @@ fn double_inversion_brillig_oracle() { PartialWitnessGeneratorStatus::RequiresForeignCall, "Should require oracle data" ); - assert!(acvm.opcodes.is_empty(), "should be fully solved"); + assert_eq!(acvm.remaining_opcodes_len(), 0, "should be fully solved"); let foreign_call_wait_info = acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); From 2459da0547c6917216ff9fb076a1e37a1dd8189b Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 09:02:34 +0000 Subject: [PATCH 12/18] chore: clippy --- acvm/tests/solver.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/acvm/tests/solver.rs b/acvm/tests/solver.rs index 2e192ba29..94fc7a542 100644 --- a/acvm/tests/solver.rs +++ b/acvm/tests/solver.rs @@ -131,9 +131,7 @@ fn inversion_brillig_oracle_equivalence() { }), ]; - let backend = StubbedPwg; - - let mut witness_assignments = BTreeMap::from([ + let witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), ]) @@ -258,9 +256,7 @@ fn double_inversion_brillig_oracle() { }), ]; - let backend = StubbedPwg; - - let mut witness_assignments = BTreeMap::from([ + let witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), (Witness(8), FieldElement::from(5u128)), @@ -384,7 +380,6 @@ fn brillig_oracle_predicate() { }), ]; - let backend = StubbedPwg; let witness_assignments = BTreeMap::from([ (Witness(1), FieldElement::from(2u128)), (Witness(2), FieldElement::from(3u128)), From a05a87524b9401954a99c8c0c70c05c7aa377fbb Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 09:33:04 +0000 Subject: [PATCH 13/18] chore: update stale comments --- acvm/src/pwg/mod.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 7ac7c8f2a..bc8f8b881 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -31,11 +31,11 @@ pub enum PartialWitnessGeneratorStatus { /// All opcodes have been solved. Solved, - /// The `PartialWitnessGenerator` has encountered a request for a Brillig [foreign call][acir::brillig_vm::Opcode::ForeignCall] - /// to retrieve information from outside of the ACVM. - /// The result of the foreign call is inserted into the `Brillig` opcode which made the call using [`UnresolvedBrilligCall::resolve`]. + /// The `ACVM` has encountered a request for a Brillig [foreign call][acir::brillig_vm::Opcode::ForeignCall] + /// to retrieve information from outside of the ACVM. The result of the foreign call must be passed back + /// to the ACVM using [`ACVM::resolve_pending_foreign_call`]. /// - /// Once this is done, the `PartialWitnessGenerator` can be restarted to solve the remaining opcodes. + /// Once this is done, the `ACVM` can be restarted to solve the remaining opcodes. RequiresForeignCall, } @@ -133,7 +133,12 @@ impl ACVM { self.opcodes.insert(0, Opcode::Brillig(resolved_brillig)); } - /// Executes a [`Circuit`] against an [initial witness][`WitnessMap`] to calculate the solved partial witness. + /// Executes the ACVM's circuit until execution halts. + /// + /// Execution can halt due to three reasons: + /// 1. All opcodes have been executed successfully. + /// 2. The circuit has been found to be unsatisfiable. + /// 2. A Brillig [foreign call][`UnresolvedBrilligCall`] has been encountered and must be resolved. pub fn solve(&mut self) -> Result { // TODO: Prevent execution with outstanding foreign calls? let mut unresolved_opcodes: Vec = Vec::new(); From 3e4987f37b0794c69ed27056af846a2eaad13a0d Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 11:26:48 +0000 Subject: [PATCH 14/18] chore: fix handling of foreign calls --- acvm/src/pwg/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index bc8f8b881..ec7b0a851 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -142,7 +142,6 @@ impl ACVM { pub fn solve(&mut self) -> Result { // TODO: Prevent execution with outstanding foreign calls? let mut unresolved_opcodes: Vec = Vec::new(); - let mut unresolved_brillig_calls: Vec = Vec::new(); while !self.opcodes.is_empty() { unresolved_opcodes.clear(); let mut stalled = true; @@ -180,7 +179,7 @@ impl ACVM { Opcode::Brillig(brillig) => brillig.clone(), _ => unreachable!("Brillig resolution for non brillig opcode"), }; - unresolved_brillig_calls.push(UnresolvedBrilligCall { + self.pending_foreign_calls.push(UnresolvedBrilligCall { brillig, foreign_call_wait_info: oracle_wait_info, }) @@ -205,7 +204,7 @@ impl ACVM { std::mem::swap(&mut self.opcodes, &mut unresolved_opcodes); // We have oracles that must be externally resolved - if !unresolved_brillig_calls.is_empty() { + if self.get_pending_foreign_call().is_some() { return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall); } // We are stalled because of an opcode being bad From ac46f2688fde90bb2dd6ccbaaeed1b5bdf4d44e8 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 11:29:46 +0000 Subject: [PATCH 15/18] chore: add comment on `std::mem:swap` relocation --- acvm/src/pwg/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index ec7b0a851..63fb8061c 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -201,12 +201,14 @@ impl ACVM { } } + // Before potentially ending execution, we must save the list of opcodes which remain to be solved. std::mem::swap(&mut self.opcodes, &mut unresolved_opcodes); // We have oracles that must be externally resolved if self.get_pending_foreign_call().is_some() { return Ok(PartialWitnessGeneratorStatus::RequiresForeignCall); } + // We are stalled because of an opcode being bad if stalled && !self.opcodes.is_empty() { return Err(OpcodeResolutionError::OpcodeNotSolvable( From 8b9e6e370e5cc1196732e1450cdc8d3783c4b565 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Tue, 20 Jun 2023 12:04:37 +0000 Subject: [PATCH 16/18] chore: track `BlockSolver` state in `ACVM` directly --- acvm/src/pwg/block.rs | 32 ++++++-------------------------- acvm/src/pwg/mod.rs | 19 ++++++++++++------- 2 files changed, 18 insertions(+), 33 deletions(-) diff --git a/acvm/src/pwg/block.rs b/acvm/src/pwg/block.rs index 9785735ae..05ef57ef4 100644 --- a/acvm/src/pwg/block.rs +++ b/acvm/src/pwg/block.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use acir::{ - circuit::opcodes::{BlockId, MemOp}, + circuit::opcodes::MemOp, native_types::{Witness, WitnessMap}, FieldElement, }; @@ -12,29 +12,11 @@ use super::{ }; use super::{OpcodeNotSolvable, OpcodeResolution, OpcodeResolutionError}; -/// Maps a block to its emulated state -#[derive(Default)] -pub struct Blocks { - blocks: HashMap, -} - -impl Blocks { - pub fn solve( - &mut self, - id: BlockId, - trace: &[MemOp], - solved_witness: &mut WitnessMap, - ) -> Result { - let solver = self.blocks.entry(id).or_default(); - solver.solve(solved_witness, trace) - } -} - /// Maintains the state for solving Block opcode /// block_value is the value of the Block at the solved_operations step /// solved_operations is the number of solved elements in the block #[derive(Default)] -struct BlockSolver { +pub(super) struct BlockSolver { block_value: HashMap, solved_operations: usize, } @@ -123,15 +105,14 @@ impl BlockSolver { #[cfg(test)] mod tests { use acir::{ - circuit::opcodes::{BlockId, MemOp}, + circuit::opcodes::MemOp, native_types::{Expression, Witness, WitnessMap}, FieldElement, }; + use super::BlockSolver; use crate::pwg::insert_value; - use super::Blocks; - #[test] fn test_solver() { let mut index = FieldElement::zero(); @@ -157,7 +138,6 @@ mod tests { index: Expression::one(), value: Expression::from(Witness(4)), }); - let id = BlockId::default(); let mut initial_witness = WitnessMap::new(); let mut value = FieldElement::zero(); insert_value(&Witness(1), value, &mut initial_witness).unwrap(); @@ -165,8 +145,8 @@ mod tests { insert_value(&Witness(2), value, &mut initial_witness).unwrap(); value = value + value; insert_value(&Witness(3), value, &mut initial_witness).unwrap(); - let mut blocks = Blocks::default(); - blocks.solve(id, &trace, &mut initial_witness).unwrap(); + let mut block_solver = BlockSolver::default(); + block_solver.solve(&mut initial_witness, &trace).unwrap(); assert_eq!(initial_witness[&Witness(4)], FieldElement::one()); } } diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 63fb8061c..4c3bd170f 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -1,14 +1,19 @@ // Re-usable methods that backends can use to implement their PWG +use std::collections::HashMap; + use crate::{Language, PartialWitnessGenerator}; use acir::{ brillig_vm::ForeignCallResult, - circuit::{brillig::Brillig, Opcode}, + circuit::{brillig::Brillig, opcodes::BlockId, Opcode}, native_types::{Expression, Witness, WitnessMap}, BlackBoxFunc, FieldElement, }; -use self::{arithmetic::ArithmeticSolver, brillig::BrilligSolver, directives::solve_directives}; +use self::{ + arithmetic::ArithmeticSolver, block::BlockSolver, brillig::BrilligSolver, + directives::solve_directives, +}; use thiserror::Error; @@ -22,8 +27,6 @@ mod directives; mod blackbox; mod block; -// Re-export `Blocks` so that it can be passed to `pwg::solve` -pub use block::Blocks; pub use brillig::ForeignCallWaitInfo; #[derive(Debug, PartialEq)] @@ -85,7 +88,8 @@ pub enum OpcodeResolutionError { pub struct ACVM { backend: B, - blocks: Blocks, + /// Stores the solver for each [block][`Opcode::Block`] opcode. This persists their internal state to prevent recomputation. + block_solvers: HashMap, /// A list of opcodes which are to be executed by the ACVM. /// /// Note that this doesn't include any opcodes which are waiting on a pending foreign call. @@ -101,7 +105,7 @@ impl ACVM { pub fn new(backend: B, opcodes: Vec, initial_witness: WitnessMap) -> Self { ACVM { backend, - blocks: Blocks::default(), + block_solvers: HashMap::default(), opcodes, witness_map: initial_witness, pending_foreign_calls: Vec::new(), @@ -158,7 +162,8 @@ impl ACVM { solve_directives(&mut self.witness_map, directive) } Opcode::Block(block) | Opcode::ROM(block) | Opcode::RAM(block) => { - self.blocks.solve(block.id, &block.trace, &mut self.witness_map) + let solver = self.block_solvers.entry(block.id).or_default(); + solver.solve(&mut self.witness_map, &block.trace) } Opcode::Brillig(brillig) => { BrilligSolver::solve(&mut self.witness_map, brillig) From dbc4945a2f59f64a10a68f1d507d5732f342e3d8 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 21 Jun 2023 05:47:56 +0000 Subject: [PATCH 17/18] chore: add method to get slice of unresolved opcodes --- acvm/src/pwg/mod.rs | 20 ++++++++++++++++---- acvm/tests/solver.rs | 6 +++--- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/acvm/src/pwg/mod.rs b/acvm/src/pwg/mod.rs index 1ea17361e..7fd34c590 100644 --- a/acvm/src/pwg/mod.rs +++ b/acvm/src/pwg/mod.rs @@ -38,7 +38,7 @@ pub enum PartialWitnessGeneratorStatus { /// to retrieve information from outside of the ACVM. The result of the foreign call must be passed back /// to the ACVM using [`ACVM::resolve_pending_foreign_call`]. /// - /// Once this is done, the `ACVM` can be restarted to solve the remaining opcodes. + /// Once this is done, the ACVM can be restarted to solve the remaining opcodes. RequiresForeignCall, } @@ -112,13 +112,25 @@ impl ACVM { } } - // necessary to fix integration tests. Should replace with a better VM status model. - pub fn remaining_opcodes_len(&self) -> usize { - self.opcodes.len() + /// Returns a reference to the current state of the ACVM's [`WitnessMap`]. + /// + /// Once execution has completed, the witness map can be extracted using [`ACVM::finalize`] + pub fn witness_map(&self) -> &WitnessMap { + &self.witness_map + } + + /// Returns a slice containing the opcodes which remain to be solved. + /// + /// Note: this doesn't include any opcodes which are waiting on a pending foreign call. + pub fn unresolved_opcodes(&self) -> &[Opcode] { + &self.opcodes } /// Finalize the ACVM execution, returning the resulting [`WitnessMap`]. pub fn finalize(self) -> WitnessMap { + if self.opcodes.is_empty() || self.get_pending_foreign_call().is_some() { + panic!("ACVM is not ready to be finalized"); + } self.witness_map } diff --git a/acvm/tests/solver.rs b/acvm/tests/solver.rs index dbd982e44..021d4c6ea 100644 --- a/acvm/tests/solver.rs +++ b/acvm/tests/solver.rs @@ -147,7 +147,7 @@ fn inversion_brillig_oracle_equivalence() { PartialWitnessGeneratorStatus::RequiresForeignCall, "Should require oracle data" ); - assert_eq!(acvm.remaining_opcodes_len(), 0, "brillig should have been removed"); + assert!(acvm.unresolved_opcodes().is_empty(), "brillig should have been removed"); let foreign_call_wait_info: &ForeignCallWaitInfo = acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); @@ -274,7 +274,7 @@ fn double_inversion_brillig_oracle() { PartialWitnessGeneratorStatus::RequiresForeignCall, "Should require oracle data" ); - assert_eq!(acvm.remaining_opcodes_len(), 0, "brillig should have been removed"); + assert!(acvm.unresolved_opcodes().is_empty(), "brillig should have been removed"); let foreign_call_wait_info: &ForeignCallWaitInfo = acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); @@ -292,7 +292,7 @@ fn double_inversion_brillig_oracle() { PartialWitnessGeneratorStatus::RequiresForeignCall, "Should require oracle data" ); - assert_eq!(acvm.remaining_opcodes_len(), 0, "should be fully solved"); + assert!(acvm.unresolved_opcodes().is_empty(), "should be fully solved"); let foreign_call_wait_info = acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); From ee47276db2937e588d23c609aa4cab5221e20f68 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 21 Jun 2023 06:26:36 +0000 Subject: [PATCH 18/18] chore: add a test for ACVM tracking opcodes across foreign calls correctly --- acvm/tests/solver.rs | 129 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/acvm/tests/solver.rs b/acvm/tests/solver.rs index 021d4c6ea..cf7469fb6 100644 --- a/acvm/tests/solver.rs +++ b/acvm/tests/solver.rs @@ -309,6 +309,135 @@ fn double_inversion_brillig_oracle() { assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); } +#[test] +fn oracle_dependent_execution() { + // This test ensures that we properly track the list of opcodes which still need to be resolved + // across any brillig foreign calls we may have to perform. + // + // Opcodes below describe the following: + // fn main(x : Field, y : pub Field) { + // assert(x == y); + // let x_inv = Oracle("inverse", x); + // let y_inv = Oracle("inverse", y); + // + // assert(x_inv == y_inv); + // } + // Also performs an unrelated equality check + // just for the sake of testing multiple brillig opcodes. + let fe_0 = FieldElement::zero(); + let fe_1 = FieldElement::one(); + let w_x = Witness(1); + let w_y = Witness(2); + let w_x_inv = Witness(3); + let w_y_inv = Witness(4); + + let brillig_data = Brillig { + inputs: vec![ + BrilligInputs::Single(w_x.into()), // Input Register 0 + BrilligInputs::Single(Expression::default()), // Input Register 1 + BrilligInputs::Single(w_y.into()), // Input Register 2, + ], + outputs: vec![ + BrilligOutputs::Simple(w_x), // Output Register 0 - from input + BrilligOutputs::Simple(w_y_inv), // Output Register 1 + BrilligOutputs::Simple(w_y), // Output Register 2 - from input + BrilligOutputs::Simple(w_y_inv), // Output Register 3 + ], + // stack of foreign call/oracle resolutions, starts empty + foreign_call_results: vec![], + bytecode: vec![ + // Oracles are named 'foreign calls' in brillig + brillig_vm::Opcode::ForeignCall { + function: "invert".into(), + destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(1))], + inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(0))], + }, + brillig_vm::Opcode::ForeignCall { + function: "invert".into(), + destinations: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(3))], + inputs: vec![RegisterOrMemory::RegisterIndex(RegisterIndex::from(2))], + }, + ], + predicate: None, + }; + + // This equality check can be executed immediately before resolving any foreign calls. + let equality_check = Expression { + mul_terms: vec![], + linear_combinations: vec![(-fe_1, w_x), (fe_1, w_y)], + q_c: fe_0, + }; + + // This equality check relies on the outputs of the Brillig call. + // It then cannot be solved until the foreign calls are resolved. + let inverse_equality_check = Expression { + mul_terms: vec![], + linear_combinations: vec![(-fe_1, w_x_inv), (fe_1, w_y_inv)], + q_c: fe_0, + }; + + let opcodes = vec![ + Opcode::Arithmetic(equality_check), + Opcode::Brillig(brillig_data), + Opcode::Arithmetic(inverse_equality_check.clone()), + ]; + + let witness_assignments = + BTreeMap::from([(w_x, FieldElement::from(2u128)), (w_y, FieldElement::from(2u128))]).into(); + + let mut acvm = ACVM::new(StubbedPwg, opcodes, witness_assignments); + + // use the partial witness generation solver with our acir program + let solver_status = acvm.solve().expect("should stall on oracle"); + assert_eq!( + solver_status, + PartialWitnessGeneratorStatus::RequiresForeignCall, + "Should require oracle data" + ); + assert_eq!(acvm.unresolved_opcodes().len(), 1, "brillig should have been removed"); + assert_eq!( + acvm.unresolved_opcodes()[0], + Opcode::Arithmetic(inverse_equality_check.clone()), + "Equality check of inverses should still be waiting to be resolved" + ); + + let foreign_call_wait_info: &ForeignCallWaitInfo = + acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); + assert_eq!(foreign_call_wait_info.inputs.len(), 1, "Should be waiting for a single input"); + + // Resolve Brillig foreign call + let x_inverse = Value::from(foreign_call_wait_info.inputs[0][0].to_field().inverse()); + acvm.resolve_pending_foreign_call(x_inverse.into()); + + // After filling data request, continue solving + let solver_status = acvm.solve().expect("should stall on oracle"); + assert_eq!( + solver_status, + PartialWitnessGeneratorStatus::RequiresForeignCall, + "Should require oracle data" + ); + assert_eq!(acvm.unresolved_opcodes().len(), 1, "brillig should have been removed"); + assert_eq!( + acvm.unresolved_opcodes()[0], + Opcode::Arithmetic(inverse_equality_check), + "Equality check of inverses should still be waiting to be resolved" + ); + + let foreign_call_wait_info: &ForeignCallWaitInfo = + acvm.get_pending_foreign_call().expect("should have a brillig foreign call request"); + assert_eq!(foreign_call_wait_info.inputs.len(), 1, "Should be waiting for a single input"); + + // Resolve Brillig foreign call + let y_inverse = Value::from(foreign_call_wait_info.inputs[0][0].to_field().inverse()); + acvm.resolve_pending_foreign_call(y_inverse.into()); + + // We've resolved all the brillig foreign calls so we should be able to complete execution now. + + // After filling data request, continue solving + let solver_status = acvm.solve().expect("should not stall on brillig call"); + assert_eq!(solver_status, PartialWitnessGeneratorStatus::Solved, "should be fully solved"); +} + #[test] fn brillig_oracle_predicate() { // Opcodes below describe the following: