Skip to content

fix: add new BrilligFunctionUnsatisfiedConstrain resolution error variant #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anaPerezGhiglia merged 7 commits into from
Jul 3, 2024
Merged

fix: add new BrilligFunctionUnsatisfiedConstrain resolution error variant #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4aa3410
Convert BrilligSolver Trap failure into UnsatisfiedConstrain error
anaPerezGhiglia Jun 25, 2024
15d6f76
remove redundant field name
anaPerezGhiglia Jun 25, 2024
bff0ec3
Fix acir::solver unsatisfied_opcode_resolved_brillig test
anaPerezGhiglia Jun 26, 2024
8a6a99c
Create new OpcodeResolutionError enum value
anaPerezGhiglia Jun 28, 2024
5ace5b6
Reorder BrilligSolver.handle_vm_status match branches
anaPerezGhiglia Jul 1, 2024
e184be1
extract extract_failure_payload function to improve readability
anaPerezGhiglia Jul 1, 2024
4adf5d1
remove unnecessary type
anaPerezGhiglia Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 68 additions & 53 deletions acvm-repo/acvm/src/pwg/brillig.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
7 changes: 6 additions & 1 deletion acvm-repo/acvm/src/pwg/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion acvm-repo/acvm/tests/solver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }]
}),
Expand Down
12 changes: 9 additions & 3 deletions acvm-repo/acvm_js/src/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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))
Expand Down
17 changes: 12 additions & 5 deletions tooling/debugger/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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,
)))
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions tooling/nargo/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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(
Expand Down
10 changes: 9 additions & 1 deletion tooling/nargo/src/ops/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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()
}
Expand Down