fix: add new BrilligFunctionUnsatisfiedConstrain resolution error v…

…ariant (#6) * Convert BrilligSolver Trap failure into `BrilligFunctionUnsatisfiedConstrain` error * extract extract_failure_payload function to improve readability * Changed the variant checking on ACVM.map_brillig_error from OpcodeResolutionError::BrilligFunctionFailed to OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain * return ownership of the solver to brillig context when the debugger fails due to `BrilligFunctionUnsatisfiedConstrain`
manastech · Jul 3, 2024 · 3936c62 · 3936c62
1 parent a8928dd
commit 3936c62
Show file tree

Hide file tree

Showing 7 changed files with 110 additions and 64 deletions.
diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs
@@ -162,70 +162,85 @@ 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();
+                let payload = self.extract_failure_payload(&reason);
 
-                            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(),
-                                    }))
-                                }
-                            }
+                let resolution_error = |payload, call_stack| match &reason {
+                    FailureReason::RuntimeError { .. } => {
+                        OpcodeResolutionError::BrilligFunctionFailed { payload, call_stack }
+                    }
+                    FailureReason::Trap { .. } => {
+                        OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain {
+                            payload,
+                            call_stack,
                         }
                     }
                 };
-                Err(OpcodeResolutionError::BrilligFunctionFailed {
-                    payload,
-                    call_stack: call_stack
-                        .iter()
-                        .map(|brillig_index| OpcodeLocation::Brillig {
-                            acir_index: self.acir_index,
-                            brillig_index: *brillig_index,
-                        })
-                        .collect(),
-                })
+
+                Err(resolution_error(payload, call_stack))
             }
             VMStatus::ForeignCallWait { function, inputs } => {
                 Ok(BrilligSolverStatus::ForeignCallWait(ForeignCallWaitInfo { function, inputs }))
             }
         }
     }
 
+    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
+                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(),
+                            }))
+                        }
+                    }
+                }
+            }
+        }
+    }
+
     pub(crate) fn finalize(
         self,
         witness: &mut WitnessMap<F>,

diff --git a/acvm-repo/acvm/src/pwg/mod.rs b/acvm-repo/acvm/src/pwg/mod.rs
@@ -134,6 +134,11 @@ pub enum OpcodeResolutionError<F> {
         call_stack: Vec<OpcodeLocation>,
         payload: Option<ResolvedAssertionPayload<F>>,
     },
+    #[error("Cannot satisfy constraint")]
+    BrilligFunctionUnsatisfiedConstrain {
+        call_stack: Vec<OpcodeLocation>,
+        payload: Option<ResolvedAssertionPayload<F>>,
+    },
     #[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")]
@@ -501,7 +506,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::BrilligFunctionUnsatisfiedConstrain { 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");

diff --git a/acvm-repo/acvm/tests/solver.rs b/acvm-repo/acvm/tests/solver.rs
@@ -670,7 +670,7 @@ fn unsatisfied_opcode_resolved_brillig() {
     let solver_status = acvm.solve();
     assert_eq!(
         solver_status,
-        ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed {
+        ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain {
             payload: None,
             call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }]
         }),

diff --git a/acvm-repo/acvm_js/src/execute.rs b/acvm-repo/acvm_js/src/execute.rs
@@ -200,9 +200,11 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ProgramExecutor<'a, B> {
                                 opcode_location: ErrorLocation::Resolved(opcode_location),
                                 ..
                             } => Some(vec![*opcode_location]),
-                            OpcodeResolutionError::BrilligFunctionFailed { call_stack, .. } => {
-                                Some(call_stack.clone())
-                            }
+                            OpcodeResolutionError::BrilligFunctionFailed { call_stack, .. }
+                            | OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain {
+                                call_stack,
+                                ..
+                            } => Some(call_stack.clone()),
                             _ => None,
                         };
                         // If the failed opcode has an assertion message, integrate it into the error message for backwards compatibility.
@@ -215,6 +217,10 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ProgramExecutor<'a, B> {
                             | OpcodeResolutionError::BrilligFunctionFailed {
                                 payload: Some(payload),
                                 ..
+                            }
+                            | OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain {
+                                payload: Some(payload),
+                                ..
                             } => match payload {
                                 ResolvedAssertionPayload::Raw(raw_payload) => {
                                     ("Assertion failed".to_string(), Some(raw_payload))

diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs
@@ -4,7 +4,8 @@ use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation};
 use acvm::acir::native_types::{Witness, WitnessMap};
 use acvm::brillig_vm::MemoryValue;
 use acvm::pwg::{
-    ACVMStatus, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, StepResult, ACVM,
+    ACVMStatus, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, OpcodeResolutionError,
+    StepResult, ACVM,
 };
 use acvm::{BlackBoxFunctionSolver, FieldElement};
 
@@ -295,10 +296,16 @@ 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::BrilligFunctionUnsatisfiedConstrain { .. } = 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,
+                )))
+            }
         }
     }
 

diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs
@@ -84,6 +84,7 @@ impl<F: AcirField> NargoError<F> {
                 | OpcodeResolutionError::UnsatisfiedConstrain { .. }
                 | OpcodeResolutionError::AcirMainCallAttempted { .. }
                 | OpcodeResolutionError::BrilligFunctionFailed { .. }
+                | OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain { .. }
                 | OpcodeResolutionError::AcirCallOutputsMismatch { .. } => None,
                 OpcodeResolutionError::BlackBoxFunctionFailed(_, reason) => {
                     Some(reason.to_string())
@@ -111,6 +112,10 @@ fn extract_locations_from_error<F: AcirField>(
         ExecutionError::SolvingError(
             OpcodeResolutionError::BrilligFunctionFailed { .. },
             acir_call_stack,
+        )
+        | ExecutionError::SolvingError(
+            OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain { .. },
+            acir_call_stack,
         ) => acir_call_stack.clone(),
         ExecutionError::AssertionFailed(_, call_stack) => Some(call_stack.clone()),
         ExecutionError::SolvingError(

diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs
@@ -97,7 +97,11 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver<F>, E: ForeignCallExecutor<F>>
                             self.call_stack.push(resolved_location);
                             Some(self.call_stack.clone())
                         }
-                        OpcodeResolutionError::BrilligFunctionFailed { call_stack, .. } => {
+                        OpcodeResolutionError::BrilligFunctionFailed { call_stack, .. }
+                        | OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain {
+                            call_stack,
+                            ..
+                        } => {
                             let brillig_call_stack =
                                 call_stack.iter().map(|location| ResolvedOpcodeLocation {
                                     acir_function_index: self.current_function_index,
@@ -111,6 +115,10 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver<F>, E: ForeignCallExecutor<F>>
 
                     let assertion_payload: Option<ResolvedAssertionPayload<F>> = match &error {
                         OpcodeResolutionError::BrilligFunctionFailed { payload, .. }
+                        | OpcodeResolutionError::BrilligFunctionUnsatisfiedConstrain {
+                            payload,
+                            ..
+                        }
                         | OpcodeResolutionError::UnsatisfiedConstrain { payload, .. } => {
                             payload.clone()
                         }