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

feat: Parsing non-string assertion payloads in noir js #6079

Merged
merged 9 commits into from
May 3, 2024
Merged
53 changes: 46 additions & 7 deletions noir/noir-repo/acvm-repo/acir/src/circuit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,6 @@ pub struct Circuit {
pub recursive: bool,
}

/// This selector indicates that the payload is a string.
/// This is used to parse any error with a string payload directly,
/// to avoid users having to parse the error externally to the ACVM.
/// Only non-string errors need to be parsed externally to the ACVM using the circuit ABI.
pub const STRING_ERROR_SELECTOR: u64 = 0;

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub enum ExpressionOrMemory {
Expression(Expression),
Expand All @@ -93,10 +87,55 @@ pub enum AssertionPayload {
Dynamic(/* error_selector */ u64, Vec<ExpressionOrMemory>),
}

#[derive(Debug, Copy, PartialEq, Eq, Hash, Clone, PartialOrd, Ord)]
pub struct ErrorSelector(u64);

impl ErrorSelector {
pub fn new(integer: u64) -> Self {
ErrorSelector(integer)
}

pub fn as_u64(&self) -> u64 {
self.0
}
}

impl Serialize for ErrorSelector {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's serialized as string since the Serde<>JsValue bridge that we use serializes u64s as JS numbers, which don't have enough precision to store u64s.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just move to u32 right? I don't foresee error selectors hitting that bound. But this is non-blocking

fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
self.0.to_string().serialize(serializer)
}
}

impl<'de> Deserialize<'de> for ErrorSelector {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
let s: String = Deserialize::deserialize(deserializer)?;
let as_u64 = s.parse().map_err(serde::de::Error::custom)?;
Ok(ErrorSelector(as_u64))
}
}

/// This selector indicates that the payload is a string.
/// This is used to parse any error with a string payload directly,
/// to avoid users having to parse the error externally to the ACVM.
/// Only non-string errors need to be parsed externally to the ACVM using the circuit ABI.
pub const STRING_ERROR_SELECTOR: ErrorSelector = ErrorSelector(0);

#[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)]
pub struct RawAssertionPayload {
pub selector: ErrorSelector,
pub data: Vec<FieldElement>,
}

#[derive(Clone, PartialEq, Eq, Debug)]
pub enum ResolvedAssertionPayload {
String(String),
Raw(/*error_selector:*/ u64, Vec<FieldElement>),
Raw(RawAssertionPayload),
}

#[derive(Debug, Copy, Clone)]
Expand Down
25 changes: 15 additions & 10 deletions noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use acir::{
circuit::{
brillig::{BrilligInputs, BrilligOutputs},
opcodes::BlockId,
OpcodeLocation, ResolvedAssertionPayload, STRING_ERROR_SELECTOR,
ErrorSelector, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload,
STRING_ERROR_SELECTOR,
},
native_types::WitnessMap,
FieldElement,
Expand Down Expand Up @@ -173,11 +174,13 @@ impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> {
let mut revert_values_iter = memory
[revert_data_offset..(revert_data_offset + revert_data_size)]
.iter();
let error_selector = revert_values_iter
.next()
.expect("Incorrect revert data size")
.try_into()
.expect("Error selector is not u64");
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 => {
Expand All @@ -194,10 +197,12 @@ impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> {
}
_ => {
// If the error selector is not 0, it means the error is a custom error
Some(ResolvedAssertionPayload::Raw(
error_selector,
revert_values_iter.map(|value| value.to_field()).collect(),
))
Some(ResolvedAssertionPayload::Raw(RawAssertionPayload {
selector: error_selector,
data: revert_values_iter
.map(|value| value.to_field())
.collect(),
}))
}
}
}
Expand Down
13 changes: 9 additions & 4 deletions noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ use std::collections::HashMap;
use acir::{
brillig::ForeignCallResult,
circuit::{
brillig::BrilligBytecode, opcodes::BlockId, AssertionPayload, ExpressionOrMemory, Opcode,
OpcodeLocation, ResolvedAssertionPayload, STRING_ERROR_SELECTOR,
brillig::BrilligBytecode, opcodes::BlockId, AssertionPayload, ErrorSelector,
ExpressionOrMemory, Opcode, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload,
STRING_ERROR_SELECTOR,
},
native_types::{Expression, Witness, WitnessMap},
BlackBoxFunc, FieldElement,
Expand Down Expand Up @@ -425,8 +426,9 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> {
}
}
}
let error_selector = ErrorSelector::new(*error_selector);

Some(match *error_selector {
Some(match error_selector {
STRING_ERROR_SELECTOR => {
// If the error selector is 0, it means the error is a string
let string = fields
Expand All @@ -444,7 +446,10 @@ impl<'a, B: BlackBoxFunctionSolver> ACVM<'a, B> {
}
_ => {
// If the error selector is not 0, it means the error is a custom error
ResolvedAssertionPayload::Raw(*error_selector, fields)
ResolvedAssertionPayload::Raw(RawAssertionPayload {
selector: error_selector,
data: fields,
})
}
})
}
Expand Down
6 changes: 3 additions & 3 deletions noir/noir-repo/acvm-repo/acvm_js/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ impl<'a, B: BlackBoxFunctionSolver> ProgramExecutor<'a, B> {
};
// If the failed opcode has an assertion message, integrate it into the error message for backwards compatibility.
// Otherwise, pass the raw assertion payload as is.
let (message, raw_assertion_payload) = match &error {
let (message, raw_assertion_payload) = match error {
OpcodeResolutionError::UnsatisfiedConstrain {
payload: Some(payload),
..
Expand All @@ -281,8 +281,8 @@ impl<'a, B: BlackBoxFunctionSolver> ProgramExecutor<'a, B> {
payload: Some(payload),
..
} => match payload {
ResolvedAssertionPayload::Raw(selector, fields) => {
(error.to_string(), Some((*selector, fields.clone())))
ResolvedAssertionPayload::Raw(raw_payload) => {
("Assertion failed".to_string(), Some(raw_payload))
}
ResolvedAssertionPayload::String(message) => {
(format!("Assertion failed: {}", message), None)
Expand Down
27 changes: 8 additions & 19 deletions noir/noir-repo/acvm-repo/acvm_js/src/js_execution_error.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
use acvm::{acir::circuit::OpcodeLocation, FieldElement};
use js_sys::{Array, Error, JsString, Map, Object, Reflect};
use acvm::acir::circuit::{OpcodeLocation, RawAssertionPayload};
use gloo_utils::format::JsValueSerdeExt;
use js_sys::{Array, Error, JsString, Reflect};
use wasm_bindgen::prelude::{wasm_bindgen, JsValue};

use crate::js_witness_map::field_element_to_js_string;

#[wasm_bindgen(typescript_custom_section)]
const EXECUTION_ERROR: &'static str = r#"
export type RawAssertionPayload = {
selector: number;
fields: string[];
selector: string;
data: string[];
};
export type ExecutionError = Error & {
callStack?: string[];
Expand All @@ -35,7 +34,7 @@ impl JsExecutionError {
pub fn new(
message: String,
call_stack: Option<Vec<OpcodeLocation>>,
assertion_payload: Option<(u64, Vec<FieldElement>)>,
assertion_payload: Option<RawAssertionPayload>,
) -> Self {
let mut error = JsExecutionError::constructor(JsString::from(message));
let js_call_stack = match call_stack {
Expand All @@ -49,18 +48,8 @@ impl JsExecutionError {
None => JsValue::UNDEFINED,
};
let assertion_payload = match assertion_payload {
Some((selector, fields)) => {
let raw_payload_map = Map::new();
raw_payload_map
.set(&JsValue::from_str("selector"), &JsValue::from(selector.to_string()));
let js_fields = Array::new();
for field in fields {
js_fields.push(&field_element_to_js_string(&field));
}
raw_payload_map.set(&JsValue::from_str("fields"), &js_fields.into());

Object::from_entries(&raw_payload_map).unwrap().into()
}
Some(raw) => <wasm_bindgen::JsValue as JsValueSerdeExt>::from_serde(&raw)
.expect("Cannot serialize assertion payload"),
None => JsValue::UNDEFINED,
};

Expand Down
3 changes: 2 additions & 1 deletion noir/noir-repo/compiler/noirc_driver/src/abi_gen.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::BTreeMap;

use acvm::acir::circuit::ErrorSelector;
use acvm::acir::native_types::Witness;
use iter_extended::{btree_map, vecmap};
use noirc_abi::{Abi, AbiErrorType, AbiParameter, AbiReturnType, AbiType, AbiValue};
Expand All @@ -20,7 +21,7 @@ pub(super) fn gen_abi(
input_witnesses: Vec<Witness>,
return_witnesses: Vec<Witness>,
return_visibility: Visibility,
error_types: BTreeMap<u64, Type>,
error_types: BTreeMap<ErrorSelector, Type>,
) -> Abi {
let (parameters, return_type) = compute_function_abi(context, func_id);
let param_witnesses = param_witnesses_from_abi_param(&parameters, input_witnesses);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ impl<'block> BrilligBlock<'block> {
condition,
payload_values,
payload_as_params,
selector.to_u64(),
selector.as_u64(),
);
}
Some(ConstrainError::Intrinsic(message)) => {
Expand Down
12 changes: 4 additions & 8 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ use std::collections::{BTreeMap, BTreeSet};
use crate::errors::{RuntimeError, SsaReport};
use acvm::acir::{
circuit::{
brillig::BrilligBytecode, Circuit, ExpressionWidth, Program as AcirProgram, PublicInputs,
brillig::BrilligBytecode, Circuit, ErrorSelector, ExpressionWidth, Program as AcirProgram,
PublicInputs,
},
native_types::Witness,
};
Expand Down Expand Up @@ -103,13 +104,13 @@ pub struct SsaProgramArtifact {
pub main_input_witnesses: Vec<Witness>,
pub main_return_witnesses: Vec<Witness>,
pub names: Vec<String>,
pub error_types: BTreeMap<u64, HirType>,
pub error_types: BTreeMap<ErrorSelector, HirType>,
}

impl SsaProgramArtifact {
fn new(
unconstrained_functions: Vec<BrilligBytecode>,
error_types: BTreeMap<u64, HirType>,
error_types: BTreeMap<ErrorSelector, HirType>,
) -> Self {
let program = AcirProgram { functions: Vec::default(), unconstrained_functions };
Self {
Expand Down Expand Up @@ -167,11 +168,6 @@ pub fn create_program(
"The generated ACIRs should match the supplied function signatures"
);

let error_types = error_types
.into_iter()
.map(|(error_typ_id, error_typ)| (error_typ_id.to_u64(), error_typ))
.collect();

let mut program_artifact = SsaProgramArtifact::new(generated_brillig, error_types);
// For setting up the ABI we need separately specify main's input and return witnesses
let mut is_main = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use self::acir_ir::generated_acir::BrilligStdlibFunc;
use super::function_builder::data_bus::DataBus;
use super::ir::dfg::CallStack;
use super::ir::function::FunctionId;
use super::ir::instruction::{ConstrainError, ErrorSelector, ErrorType};
use super::ir::instruction::{ConstrainError, ErrorType};
use super::ir::printer::try_to_extract_string_from_error_payload;
use super::{
ir::{
Expand All @@ -32,7 +32,7 @@ pub(crate) use acir_ir::generated_acir::GeneratedAcir;
use noirc_frontend::monomorphization::ast::InlineType;

use acvm::acir::circuit::brillig::BrilligBytecode;
use acvm::acir::circuit::{AssertionPayload, OpcodeLocation};
use acvm::acir::circuit::{AssertionPayload, ErrorSelector, OpcodeLocation};
use acvm::acir::native_types::Witness;
use acvm::acir::BlackBoxFunc;
use acvm::{
Expand Down Expand Up @@ -639,7 +639,7 @@ impl<'a> Context<'a> {
self.acir_context.vars_to_expressions_or_memory(&acir_vars)?;

Some(AssertionPayload::Dynamic(
error_selector.to_u64(),
error_selector.as_u64(),
expressions_or_memory,
))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ pub(crate) mod data_bus;

use std::{borrow::Cow, collections::BTreeMap, rc::Rc};

use acvm::FieldElement;
use acvm::{acir::circuit::ErrorSelector, FieldElement};
use noirc_errors::Location;
use noirc_frontend::monomorphization::ast::InlineType;

use crate::ssa::ir::{
basic_block::BasicBlockId,
function::{Function, FunctionId},
instruction::{Binary, BinaryOp, ErrorSelector, Instruction, TerminatorInstruction},
instruction::{Binary, BinaryOp, Instruction, TerminatorInstruction},
types::Type,
value::{Value, ValueId},
};
Expand Down
32 changes: 13 additions & 19 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use std::hash::{Hash, Hasher};

use acvm::{
acir::{circuit::STRING_ERROR_SELECTOR, BlackBoxFunc},
acir::{
circuit::{ErrorSelector, STRING_ERROR_SELECTOR},
BlackBoxFunc,
},
FieldElement,
};
use fxhash::FxHasher;
Expand Down Expand Up @@ -605,26 +608,17 @@ impl Instruction {

pub(crate) type ErrorType = HirType;

#[derive(Debug, Copy, PartialEq, Eq, Hash, Clone, PartialOrd, Ord)]
pub(crate) struct ErrorSelector(u64);

impl ErrorSelector {
pub(crate) fn new(typ: &ErrorType) -> Self {
match typ {
ErrorType::String(_) => Self(STRING_ERROR_SELECTOR),
_ => {
let mut hasher = FxHasher::default();
typ.hash(&mut hasher);
let hash = hasher.finish();
assert!(hash != 0, "ICE: Error type {} collides with the string error type", typ);
Self(hash)
}
pub(crate) fn error_selector_from_type(typ: &ErrorType) -> ErrorSelector {
match typ {
ErrorType::String(_) => STRING_ERROR_SELECTOR,
_ => {
let mut hasher = FxHasher::default();
typ.hash(&mut hasher);
let hash = hasher.finish();
assert!(hash != 0, "ICE: Error type {} collides with the string error type", typ);
ErrorSelector::new(hash)
}
}

pub(crate) fn to_u64(self) -> u64 {
self.0
}
}

#[derive(Debug, PartialEq, Eq, Hash, Clone)]
Expand Down
Loading
Loading