Skip to content
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.

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

fix(debugger): improve output when assertion fails in Brillig #4

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 24 additions & 18 deletions acvm-repo/acvm/src/pwg/brillig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,28 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
match vm_status {
VMStatus::Finished { .. } => Ok(BrilligSolverStatus::Finished),
VMStatus::InProgress => Ok(BrilligSolverStatus::InProgress),
VMStatus::ForeignCallWait { function, inputs } => {
Ok(BrilligSolverStatus::ForeignCallWait(ForeignCallWaitInfo { function, inputs }))
}
VMStatus::Failure { reason, call_stack } => {
let payload = match reason {
match reason {
FailureReason::RuntimeError { message } => {
Some(ResolvedAssertionPayload::String(message))
let call_stack = call_stack
.iter()
.map(|brillig_index| OpcodeLocation::Brillig {
acir_index: self.acir_index,
brillig_index: *brillig_index,
})
.collect();
Err(OpcodeResolutionError::BrilligFunctionFailed {
payload: Some(ResolvedAssertionPayload::String(message)),
call_stack,
})
}

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 {
let payload = if revert_data_size == 0 {
None
} else {
let memory = self.vm.get_memory();
Expand Down Expand Up @@ -206,22 +220,14 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
}))
}
}
}
}
};
Err(OpcodeResolutionError::BrilligFunctionFailed {
payload,
call_stack: call_stack
.iter()
.map(|brillig_index| OpcodeLocation::Brillig {
acir_index: self.acir_index,
brillig_index: *brillig_index,
};

Err(OpcodeResolutionError::UnsatisfiedConstrain {
opcode_location: super::ErrorLocation::Unresolved,
payload,
})
.collect(),
})
}
VMStatus::ForeignCallWait { function, inputs } => {
Ok(BrilligSolverStatus::ForeignCallWait(ForeignCallWaitInfo { function, inputs }))
}
}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions acvm-repo/acvm/tests/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -670,9 +670,9 @@ fn unsatisfied_opcode_resolved_brillig() {
let solver_status = acvm.solve();
assert_eq!(
solver_status,
ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed {
payload: None,
call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }]
ACVMStatus::Failure(OpcodeResolutionError::UnsatisfiedConstrain {
opcode_location: ErrorLocation::Resolved(OpcodeLocation::Acir(0)),
payload: None
}),
"The first opcode is not satisfiable, expected an error indicating this"
);
Expand Down
15 changes: 11 additions & 4 deletions tooling/debugger/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use acvm::brillig_vm::MemoryValue;
use acvm::pwg::{
ACVMStatus, AcirCallWaitInfo, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo,
OpcodeNotSolvable, StepResult, ACVM,
OpcodeNotSolvable, OpcodeResolutionError, StepResult, ACVM,
};
use acvm::{BlackBoxFunctionSolver, FieldElement};

Expand Down Expand Up @@ -471,9 +471,16 @@
self.brillig_solver = Some(solver);
self.handle_foreign_call(foreign_call)
}
Err(err) => DebugCommandResult::Error(NargoError::ExecutionError(
ExecutionError::SolvingError(err, None),
)),
Err(err) => {
if let OpcodeResolutionError::UnsatisfiedConstrain { .. } = 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 Expand Up @@ -897,7 +904,7 @@
outputs: vec![],
predicate: None,
}];
let brillig_funcs = &vec![brillig_bytecode];

Check warning on line 907 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
let current_witness_index = 2;
let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() };
let circuits = &vec![circuit];
Expand All @@ -916,7 +923,7 @@
debug_artifact,
initial_witness,
foreign_call_executor,
brillig_funcs,

Check warning on line 926 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
);

assert_eq!(
Expand Down Expand Up @@ -1035,14 +1042,14 @@

let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let brillig_funcs = &vec![brillig_bytecode];

Check warning on line 1045 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
let mut context = DebugContext::new(
&StubbedBlackBoxSolver,
circuits,
debug_artifact,
initial_witness,
foreign_call_executor,
brillig_funcs,

Check warning on line 1052 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
);

// set breakpoint
Expand Down Expand Up @@ -1109,7 +1116,7 @@
};
let circuits = vec![circuit_one, circuit_two];
let debug_artifact = DebugArtifact { debug_symbols: vec![], file_map: BTreeMap::new() };
let brillig_funcs = &vec![brillig_one, brillig_two];

Check warning on line 1119 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)

let context = DebugContext::new(
&StubbedBlackBoxSolver,
Expand Down
Loading