fix(debugger): improve output when constrain fails

acvm-repo/acvm/src/pwg/brillig.rs

@@ -162,62 +162,18 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
             VMStatus::Finished { .. } => Ok(BrilligSolverStatus::Finished),
             VMStatus::InProgress => Ok(BrilligSolverStatus::InProgress),
             VMStatus::Failure { reason, call_stack } => {
-                let payload = match reason {
-                    FailureReason::RuntimeError { message } => {
-                        Some(ResolvedAssertionPayload::String(message))
-                    }
-                    FailureReason::Trap { revert_data_offset, revert_data_size } => {
-                        // Since noir can only revert with strings currently, we can parse return data as a string
-                        if revert_data_size == 0 {
-                            None
-                        } else {
-                            let memory = self.vm.get_memory();
-                            let mut revert_values_iter = memory
-                                [revert_data_offset..(revert_data_offset + revert_data_size)]
-                                .iter();
-                            let error_selector = ErrorSelector::new(
-                                revert_values_iter
-                                    .next()
-                                    .expect("Incorrect revert data size")
-                                    .try_into()
-                                    .expect("Error selector is not u64"),
-                            );
+                let call_stack = call_stack
+                    .iter()
+                    .map(|brillig_index| OpcodeLocation::Brillig {
+                        acir_index: self.acir_index,
+                        brillig_index: *brillig_index,
+                    })
+                    .collect();
 
-                            match error_selector {
-                                STRING_ERROR_SELECTOR => {
-                                    // If the error selector is 0, it means the error is a string
-                                    let string = revert_values_iter
-                                        .map(|memory_value| {
-                                            let as_u8: u8 = memory_value
-                                                .try_into()
-                                                .expect("String item is not u8");
-                                            as_u8 as char
-                                        })
-                                        .collect();
-                                    Some(ResolvedAssertionPayload::String(string))
-                                }
-                                _ => {
-                                    // If the error selector is not 0, it means the error is a custom error
-                                    Some(ResolvedAssertionPayload::Raw(RawAssertionPayload {
-                                        selector: error_selector,
-                                        data: revert_values_iter
-                                            .map(|value| value.to_field())
-                                            .collect(),
-                                    }))
-                                }
-                            }
-                        }
-                    }
-                };
                 Err(OpcodeResolutionError::BrilligFunctionFailed {
-                    payload,
-                    call_stack: call_stack
-                        .iter()
-                        .map(|brillig_index| OpcodeLocation::Brillig {
-                            acir_index: self.acir_index,
-                            brillig_index: *brillig_index,
-                        })
-                        .collect(),
+                    payload: self.extract_failure_payload(&reason),
+                    call_stack,
+                    reason,
                 })
             }
             VMStatus::ForeignCallWait { function, inputs } => {
@@ -226,6 +182,57 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
         }
     }
 
+    fn extract_failure_payload(
+        &self,
+        reason: &FailureReason,
+    ) -> Option<ResolvedAssertionPayload<F>> {
+        match reason {
+            FailureReason::RuntimeError { message } => {
+                Some(ResolvedAssertionPayload::String(message.clone()))
+            }
+            FailureReason::Trap { revert_data_offset, revert_data_size } => {
+                // Since noir can only revert with strings currently, we can parse return data as a string
+                let payload = if *revert_data_size == 0 {
+                    None
+                } else {
+                    let memory = self.vm.get_memory();
+                    let mut revert_values_iter = memory
+                        [*revert_data_offset..(*revert_data_offset + *revert_data_size)]
+                        .iter();
+                    let error_selector = ErrorSelector::new(
+                        revert_values_iter
+                            .next()
+                            .expect("Incorrect revert data size")
+                            .try_into()
+                            .expect("Error selector is not u64"),
+                    );
+
+                    match error_selector {
+                        STRING_ERROR_SELECTOR => {
+                            // If the error selector is 0, it means the error is a string
+                            let string = revert_values_iter
+                                .map(|memory_value| {
+                                    let as_u8: u8 =
+                                        memory_value.try_into().expect("String item is not u8");
+                                    as_u8 as char
+                                })
+                                .collect();
+                            Some(ResolvedAssertionPayload::String(string))
+                        }
+                        _ => {
+                            // If the error selector is not 0, it means the error is a custom error
+                            Some(ResolvedAssertionPayload::Raw(RawAssertionPayload {
+                                selector: error_selector,
+                                data: revert_values_iter.map(|value| value.to_field()).collect(),
+                            }))
+                        }
+                    }
+                };
+                payload
+            }
+        }
+    }
+
     pub(crate) fn finalize(
         self,
         witness: &mut WitnessMap<F>,

acvm-repo/acvm/src/pwg/mod.rs

@@ -13,6 +13,7 @@ use acir::{
     AcirField, BlackBoxFunc,
 };
 use acvm_blackbox_solver::BlackBoxResolutionError;
+use brillig_vm::FailureReason;
 
 use self::{
     arithmetic::ExpressionSolver, blackbox::bigint::AcvmBigIntSolver, directives::solve_directives,
@@ -116,28 +117,53 @@ impl std::fmt::Display for ErrorLocation {
     }
 }
 
-#[derive(Clone, PartialEq, Eq, Debug, Error)]
+#[derive(Clone, PartialEq, Eq, Debug)]
 pub enum OpcodeResolutionError<F> {
-    #[error("Cannot solve opcode: {0}")]
-    OpcodeNotSolvable(#[from] OpcodeNotSolvable<F>),
-    #[error("Cannot satisfy constraint")]
+    OpcodeNotSolvable(OpcodeNotSolvable<F>),
     UnsatisfiedConstrain {
         opcode_location: ErrorLocation,
         payload: Option<ResolvedAssertionPayload<F>>,
     },
-    #[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}")]
+    IndexOutOfBounds {
+        opcode_location: ErrorLocation,
+        index: u32,
+        array_size: u32,
+    },
     BlackBoxFunctionFailed(BlackBoxFunc, String),
-    #[error("Failed to solve brillig function")]
     BrilligFunctionFailed {
         call_stack: Vec<OpcodeLocation>,
         payload: Option<ResolvedAssertionPayload<F>>,
+        reason: FailureReason,
+    },
+    AcirMainCallAttempted {
+        opcode_location: ErrorLocation,
+    },
+    AcirCallOutputsMismatch {
+        opcode_location: ErrorLocation,
+        results_size: u32,
+        outputs_size: u32,
     },
-    #[error("Attempted to call `main` with a `Call` opcode")]
-    AcirMainCallAttempted { opcode_location: ErrorLocation },
-    #[error("{results_size:?} result values were provided for {outputs_size:?} call output witnesses, most likely due to bad ACIR codegen")]
-    AcirCallOutputsMismatch { opcode_location: ErrorLocation, results_size: u32, outputs_size: u32 },
+}
+
+impl<F: acir::AcirField> std::fmt::Display for OpcodeResolutionError<F> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            OpcodeResolutionError::OpcodeNotSolvable(opcode_not_solvable) => write!(f, "Cannot solve opcode: {}", opcode_not_solvable),
+            OpcodeResolutionError::UnsatisfiedConstrain { .. } => write!(f, "Cannot satisfy constraint"),
+            OpcodeResolutionError::IndexOutOfBounds { array_size, index, .. } => write!(f, "Index out of bounds, array has size {}, but index was {}", array_size, index),
+            OpcodeResolutionError::BlackBoxFunctionFailed(func, reason) => write!(f,"Failed to solve blackbox function: {}, reason: {}", func, reason),
+            OpcodeResolutionError::BrilligFunctionFailed{reason: FailureReason::Trap {.. }, .. } => write!(f, "Cannot satisfy constraint"),
+            OpcodeResolutionError::BrilligFunctionFailed{ .. } => write!(f, "Failed to solve brillig function"),
+            OpcodeResolutionError::AcirMainCallAttempted{ .. } => write!(f, "Attempted to call `main` with a `Call` opcode"),
+            OpcodeResolutionError::AcirCallOutputsMismatch {results_size, outputs_size, .. } => write!(f, "{} result values were provided for {} call output witnesses, most likely due to bad ACIR codegen", results_size, outputs_size),
+    }
+    }
+}
+
+impl<F> From<OpcodeNotSolvable<F>> for OpcodeResolutionError<F> {
+    fn from(opcode: OpcodeNotSolvable<F>) -> Self {
+        OpcodeResolutionError::OpcodeNotSolvable(opcode)
+    }
 }
 
 impl<F> From<BlackBoxResolutionError> for OpcodeResolutionError<F> {
@@ -501,7 +527,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver<F>> ACVM<'a, F, B> {
 
     fn map_brillig_error(&self, mut err: OpcodeResolutionError<F>) -> OpcodeResolutionError<F> {
         match &mut err {
-            OpcodeResolutionError::BrilligFunctionFailed { call_stack, payload } => {
+            OpcodeResolutionError::BrilligFunctionFailed { call_stack, payload, .. } => {
                 // Some brillig errors have static strings as payloads, we can resolve them here
                 let last_location =
                     call_stack.last().expect("Call stacks should have at least one item");

acvm-repo/acvm/tests/solver.rs

@@ -14,7 +14,7 @@ use acir::{
 
 use acvm::pwg::{ACVMStatus, ErrorLocation, ForeignCallWaitInfo, OpcodeResolutionError, ACVM};
 use acvm_blackbox_solver::StubbedBlackBoxSolver;
-use brillig_vm::brillig::HeapValueType;
+use brillig_vm::{brillig::HeapValueType, FailureReason};
 
 // Reenable these test cases once we move the brillig implementation of inversion down into the acvm stdlib.
 
@@ -672,7 +672,8 @@ fn unsatisfied_opcode_resolved_brillig() {
         solver_status,
         ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed {
             payload: None,
-            call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }]
+            call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }],
+            reason: FailureReason::Trap { revert_data_offset: 0, revert_data_size: 0 },
         }),
         "The first opcode is not satisfiable, expected an error indicating this"
     );

tooling/debugger/src/context.rs

@@ -2,9 +2,10 @@ use crate::foreign_calls::DebugForeignCallExecutor;
 use acvm::acir::circuit::brillig::BrilligBytecode;
 use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation};
 use acvm::acir::native_types::{Witness, WitnessMap};
-use acvm::brillig_vm::MemoryValue;
+use acvm::brillig_vm::{FailureReason, MemoryValue};
 use acvm::pwg::{
-    ACVMStatus, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, StepResult, ACVM,
+    ACVMStatus, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo,
+    OpcodeResolutionError, StepResult, ACVM,
 };
 use acvm::{BlackBoxFunctionSolver, FieldElement};
 
@@ -295,10 +296,20 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> DebugContext<'a, B> {
                 self.brillig_solver = Some(solver);
                 self.handle_foreign_call(foreign_call)
             }
-            Err(err) => DebugCommandResult::Error(NargoError::ExecutionError(
-                // TODO: debugger does not handle multiple acir calls
-                ExecutionError::SolvingError(err, None),
-            )),
+            Err(err) => {
+                if let OpcodeResolutionError::BrilligFunctionFailed {
+                    reason: FailureReason::Trap { .. },
+                    ..
+                } = err
+                {
+                    // return solver ownership so brillig_solver it has the right opcode location
+                    self.brillig_solver = Some(solver);
+                }
+                // TODO: should we return solver ownership in all Err scenarios>?
+                DebugCommandResult::Error(NargoError::ExecutionError(ExecutionError::SolvingError(
+                    err, None,
+                )))
+            }
         }
     }