Skip to content

chore(acir): validate predicate use against requires_acir_gen_predicate #13517

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

Open
asterite wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore(acir): validate predicate use against requires_acir_gen_predicate #13517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 52 additions & 23 deletions compiler/noirc_evaluator/src/acir/arrays.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,6 @@
index: ValueId,
store_value: Option<ValueId>,
) -> Result<bool, RuntimeError> {
if !self.acir_context.is_constant_zero(&self.current_side_effects_enabled_var) {
return Ok(false);
}

// The side-effects predicate is only this instruction's own for the instructions
// [`crate::ssa::opt::remove_enable_side_effects`] fences, that is those reporting
// `Instruction::requires_acir_gen_predicate == true`. A read at a statically safe index
Expand All @@ -311,10 +307,18 @@
// Resolving it as disabled is also unnecessary. A safe index is in bounds by construction,
// so it never needs the predicate's fallback to a valid slot and the ordinary path emits
// exactly the read the program asked for.
//
// This check runs before the predicate is inspected: a safe read's outcome here is
// "not handled" regardless of the predicate's value, so it is not a predicate read.
if store_value.is_none() && dfg.is_safe_index(index, array) {
return Ok(false);
}

let predicate = self.read_predicate();
if !self.acir_context.is_constant_zero(&predicate) {
return Ok(false);
}

let value = if store_value.is_some() {
self.convert_value(array, dfg)
} else {
Expand Down Expand Up @@ -351,7 +355,8 @@
}
// Make sure this code is disabled, or fail with "Index out of bounds".
let msg = "Index out of bounds, array has size 0".to_string();
self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?;
let predicate = self.read_predicate();
self.acir_context.assert_zero_var(predicate, msg)?;
Ok(true)
}

Expand Down Expand Up @@ -380,11 +385,27 @@
call_stack: self.acir_context.get_call_stack(),
}))
}
AcirValue::Array(array) => {
AcirValue::Array(array_value) => {
// `AcirValue::Array` supports reading/writing to constant indices at compile-time in some cases.
if let Some(constant_index) = self.constant_index(index, dfg)? {
let store_value = store_value.map(|value| self.convert_value(value, dfg));
self.handle_constant_index(instruction, dfg, array, constant_index, store_value)
let store = store_value.map(|value| self.convert_value(value, dfg));
let resolved = self.handle_constant_index(
instruction,
dfg,
array_value,
constant_index,
store,
)?;
// A compile-time read at an index that is not statically safe reports
// `requires_acir_gen_predicate = true`, yet resolves optimistically
// without consulting the predicate: if the predicate were false the
// result is a don't-care that downstream predication masks anyway.
if resolved && store_value.is_none() && !dfg.is_safe_index(index, array) {
self.predicate_not_needed(
"constant in-bounds index resolved at compile time",
);
}
Ok(resolved)
} else {
Ok(false)
}
Expand Down Expand Up @@ -432,7 +453,7 @@
&mut self,
instruction: InstructionId,
dfg: &DataFlowGraph,
array: imbl::Vector<AcirValue>,

Check warning on line 456 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
index: FieldElement,
store_value: Option<AcirValue>,
) -> Result<bool, RuntimeError> {
Expand All @@ -454,8 +475,8 @@
}

if let Some(store_value) = store_value {
let side_effects_always_enabled =
self.acir_context.is_constant_one(&self.current_side_effects_enabled_var);
let predicate = self.read_predicate();
let side_effects_always_enabled = self.acir_context.is_constant_one(&predicate);

if side_effects_always_enabled {
// If we know that this write will always occur then we can perform it at compile time.
Expand Down Expand Up @@ -561,15 +582,18 @@
let index_var =
self.get_flattened_index(&array_typ, array_id, index_var, dfg, gating, shift)?;

// Reads need no store predication; only the store value depends on the predicate.
let Some(store) = store_value else {
return Ok((index_var, None));
};

// Side-effects are always enabled so we do not need to do any predication
if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
let store_value = store_value.map(|store| self.convert_value(store, dfg));
return Ok((index_var, store_value));
let predicate = self.read_predicate();
if self.acir_context.is_constant_one(&predicate) {
return Ok((index_var, Some(self.convert_value(store, dfg))));
}

let new_value = store_value
.map(|store| self.predicated_store_value(store, dfg, array_id, index_var))
.transpose()?;
let new_value = Some(self.predicated_store_value(store, dfg, array_id, index_var)?);

Ok((index_var, new_value))
}
Expand Down Expand Up @@ -600,18 +624,17 @@
) -> Result<AcirValue, RuntimeError> {
match (store_value, dummy_value) {
(AcirValue::Var(store_var, typ), AcirValue::Var(dummy_var, _)) => {
let true_pred =
self.acir_context.mul_var(*store_var, self.current_side_effects_enabled_var)?;
let predicate = self.read_predicate();
let true_pred = self.acir_context.mul_var(*store_var, predicate)?;
let one = self.acir_context.add_constant(FieldElement::one());
let not_pred =
self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?;
let not_pred = self.acir_context.sub_var(one, predicate)?;
let false_pred = self.acir_context.mul_var(not_pred, *dummy_var)?;
// predicate*value + (1-predicate)*dummy
let new_value = self.acir_context.add_var(true_pred, false_pred)?;
Ok(AcirValue::Var(new_value, *typ))
}
(AcirValue::Array(values), AcirValue::Array(dummy_values)) => {
let mut elements = imbl::Vector::new();

Check warning on line 637 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)

assert_eq!(
values.len(),
Expand Down Expand Up @@ -646,7 +669,7 @@
let values: Vec<_> = self
.read_dynamic_array(*block_id, *len, value_types)
.collect::<Result<_, _>>()?;
let mut elements = imbl::Vector::new();

Check warning on line 672 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
for (val, dummy_val) in values.iter().zip_eq(dummy_values) {
elements.push_back(self.convert_array_set_store_value(val, &dummy_val)?);
}
Expand All @@ -672,7 +695,7 @@
match typ {
Type::Numeric(_) => self.array_get_value(typ, call_data_block, offset),
Type::Array(arc, len) => {
let mut result = imbl::Vector::new();

Check warning on line 698 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
for _i in 0..len.0 {
for sub_type in arc.iter() {
let element = self.get_from_call_data(offset, call_data_block, sub_type)?;
Expand Down Expand Up @@ -783,7 +806,7 @@
Ok(AcirValue::Var(read, *numeric_type))
}
Type::Array(element_types, len) => {
let mut values = imbl::Vector::new();

Check warning on line 809 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
for _ in 0..len.0 {
for typ in element_types.as_ref() {
values.push_back(self.array_get_value(typ, block_id, var_index)?);
Expand Down Expand Up @@ -1230,7 +1253,7 @@
// constant into a witness and hide its value. An out-of-bounds constant index (no table
// entry) falls through to the runtime path, which defers the bounds failure to execution.
//
// This resolved index is in bounds and ungated even when the caller asked for gating

Check warning on line 1256 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (ungated)
// ([`DataFlowGraph::is_safe_index`] cannot see it: it holds vector indices to the vector's
// unknown semantic length, so it is `false` for every vector). The access reads the slots
// the program asked for, so no fallback bias applies.
Expand All @@ -1245,6 +1268,11 @@
.get(index as usize)
.copied()
{
if matches!(gating, IndexGating::Gated { .. }) {
self.predicate_not_needed(
"constant index resolved to a fixed flattened offset at compile time",
);
}
return Ok(self.acir_context.add_constant(offset));
}

Expand All @@ -1256,7 +1284,8 @@
let var_index = match gating {
IndexGating::Safe => var_index,
IndexGating::Gated { .. } => {
self.acir_context.mul_var(var_index, self.current_side_effects_enabled_var)?
let predicate = self.read_predicate();
self.acir_context.mul_var(var_index, predicate)?
}
};

Expand All @@ -1276,8 +1305,8 @@
match gating {
IndexGating::Gated { fallback_offset } if fallback_offset != 0 => {
let one = self.acir_context.add_constant(FieldElement::one());
let not_pred =
self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?;
let predicate = self.read_predicate();
let not_pred = self.acir_context.sub_var(one, predicate)?;
let offset_var = self.acir_context.add_constant(fallback_offset);
let offset_term = self.acir_context.mul_var(offset_var, not_pred)?;
Ok(self.acir_context.add_var(flat_index, offset_term)?)
Expand Down Expand Up @@ -1412,7 +1441,7 @@
/// How an index is treated on a branch the side-effects predicate disables.
///
/// The two cases are one decision, not two independent knobs: a fallback slot is only reachable
/// because gating collapsed the index to `0` first, so an ungated index has no fallback to speak

Check warning on line 1444 in compiler/noirc_evaluator/src/acir/arrays.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (ungated)
/// of. [`Context::get_flattened_index`] is the only place that can tell them apart.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub(super) enum IndexGating {
Expand Down
13 changes: 12 additions & 1 deletion compiler/noirc_evaluator/src/acir/call/intrinsics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,23 @@ impl Context<'_> {
.map(|result_id| dfg.type_of_value(*result_id).flattened_size())
.sum();

// Only `RecursiveAggregation` consumes the side-effects predicate: it is
// injected as an extra witness input so aggregation can be conditionally
// executed. Other blackbox functions either take no predicate or receive
// one as an ordinary SSA-level argument (e.g. MSM, ECDSA).
let predicate =
if matches!(black_box, acvm::acir::BlackBoxFunc::RecursiveAggregation) {
Some(self.read_predicate())
} else {
None
};

let vars = self.acir_context.black_box_function(
black_box,
inputs,
None,
output_count,
Some(self.current_side_effects_enabled_var),
predicate,
)?;

Ok(self.convert_vars_to_values(vars, dfg, result_ids))
Expand Down
39 changes: 29 additions & 10 deletions compiler/noirc_evaluator/src/acir/call/intrinsics/vector_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
Ok(AcirValue::Var(read, *numeric_type))
}
Type::Array(element_types, len) => {
let mut result = imbl::Vector::new();

Check warning on line 67 in compiler/noirc_evaluator/src/acir/call/intrinsics/vector_ops.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
for _ in 0..len.0 {
for typ in element_types.as_ref() {
result.push_back(self.read_vector_value(
Expand Down Expand Up @@ -197,6 +197,9 @@
});
let new_vector_val = if let Some(len_const) = len_const {
// Length is known at compile time - we can precisely determine where to write
self.predicate_not_needed(
"vector length folded to a compile-time constant; elements placed inline",
);
let mut new_vector = self.read_array_with_type(vector, &vector_typ)?;
// length of Acir Values vector
let len = len_const.to_u128() as usize * elements_to_push.len();
Expand Down Expand Up @@ -278,9 +281,8 @@
// element-type-sizes table, but a whole-element append needs no per-member offsets:
// multiply the length by the flattened element size directly and skip building that
// table for this write.
let predicated_length = self
.acir_context
.mul_var(vector_length, self.current_side_effects_enabled_var)?;
let predicate = self.read_predicate();
let predicated_length = self.acir_context.mul_var(vector_length, predicate)?;
let element_flattened_size = self.acir_context.add_constant(elements_var.len());
self.acir_context.mul_var(predicated_length, element_flattened_size)?
};
Expand Down Expand Up @@ -390,7 +392,8 @@
if self.has_zero_length(vector_contents_id, dfg) {
// Make sure this code is disabled, or fail with the empty-vector pop message.
let msg = "Attempt to pop from an empty vector".to_string();
self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?;
let predicate = self.read_predicate();
self.acir_context.assert_zero_var(predicate, msg)?;

// Fill the result with default values.
let mut results = Vec::with_capacity(result_ids.len());
Expand Down Expand Up @@ -472,12 +475,17 @@
let assert_message = self.acir_context.generate_assertion_message_payload(
"Attempt to pop from an empty vector".to_string(),
);
let predicate = self.read_predicate();
self.acir_context.assert_neq_var(
vector_length_var,
zero,
self.current_side_effects_enabled_var,
predicate,
Some(assert_message),
)?;
} else {
// A known-constant, nonzero length (the constant-zero case was handled by the
// caller) needs neither the runtime emptiness assertion nor index gating.
self.predicate_not_needed("vector length is a known nonzero constant");
}

let one = self.acir_context.add_constant(FieldElement::one());
Expand All @@ -487,9 +495,8 @@
// to ensure we don't end up trying to look up an item at index -1, when the semantic length is 0,
// which can fail a circuit even when the side effects are disabled.
if is_unknown_length {
new_vector_length_var = self
.acir_context
.mul_var(new_vector_length_var, self.current_side_effects_enabled_var)?;
let predicate = self.read_predicate();
new_vector_length_var = self.acir_context.mul_var(new_vector_length_var, predicate)?;
}

Ok(new_vector_length_var)
Expand Down Expand Up @@ -551,7 +558,8 @@
if self.has_zero_length(vector_contents_id, dfg) {
// Make sure this code is disabled, or fail with the empty-vector pop message.
let msg = "Attempt to pop from an empty vector".to_string();
self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?;
let predicate = self.read_predicate();
self.acir_context.assert_zero_var(predicate, msg)?;

// Fill the result with default values.
let mut results = Vec::with_capacity(result_ids.len());
Expand Down Expand Up @@ -661,6 +669,11 @@
// Fetch the flattened index from the user provided index argument.
let item_size = self.acir_context.add_constant(elements_to_insert.len());
let is_safe_index = Self::is_index_safe(arguments[2], dfg, &vector_typ, vector_size);
if is_safe_index {
// A statically safe insert index needs no gating, so no lowering path below
// consults the predicate.
self.predicate_not_needed("insert index is statically safe");
}
let insert_index = self.acir_context.mul_var(insert_index, item_size)?;

// Because the insert index might be at the end of the vector, the element type sizes we
Expand Down Expand Up @@ -877,7 +890,8 @@
if self.has_zero_length(vector_contents, dfg) {
// Make sure this code is disabled, or fail with "Index out of bounds".
let msg = "Index out of bounds, vector has size 0".to_string();
self.acir_context.assert_zero_var(self.current_side_effects_enabled_var, msg)?;
let predicate = self.read_predicate();
self.acir_context.assert_zero_var(predicate, msg)?;

// Fill the result with default values.
let mut results = Vec::with_capacity(result_ids.len());
Expand Down Expand Up @@ -920,6 +934,11 @@
let item_size_var = self.acir_context.add_constant(item_size);
let remove_index = self.acir_context.mul_var(remove_index, item_size_var)?;
let is_safe_index = Self::is_index_safe(arguments[2], dfg, &vector_typ, vector_size);
if is_safe_index {
// A statically safe remove index needs no gating, so no lowering path below
// consults the predicate.
self.predicate_not_needed("remove index is statically safe");
}

// Fetch the flattened index from the user provided index argument.
let flat_user_index = self.get_flattened_index(
Expand Down
8 changes: 5 additions & 3 deletions compiler/noirc_evaluator/src/acir/call/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,12 @@
);
};

let predicate = self.read_predicate();
let output_vars = self.acir_context.call_acir_function(
AcirFunctionId::new(acir_function_id),
inputs,
output_count,
self.current_side_effects_enabled_var,
predicate,
)?;

let output_values = self.convert_vars_to_values(output_vars, dfg, result_ids);
Expand All @@ -143,13 +144,14 @@
vecmap(result_ids, |result_id| dfg.type_of_value(*result_id).as_ref().into());

// Reuse or generate Brillig code
let predicate = self.read_predicate();
let output_values = if let Some(generated_pointer) =
self.shared_context.generated_brillig_pointer(func.id(), arguments.clone())
{
let code = self.shared_context.generated_brillig(generated_pointer.as_usize());
let skip_output_range_checks = false;
self.acir_context.brillig_call(
self.current_side_effects_enabled_var,
predicate,
code,
inputs,
outputs,
Expand All @@ -162,7 +164,7 @@
let generated_pointer = self.shared_context.new_generated_pointer();
let skip_output_range_checks = false;
let output_values = self.acir_context.brillig_call(
self.current_side_effects_enabled_var,
predicate,
&code,
inputs,
outputs,
Expand Down Expand Up @@ -276,7 +278,7 @@
.constant(&len, "len".to_string())
.expect("ICE - expected the variable to be a constant value")
.to_u128();
let mut element_values = imbl::Vector::new();

Check warning on line 281 in compiler/noirc_evaluator/src/acir/call/mod.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
for _ in 0..len {
for element_type in elements_type.iter() {
let element = Self::convert_var_type_to_values(element_type, &mut vars);
Expand Down Expand Up @@ -305,7 +307,7 @@
) -> AcirValue {
match result_type {
Type::Array(elements, size) => {
let mut element_values = imbl::Vector::new();

Check warning on line 310 in compiler/noirc_evaluator/src/acir/call/mod.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (imbl)
for _ in 0..size.0 {
for element_type in elements.iter() {
let element = Self::convert_var_type_to_values(element_type, vars);
Expand Down
Loading
Loading