feat!: Provide runtime callstacks for brillig failures and return err…

…ors in acvm_js (#523)

acvm/src/pwg/brillig.rs

@@ -1,6 +1,9 @@
 use acir::{
     brillig::{RegisterIndex, Value},
-    circuit::brillig::{Brillig, BrilligInputs, BrilligOutputs},
+    circuit::{
+        brillig::{Brillig, BrilligInputs, BrilligOutputs},
+        OpcodeLocation,
+    },
     native_types::WitnessMap,
     FieldElement,
 };
@@ -18,6 +21,7 @@ impl BrilligSolver {
         initial_witness: &mut WitnessMap,
         brillig: &Brillig,
         bb_solver: &B,
+        acir_index: usize,
     ) -> Result<Option<ForeignCallWaitInfo>, OpcodeResolutionError> {
         // If the predicate is `None`, then we simply return the value 1
         // If the predicate is `Some` but we cannot find a value, then we return stalled
@@ -107,8 +111,17 @@ impl BrilligSolver {
                 Ok(None)
             }
             VMStatus::InProgress => unreachable!("Brillig VM has not completed execution"),
-            VMStatus::Failure { message } => {
-                Err(OpcodeResolutionError::BrilligFunctionFailed(message, vm.program_counter()))
+            VMStatus::Failure { message, call_stack } => {
+                Err(OpcodeResolutionError::BrilligFunctionFailed {
+                    message,
+                    call_stack: call_stack
+                        .iter()
+                        .map(|brillig_index| OpcodeLocation::Brillig {
+                            acir_index,
+                            brillig_index: *brillig_index,
+                        })
+                        .collect(),
+                })
             }
             VMStatus::ForeignCallWait { function, inputs } => {
                 Ok(Some(ForeignCallWaitInfo { function, inputs }))

acvm/src/pwg/mod.rs

@@ -99,18 +99,18 @@ impl std::fmt::Display for ErrorLocation {
 
 #[derive(Clone, PartialEq, Eq, Debug, Error)]
 pub enum OpcodeResolutionError {
-    #[error("cannot solve opcode: {0}")]
+    #[error("Cannot solve opcode: {0}")]
     OpcodeNotSolvable(#[from] OpcodeNotSolvable),
-    #[error("backend does not currently support the {0} opcode. ACVM does not currently have a fallback for this opcode.")]
+    #[error("Backend does not currently support the {0} opcode. ACVM does not currently have a fallback for this opcode.")]
     UnsupportedBlackBoxFunc(BlackBoxFunc),
-    #[error("Cannot satisfy constraint {opcode_location}")]
+    #[error("Cannot satisfy constraint")]
     UnsatisfiedConstrain { opcode_location: ErrorLocation },
-    #[error("Index out of bounds, array has size {array_size:?}, but index was {index:?} at {opcode_location}")]
+    #[error("Index out of bounds, array has size {array_size:?}, but index was {index:?}")]
     IndexOutOfBounds { opcode_location: ErrorLocation, index: u32, array_size: u32 },
-    #[error("failed to solve blackbox function: {0}, reason: {1}")]
+    #[error("Failed to solve blackbox function: {0}, reason: {1}")]
     BlackBoxFunctionFailed(BlackBoxFunc, String),
-    #[error("failed to solve brillig function, reason: {0} at index {1}")]
-    BrilligFunctionFailed(String, usize),
+    #[error("Failed to solve brillig function, reason: {message}")]
+    BrilligFunctionFailed { message: String, call_stack: Vec<OpcodeLocation> },
 }
 
 impl From<BlackBoxResolutionError> for OpcodeResolutionError {
@@ -258,7 +258,12 @@ impl<'backend, B: BlackBoxFunctionSolver> ACVM<'backend, B> {
                 solver.solve_memory_op(op, &mut self.witness_map, predicate)
             }
             Opcode::Brillig(brillig) => {
-                match BrilligSolver::solve(&mut self.witness_map, brillig, self.backend) {
+                match BrilligSolver::solve(
+                    &mut self.witness_map,
+                    brillig,
+                    self.backend,
+                    self.instruction_pointer,
+                ) {
                     Ok(Some(foreign_call)) => return self.wait_for_foreign_call(foreign_call),
                     res => res.map(|_| ()),
                 }
@@ -289,20 +294,6 @@ impl<'backend, B: BlackBoxFunctionSolver> ACVM<'backend, B> {
                             self.instruction_pointer(),
                         ));
                     }
-                    // If a brillig function has failed, we return an unsatisfied constraint error
-                    // We intentionally ignore the brillig failure message, as there is no way to
-                    // propagate this to the caller.
-                    OpcodeResolutionError::BrilligFunctionFailed(
-                        _,
-                        brillig_instruction_pointer,
-                    ) => {
-                        error = OpcodeResolutionError::UnsatisfiedConstrain {
-                            opcode_location: ErrorLocation::Resolved(OpcodeLocation::Brillig {
-                                acir_index: self.instruction_pointer(),
-                                brillig_index: *brillig_instruction_pointer,
-                            }),
-                        }
-                    }
                     // All other errors are thrown normally.
                     _ => (),
                 };

acvm/tests/solver.rs

@@ -598,11 +598,9 @@ fn unsatisfied_opcode_resolved_brillig() {
     let solver_status = acvm.solve();
     assert_eq!(
         solver_status,
-        ACVMStatus::Failure(OpcodeResolutionError::UnsatisfiedConstrain {
-            opcode_location: ErrorLocation::Resolved(OpcodeLocation::Brillig {
-                acir_index: 0,
-                brillig_index: 2
-            }),
+        ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed {
+            message: "explicit trap hit in brillig".to_string(),
+            call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }]
         }),
         "The first opcode is not satisfiable, expected an error indicating this"
     );

acvm_js/src/execute.rs

@@ -5,11 +5,12 @@ use acvm::{
     pwg::{ACVMStatus, ErrorLocation, OpcodeResolutionError, ACVM},
 };
 
+use js_sys::Error;
 use wasm_bindgen::prelude::wasm_bindgen;
 
 use crate::{
     foreign_call::{resolve_brillig, ForeignCallHandler},
-    JsWitnessMap,
+    JsExecutionError, JsWitnessMap,
 };
 
 #[wasm_bindgen]
@@ -39,7 +40,7 @@ pub async fn execute_circuit(
     circuit: Vec<u8>,
     initial_witness: JsWitnessMap,
     foreign_call_handler: ForeignCallHandler,
-) -> Result<JsWitnessMap, js_sys::JsString> {
+) -> Result<JsWitnessMap, Error> {
     console_error_panic_hook::set_once();
 
     let solver = WasmBlackBoxFunctionSolver::initialize().await;
@@ -61,7 +62,7 @@ pub async fn execute_circuit_with_black_box_solver(
     circuit: Vec<u8>,
     initial_witness: JsWitnessMap,
     foreign_call_handler: ForeignCallHandler,
-) -> Result<JsWitnessMap, js_sys::JsString> {
+) -> Result<JsWitnessMap, Error> {
     console_error_panic_hook::set_once();
     let circuit: Circuit = Circuit::read(&*circuit).expect("Failed to deserialize circuit");
 
@@ -76,23 +77,34 @@ pub async fn execute_circuit_with_black_box_solver(
                 unreachable!("Execution should not stop while in `InProgress` state.")
             }
             ACVMStatus::Failure(error) => {
-                let assert_message = match &error {
+                let (assert_message, call_stack) = match &error {
                     OpcodeResolutionError::UnsatisfiedConstrain {
                         opcode_location: ErrorLocation::Resolved(opcode_location),
                     }
                     | OpcodeResolutionError::IndexOutOfBounds {
                         opcode_location: ErrorLocation::Resolved(opcode_location),
                         ..
-                    } => get_assert_message(&circuit.assert_messages, opcode_location),
-                    _ => None,
+                    } => (
+                        get_assert_message(&circuit.assert_messages, opcode_location),
+                        Some(vec![*opcode_location]),
+                    ),
+                    OpcodeResolutionError::BrilligFunctionFailed { call_stack, .. } => {
+                        let failing_opcode =
+                            call_stack.last().expect("Brillig error call stacks cannot be empty");
+                        (
+                            get_assert_message(&circuit.assert_messages, failing_opcode),
+                            Some(call_stack.clone()),
+                        )
+                    }
+                    _ => (None, None),
                 };
 
-                let error_string = match assert_message {
-                    Some(assert_message) => format!("{}: {}", error, assert_message),
+                let error_string = match &assert_message {
+                    Some(assert_message) => format!("Assertion failed: {}", assert_message),
                     None => error.to_string(),
                 };
 
-                return Err(error_string.into());
+                return Err(JsExecutionError::new(error_string.into(), call_stack).into());
             }
             ACVMStatus::RequiresForeignCall(foreign_call) => {
                 let result = resolve_brillig(&foreign_call_handler, &foreign_call).await?;

acvm_js/src/foreign_call/mod.rs

@@ -1,6 +1,6 @@
 use acvm::{brillig_vm::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo};
 
-use js_sys::JsString;
+use js_sys::{Error, JsString};
 use wasm_bindgen::{prelude::wasm_bindgen, JsValue};
 
 mod inputs;
@@ -30,7 +30,7 @@ extern "C" {
 pub(super) async fn resolve_brillig(
     foreign_call_callback: &ForeignCallHandler,
     foreign_call_wait_info: &ForeignCallWaitInfo,
-) -> Result<ForeignCallResult, String> {
+) -> Result<ForeignCallResult, Error> {
     // Prepare to call
     let name = JsString::from(foreign_call_wait_info.function.clone());
     let inputs = inputs::encode_foreign_call_inputs(&foreign_call_wait_info.inputs);
@@ -40,38 +40,35 @@ pub(super) async fn resolve_brillig(
 
     // The Brillig VM checks that the number of return values from
     // the foreign call is valid so we don't need to do it here.
-    outputs::decode_foreign_call_result(outputs)
+    outputs::decode_foreign_call_result(outputs).map_err(|message| Error::new(&message))
 }
 
-#[allow(dead_code)]
 async fn perform_foreign_call(
     foreign_call_handler: &ForeignCallHandler,
     name: JsString,
     inputs: js_sys::Array,
-) -> Result<js_sys::Array, String> {
+) -> Result<js_sys::Array, Error> {
     // Call and await
     let this = JsValue::null();
     let ret_js_val = foreign_call_handler
         .call2(&this, &name, &inputs)
-        .map_err(|err| format!("Error calling `foreign_call_callback`: {}", format_js_err(err)))?;
+        .map_err(|err| wrap_js_error("Error calling `foreign_call_callback`", &err))?;
     let ret_js_prom: js_sys::Promise = ret_js_val.into();
     let ret_future: wasm_bindgen_futures::JsFuture = ret_js_prom.into();
     let js_resolution = ret_future
         .await
-        .map_err(|err| format!("Error awaiting `foreign_call_handler`: {}", format_js_err(err)))?;
+        .map_err(|err| wrap_js_error("Error awaiting `foreign_call_handler`", &err))?;
 
     // Check that result conforms to expected shape.
     if !js_resolution.is_array() {
-        return Err("`foreign_call_handler` must return a Promise<ForeignCallValue[]>".into());
+        return Err(Error::new("Expected `foreign_call_handler` to return an array"));
     }
 
     Ok(js_sys::Array::from(&js_resolution))
 }
 
-#[allow(dead_code)]
-fn format_js_err(err: JsValue) -> String {
-    match err.as_string() {
-        Some(str) => str,
-        None => "Unknown".to_owned(),
-    }
+fn wrap_js_error(message: &str, err: &JsValue) -> Error {
+    let new_error = Error::new(message);
+    new_error.set_cause(err);
+    new_error
 }

acvm_js/src/js_execution_error.rs

@@ -0,0 +1,52 @@
+use acvm::acir::circuit::OpcodeLocation;
+use js_sys::{Array, Error, JsString, Reflect};
+use wasm_bindgen::prelude::{wasm_bindgen, JsValue};
+
+#[wasm_bindgen(typescript_custom_section)]
+const EXECUTION_ERROR: &'static str = r#"
+export type ExecutionError = Error & {
+    callStack?: string[];
+};
+"#;
+
+/// JsExecutionError is a raw js error.
+/// It'd be ideal that execution error was a subclass of Error, but for that we'd need to use JS snippets or a js module.
+/// Currently JS snippets don't work with a nodejs target. And a module would be too much for just a custom error type.
+#[wasm_bindgen]
+extern "C" {
+    #[wasm_bindgen(extends = Error, js_name = "ExecutionError", typescript_type = "ExecutionError")]
+    #[derive(Clone, Debug, PartialEq, Eq)]
+    pub type JsExecutionError;
+
+    #[wasm_bindgen(constructor, js_class = "Error")]
+    fn constructor(message: JsString) -> JsExecutionError;
+}
+
+impl JsExecutionError {
+    /// Creates a new execution error with the given call stack.
+    /// Call stacks won't be optional in the future, after removing ErrorLocation in ACVM.
+    pub fn new(message: String, call_stack: Option<Vec<OpcodeLocation>>) -> Self {
+        let mut error = JsExecutionError::constructor(JsString::from(message));
+        let js_call_stack = match call_stack {
+            Some(call_stack) => {
+                let js_array = Array::new();
+                for loc in call_stack {
+                    js_array.push(&JsValue::from(format!("{}", loc)));
+                }
+                js_array.into()
+            }
+            None => JsValue::UNDEFINED,
+        };
+
+        error.set_property("callStack", js_call_stack);
+
+        error
+    }
+
+    fn set_property(&mut self, property: &str, value: JsValue) {
+        assert!(
+            Reflect::set(self, &JsValue::from(property), &value).expect("Errors should be objects"),
+            "Errors should be writable"
+        );
+    }
+}

acvm_js/src/lib.rs

@@ -14,13 +14,14 @@ cfg_if::cfg_if! {
         mod js_witness_map;
         mod logging;
         mod public_witness;
+        mod js_execution_error;
 
         pub use build_info::build_info;
         pub use compression::{compress_witness, decompress_witness};
         pub use execute::{execute_circuit, execute_circuit_with_black_box_solver, create_black_box_solver};
         pub use js_witness_map::JsWitnessMap;
         pub use logging::{init_log_level, LogLevel};
         pub use public_witness::{get_public_parameters_witness, get_public_witness, get_return_witness};
-
+        pub use js_execution_error::JsExecutionError;
     }
 }

brillig_vm/src/lib.rs

@@ -30,12 +30,16 @@ pub use memory::Memory;
 use num_bigint::BigUint;
 pub use registers::Registers;
 
+/// The error call stack contains the opcode indexes of the call stack at the time of failure, plus the index of the opcode that failed.
+pub type ErrorCallStack = Vec<usize>;
+
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum VMStatus {
     Finished,
     InProgress,
     Failure {
         message: String,
+        call_stack: ErrorCallStack,
     },
     /// The VM process is not solvable as a [foreign call][Opcode::ForeignCall] has been
     /// reached where the outputs are yet to be resolved.  
@@ -121,7 +125,10 @@ impl<'bb_solver, B: BlackBoxFunctionSolver> VM<'bb_solver, B> {
     /// Indicating that the VM encountered a `Trap` Opcode
     /// or an invalid state.
     fn fail(&mut self, message: String) -> VMStatus {
-        self.status(VMStatus::Failure { message });
+        let mut error_stack: Vec<_> =
+            self.call_stack.iter().map(|value| value.to_usize()).collect();
+        error_stack.push(self.program_counter);
+        self.status(VMStatus::Failure { call_stack: error_stack, message });
         self.status.clone()
     }
 
@@ -174,7 +181,7 @@ impl<'bb_solver, B: BlackBoxFunctionSolver> VM<'bb_solver, B> {
             }
             Opcode::Return => {
                 if let Some(register) = self.call_stack.pop() {
-                    self.set_program_counter(register.to_usize())
+                    self.set_program_counter(register.to_usize() + 1)
                 } else {
                     self.fail("return opcode hit, but callstack already empty".to_string())
                 }
@@ -278,7 +285,7 @@ impl<'bb_solver, B: BlackBoxFunctionSolver> VM<'bb_solver, B> {
             }
             Opcode::Call { location } => {
                 // Push a return location
-                self.call_stack.push(Value::from(self.program_counter + 1));
+                self.call_stack.push(Value::from(self.program_counter));
                 self.set_program_counter(*location)
             }
             Opcode::Const { destination, value } => {
@@ -539,7 +546,10 @@ mod tests {
         let status = vm.process_opcode();
         assert_eq!(
             status,
-            VMStatus::Failure { message: "explicit trap hit in brillig".to_string() }
+            VMStatus::Failure {
+                message: "explicit trap hit in brillig".to_string(),
+                call_stack: vec![1]
+            }
         );
 
         // The register at index `2` should have not changed as we jumped over the add opcode