Skip to content

Commit

Permalink
feat: Evaluation of dynamic assert messages (#4101)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #3113

## Summary\*

The first major change is a new MacroProcessor that generates an oracle
and the respective unconstrained oracle wrapper for resolving assert
message expressions. This was done as to prevent exposing the resolve
assert message funcs in the stdlib directly.

Before an assertion with a dynamic assert message we perform a foreign
call out to the user with the format-string and the values to be
formatted. The user must then return what the assert message should look
like. This is then cached on by nargo when executing the ACVM and should
the next opcode fail to execute then this error message will be returned
to the user.

We now require end-users (people running ACVM) to format their own
assert messages.

Example output for `assert_msg_runtime` test:
<img width="737" alt="Screenshot 2024-01-23 at 4 07 18 PM"
src="https://github.com/noir-lang/noir/assets/43554004/96879ab1-5192-4b01-b798-4a63631bd270">

Assert message still work in the stdlib as expected:
<img width="780" alt="Screenshot 2024-01-23 at 4 43 47 PM"
src="https://github.com/noir-lang/noir/assets/43554004/1ad0c00b-7d5f-4a3f-94d8-955589f487c2">

Output for debug logs when putting `assert(x == y, struct_string);` on
line 48 immediately following `std::println(struct_string);`:
<img width="1040" alt="Screenshot 2024-01-23 at 4 47 55 PM"
src="https://github.com/noir-lang/noir/assets/43554004/6a8220b8-dd19-4e1b-b360-976a6952a36f">


## Additional Context

## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <tom@tomfren.ch>
  • Loading branch information
3 people authored Feb 1, 2024
1 parent 0e2a552 commit c284e01
Show file tree
Hide file tree
Showing 48 changed files with 701 additions and 162 deletions.
9 changes: 9 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
[workspace]

members = [
# Macros crates for metaprogramming
"aztec_macros",
"noirc_macros",
# Compiler crates
"compiler/noirc_evaluator",
"compiler/noirc_frontend",
"compiler/noirc_errors",
Expand Down
4 changes: 4 additions & 0 deletions acvm-repo/acir/src/circuit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ pub struct Circuit {
// Note: This should be a BTreeMap, but serde-reflect is creating invalid
// c++ code at the moment when it is, due to OpcodeLocation needing a comparison
// implementation which is never generated.
//
// TODO: These are only used for constraints that are explicitly created during code generation (such as index out of bounds on slices)
// TODO: We should move towards having all the checks being evaluated in the same manner
// TODO: as runtime assert messages specified by the user. This will also be a breaking change as the `Circuit` structure will change.
pub assert_messages: Vec<(OpcodeLocation, String)>,
}

Expand Down
2 changes: 1 addition & 1 deletion acvm-repo/brillig/src/foreign_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl ForeignCallParam {
}

/// Represents the full output of a [foreign call][crate::Opcode::ForeignCall].
#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)]
#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone, Default)]
pub struct ForeignCallResult {
/// Resolved output values of the foreign call.
pub values: Vec<ForeignCallParam>,
Expand Down
2 changes: 1 addition & 1 deletion acvm-repo/brillig_vm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ impl<'a, B: BlackBoxFunctionSolver> VM<'a, B> {
self.registers.set(*value_index, *value);
}
_ => unreachable!(
"Function result size does not match brillig bytecode (expected 1 result)"
"Function result size does not match brillig bytecode. Expected 1 result but got {output:?}"
),
},
RegisterOrMemory::HeapArray(HeapArray { pointer: pointer_index, size }) => {
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_driver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ rust-embed.workspace = true
tracing.workspace = true

aztec_macros = { path = "../../aztec_macros" }
noirc_macros = { path = "../../noirc_macros" }
10 changes: 7 additions & 3 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub struct CompileOptions {
#[arg(long, hide = true)]
pub only_acir: bool,

/// Disables the builtin macros being used in the compiler
/// Disables the builtin Aztec macros being used in the compiler
#[arg(long, hide = true)]
pub disable_macros: bool,

Expand Down Expand Up @@ -191,9 +191,12 @@ pub fn check_crate(
disable_macros: bool,
) -> CompilationResult<()> {
let macros: Vec<&dyn MacroProcessor> = if disable_macros {
vec![]
vec![&noirc_macros::AssertMessageMacro as &dyn MacroProcessor]
} else {
vec![&aztec_macros::AztecMacro as &dyn MacroProcessor]
vec![
&aztec_macros::AztecMacro as &dyn MacroProcessor,
&noirc_macros::AssertMessageMacro as &dyn MacroProcessor,
]
};

let mut errors = vec![];
Expand Down Expand Up @@ -244,6 +247,7 @@ pub fn compile_main(
let compiled_program =
compile_no_check(context, options, main, cached_program, options.force_compile)
.map_err(FileDiagnostic::from)?;

let compilation_warnings = vecmap(compiled_program.warnings.clone(), FileDiagnostic::from);
if options.deny_warnings && !compilation_warnings.is_empty() {
return Err(compilation_warnings);
Expand Down
33 changes: 28 additions & 5 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::brillig::brillig_ir::{
BrilligBinaryOp, BrilligContext, BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE,
};
use crate::ssa::ir::dfg::CallStack;
use crate::ssa::ir::instruction::ConstrainError;
use crate::ssa::ir::{
basic_block::{BasicBlock, BasicBlockId},
dfg::DataFlowGraph,
Expand Down Expand Up @@ -265,7 +266,30 @@ impl<'block> BrilligBlock<'block> {
condition,
);

self.brillig_context.constrain_instruction(condition, assert_message.clone());
let assert_message = if let Some(error) = assert_message {
match error.as_ref() {
ConstrainError::Static(string) => Some(string.clone()),
ConstrainError::Dynamic(call_instruction) => {
let Instruction::Call { func, arguments } = call_instruction else {
unreachable!("expected a call instruction")
};

let Value::Function(func_id) = &dfg[*func] else {
unreachable!("expected a function value")
};

self.convert_ssa_function_call(*func_id, arguments, dfg, &[]);

// Dynamic assert messages are handled in the generated function call.
// We then don't need to attach one to the constrain instruction.
None
}
}
} else {
None
};

self.brillig_context.constrain_instruction(condition, assert_message);
self.brillig_context.deallocate_register(condition);
}
Instruction::Allocate => {
Expand Down Expand Up @@ -369,7 +393,8 @@ impl<'block> BrilligBlock<'block> {
}
}
Value::Function(func_id) => {
self.convert_ssa_function_call(*func_id, arguments, dfg, instruction_id);
let result_ids = dfg.instruction_results(instruction_id);
self.convert_ssa_function_call(*func_id, arguments, dfg, result_ids);
}
Value::Intrinsic(Intrinsic::BlackBox(bb_func)) => {
// Slices are represented as a tuple of (length, slice contents).
Expand Down Expand Up @@ -638,16 +663,14 @@ impl<'block> BrilligBlock<'block> {
func_id: FunctionId,
arguments: &[ValueId],
dfg: &DataFlowGraph,
instruction_id: InstructionId,
result_ids: &[ValueId],
) {
// Convert the arguments to registers casting those to the types of the receiving function
let argument_registers: Vec<RegisterIndex> = arguments
.iter()
.flat_map(|argument_id| self.convert_ssa_value(*argument_id, dfg).extract_registers())
.collect();

let result_ids = dfg.instruction_results(instruction_id);

// Create label for the function that will be called
let label_of_function_to_call = FunctionContext::function_id_to_function_label(func_id);

Expand Down
130 changes: 85 additions & 45 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::fmt::Debug;
use self::acir_ir::acir_variable::{AcirContext, AcirType, AcirVar};
use super::function_builder::data_bus::DataBus;
use super::ir::dfg::CallStack;
use super::ir::instruction::ConstrainError;
use super::{
ir::{
dfg::DataFlowGraph,
Expand Down Expand Up @@ -413,15 +414,94 @@ impl Context {
let lhs = self.convert_numeric_value(*lhs, dfg)?;
let rhs = self.convert_numeric_value(*rhs, dfg)?;

self.acir_context.assert_eq_var(lhs, rhs, assert_message.clone())?;
let assert_message = if let Some(error) = assert_message {
match error.as_ref() {
ConstrainError::Static(string) => Some(string.clone()),
ConstrainError::Dynamic(call_instruction) => {
self.convert_ssa_call(call_instruction, dfg, ssa, brillig, &[])?;
None
}
}
} else {
None
};

self.acir_context.assert_eq_var(lhs, rhs, assert_message)?;
}
Instruction::Cast(value_id, _) => {
let acir_var = self.convert_numeric_value(*value_id, dfg)?;
self.define_result_var(dfg, instruction_id, acir_var);
}
Instruction::Call { func, arguments } => {
Instruction::Call { .. } => {
let result_ids = dfg.instruction_results(instruction_id);
match &dfg[*func] {
warnings.extend(self.convert_ssa_call(
instruction,
dfg,
ssa,
brillig,
result_ids,
)?);
}
Instruction::Not(value_id) => {
let (acir_var, typ) = match self.convert_value(*value_id, dfg) {
AcirValue::Var(acir_var, typ) => (acir_var, typ),
_ => unreachable!("NOT is only applied to numerics"),
};
let result_acir_var = self.acir_context.not_var(acir_var, typ)?;
self.define_result_var(dfg, instruction_id, result_acir_var);
}
Instruction::Truncate { value, bit_size, max_bit_size } => {
let result_acir_var =
self.convert_ssa_truncate(*value, *bit_size, *max_bit_size, dfg)?;
self.define_result_var(dfg, instruction_id, result_acir_var);
}
Instruction::EnableSideEffects { condition } => {
let acir_var = self.convert_numeric_value(*condition, dfg)?;
self.current_side_effects_enabled_var = acir_var;
}
Instruction::ArrayGet { .. } | Instruction::ArraySet { .. } => {
self.handle_array_operation(instruction_id, dfg, last_array_uses)?;
}
Instruction::Allocate => {
unreachable!("Expected all allocate instructions to be removed before acir_gen")
}
Instruction::Store { .. } => {
unreachable!("Expected all store instructions to be removed before acir_gen")
}
Instruction::Load { .. } => {
unreachable!("Expected all load instructions to be removed before acir_gen")
}
Instruction::IncrementRc { .. } => {
// Do nothing. Only Brillig needs to worry about reference counted arrays
}
Instruction::RangeCheck { value, max_bit_size, assert_message } => {
let acir_var = self.convert_numeric_value(*value, dfg)?;
self.acir_context.range_constrain_var(
acir_var,
&NumericType::Unsigned { bit_size: *max_bit_size },
assert_message.clone(),
)?;
}
}

self.acir_context.set_call_stack(CallStack::new());
Ok(warnings)
}

fn convert_ssa_call(
&mut self,
instruction: &Instruction,
dfg: &DataFlowGraph,
ssa: &Ssa,
brillig: &Brillig,
result_ids: &[ValueId],
) -> Result<Vec<SsaReport>, RuntimeError> {
let mut warnings = Vec::new();

match instruction {
Instruction::Call { func, arguments } => {
let function_value = &dfg[*func];
match function_value {
Value::Function(id) => {
let func = &ssa.functions[id];
match func.runtime() {
Expand Down Expand Up @@ -495,51 +575,11 @@ impl Context {
Value::ForeignFunction(_) => unreachable!(
"All `oracle` methods should be wrapped in an unconstrained fn"
),
_ => unreachable!("expected calling a function"),
_ => unreachable!("expected calling a function but got {function_value:?}"),
}
}
Instruction::Not(value_id) => {
let (acir_var, typ) = match self.convert_value(*value_id, dfg) {
AcirValue::Var(acir_var, typ) => (acir_var, typ),
_ => unreachable!("NOT is only applied to numerics"),
};
let result_acir_var = self.acir_context.not_var(acir_var, typ)?;
self.define_result_var(dfg, instruction_id, result_acir_var);
}
Instruction::Truncate { value, bit_size, max_bit_size } => {
let result_acir_var =
self.convert_ssa_truncate(*value, *bit_size, *max_bit_size, dfg)?;
self.define_result_var(dfg, instruction_id, result_acir_var);
}
Instruction::EnableSideEffects { condition } => {
let acir_var = self.convert_numeric_value(*condition, dfg)?;
self.current_side_effects_enabled_var = acir_var;
}
Instruction::ArrayGet { .. } | Instruction::ArraySet { .. } => {
self.handle_array_operation(instruction_id, dfg, last_array_uses)?;
}
Instruction::Allocate => {
unreachable!("Expected all allocate instructions to be removed before acir_gen")
}
Instruction::Store { .. } => {
unreachable!("Expected all store instructions to be removed before acir_gen")
}
Instruction::Load { .. } => {
unreachable!("Expected all load instructions to be removed before acir_gen")
}
Instruction::IncrementRc { .. } => {
// Do nothing. Only Brillig needs to worry about reference counted arrays
}
Instruction::RangeCheck { value, max_bit_size, assert_message } => {
let acir_var = self.convert_numeric_value(*value, dfg)?;
self.acir_context.range_constrain_var(
acir_var,
&NumericType::Unsigned { bit_size: *max_bit_size },
assert_message.clone(),
)?;
}
_ => unreachable!("expected calling a call instruction"),
}
self.acir_context.set_call_stack(CallStack::new());
Ok(warnings)
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use super::{
basic_block::BasicBlock,
dfg::{CallStack, InsertInstructionResult},
function::RuntimeType,
instruction::{Endian, InstructionId, Intrinsic},
instruction::{ConstrainError, Endian, InstructionId, Intrinsic},
types::NumericType,
},
ssa_gen::Ssa,
Expand Down Expand Up @@ -250,7 +250,7 @@ impl FunctionBuilder {
&mut self,
lhs: ValueId,
rhs: ValueId,
assert_message: Option<String>,
assert_message: Option<Box<ConstrainError>>,
) {
self.insert_instruction(Instruction::Constrain(lhs, rhs, assert_message), None);
}
Expand Down
Loading

0 comments on commit c284e01

Please sign in to comment.