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!: Do not encode assertion strings in the programs #8315

Merged
merged 7 commits into from
Sep 2, 2024
Merged
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
16 changes: 16 additions & 0 deletions avm-transpiler/Cargo.lock

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

3 changes: 2 additions & 1 deletion avm-transpiler/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ license = "MIT OR Apache-2.0"

[dependencies]
# local
acvm = { path = "../noir/noir-repo/acvm-repo/acvm", features=["bn254"] }
acvm = { path = "../noir/noir-repo/acvm-repo/acvm", features = ["bn254"] }
noirc_errors = { path = "../noir/noir-repo/compiler/noirc_errors" }
fxhash = "0.2.1"

# external
base64 = "0.21"
Expand Down
16 changes: 14 additions & 2 deletions avm-transpiler/src/transpile.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::collections::BTreeMap;

use acvm::acir::brillig::{BitSize, IntegerBitSize, Opcode as BrilligOpcode};
use fxhash::FxHashMap as HashMap;
use std::collections::BTreeMap;

use acvm::acir::circuit::BrilligOpcodeLocation;
use acvm::brillig_vm::brillig::{
Expand Down Expand Up @@ -1100,6 +1100,18 @@ pub fn patch_debug_info_pcs(
debug_infos
}

/// Patch the assert messages with updated PCs since transpilation injects extra
/// opcodes into the bytecode.
pub fn patch_assert_message_pcs(
assert_messages: HashMap<usize, String>,
brillig_pcs_to_avm_pcs: &[usize],
) -> HashMap<usize, String> {
assert_messages
.into_iter()
.map(|(brillig_pc, message)| (brillig_pcs_to_avm_pcs[brillig_pc], message))
.collect()
}

/// Compute an array that maps each Brillig pc to an AVM pc.
/// This must be done before transpiling to properly transpile jump destinations.
/// This is necessary for two reasons:
Expand Down
14 changes: 12 additions & 2 deletions avm-transpiler/src/transpile_contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@ use serde::{Deserialize, Serialize};
use acvm::acir::circuit::Program;
use noirc_errors::debug_info::ProgramDebugInfo;

use crate::transpile::{brillig_to_avm, map_brillig_pcs_to_avm_pcs, patch_debug_info_pcs};
use crate::utils::extract_brillig_from_acir_program;
use crate::transpile::{
brillig_to_avm, map_brillig_pcs_to_avm_pcs, patch_assert_message_pcs, patch_debug_info_pcs,
};
use crate::utils::{extract_brillig_from_acir_program, extract_static_assert_messages};
use fxhash::FxHashMap as HashMap;

/// Representation of a contract with some transpiled functions
#[derive(Debug, Serialize, Deserialize)]
Expand Down Expand Up @@ -49,6 +52,7 @@ pub struct AvmContractFunctionArtifact {
deserialize_with = "ProgramDebugInfo::deserialize_compressed_base64_json"
)]
pub debug_symbols: ProgramDebugInfo,
pub assert_messages: HashMap<usize, String>,
}

/// Representation of an ACIR contract function but with
Expand Down Expand Up @@ -93,10 +97,15 @@ impl From<CompiledAcirContractArtifact> for TranspiledContractArtifact {
// Extract Brillig Opcodes from acir
let acir_program = function.bytecode;
let brillig_bytecode = extract_brillig_from_acir_program(&acir_program);
let assert_messages = extract_static_assert_messages(&acir_program);

// Map Brillig pcs to AVM pcs (index is Brillig PC, value is AVM PC)
let brillig_pcs_to_avm_pcs = map_brillig_pcs_to_avm_pcs(brillig_bytecode);

// Patch the assert messages with updated PCs
let assert_messages =
patch_assert_message_pcs(assert_messages, &brillig_pcs_to_avm_pcs);

// Transpile to AVM
let avm_bytecode = brillig_to_avm(brillig_bytecode, &brillig_pcs_to_avm_pcs);

Expand Down Expand Up @@ -130,6 +139,7 @@ impl From<CompiledAcirContractArtifact> for TranspiledContractArtifact {
abi: function.abi,
bytecode: base64::prelude::BASE64_STANDARD.encode(compressed_avm_bytecode),
debug_symbols: ProgramDebugInfo { debug_infos },
assert_messages,
},
));
} else {
Expand Down
31 changes: 30 additions & 1 deletion avm-transpiler/src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use fxhash::FxHashMap as HashMap;

use acvm::acir::circuit::brillig::BrilligFunctionId;
use acvm::FieldElement;
use log::debug;

use acvm::acir::brillig::Opcode as BrilligOpcode;
use acvm::acir::circuit::{Opcode, Program};
use acvm::acir::circuit::{AssertionPayload, Opcode, Program};

use crate::instructions::AvmInstruction;

Expand Down Expand Up @@ -36,6 +38,33 @@ pub fn extract_brillig_from_acir_program(
&program.unconstrained_functions[0].bytecode
}

/// Assertion messages that are static strings are stored in the assert_messages map of the ACIR program.
pub fn extract_static_assert_messages(program: &Program<FieldElement>) -> HashMap<usize, String> {
assert_eq!(
program.functions.len(),
1,
"An AVM program should have only a single ACIR function with a 'BrilligCall'"
);
let main_function = &program.functions[0];
main_function
.assert_messages
.iter()
.filter_map(|(location, payload)| {
if let AssertionPayload::StaticString(static_string) = payload {
Some((
location
.to_brillig_location()
.expect("Assert message is not for the brillig function")
.0,
static_string.clone(),
))
} else {
None
}
})
.collect()
}

/// Print inputs, outputs, and instructions in a Brillig program
pub fn dbg_print_brillig_program(brillig_bytecode: &[BrilligOpcode<FieldElement>]) {
debug!("Printing Brillig program...");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ impl<'block> BrilligBlock<'block> {
condition,
);
match assert_message {
Some(ConstrainError::UserDefined(selector, values)) => {
Some(ConstrainError::Dynamic(selector, values)) => {
let payload_values =
vecmap(values, |value| self.convert_ssa_value(*value, dfg));
let payload_as_params = vecmap(values, |value| {
Expand All @@ -280,7 +280,7 @@ impl<'block> BrilligBlock<'block> {
selector.as_u64(),
);
}
Some(ConstrainError::Intrinsic(message)) => {
Some(ConstrainError::StaticString(message)) => {
self.brillig_context.codegen_constrain(condition, Some(message.clone()));
}
None => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -647,10 +647,10 @@ impl<'a> Context<'a> {

let assert_payload = if let Some(error) = assert_message {
match error {
ConstrainError::Intrinsic(string) => {
ConstrainError::StaticString(string) => {
Some(AssertionPayload::StaticString(string.clone()))
}
ConstrainError::UserDefined(error_selector, values) => {
ConstrainError::Dynamic(error_selector, values) => {
if let Some(constant_string) = try_to_extract_string_from_error_payload(
*error_selector,
values,
Expand Down
22 changes: 10 additions & 12 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,10 @@ impl Instruction {
let lhs = f(*lhs);
let rhs = f(*rhs);
let assert_message = assert_message.as_ref().map(|error| match error {
ConstrainError::UserDefined(selector, payload_values) => {
ConstrainError::UserDefined(
*selector,
payload_values.iter().map(|&value| f(value)).collect(),
)
}
ConstrainError::Dynamic(selector, payload_values) => ConstrainError::Dynamic(
*selector,
payload_values.iter().map(|&value| f(value)).collect(),
),
_ => error.clone(),
});
Instruction::Constrain(lhs, rhs, assert_message)
Expand Down Expand Up @@ -541,7 +539,7 @@ impl Instruction {
Instruction::Constrain(lhs, rhs, assert_error) => {
f(*lhs);
f(*rhs);
if let Some(ConstrainError::UserDefined(_, values)) = assert_error.as_ref() {
if let Some(ConstrainError::Dynamic(_, values)) = assert_error.as_ref() {
values.iter().for_each(|&val| {
f(val);
});
Expand Down Expand Up @@ -836,15 +834,15 @@ pub(crate) fn error_selector_from_type(typ: &ErrorType) -> ErrorSelector {

#[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)]
pub(crate) enum ConstrainError {
// These are errors which have been hardcoded during SSA gen
Intrinsic(String),
// These are errors issued by the user
UserDefined(ErrorSelector, Vec<ValueId>),
// Static string errors are not handled inside the program as data for efficiency reasons.
StaticString(String),
// These errors are handled by the program as data.
Dynamic(ErrorSelector, Vec<ValueId>),
}

impl From<String> for ConstrainError {
fn from(value: String) -> Self {
ConstrainError::Intrinsic(value)
ConstrainError::StaticString(value)
}
}

Expand Down
4 changes: 2 additions & 2 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,10 @@ fn display_constrain_error(
f: &mut Formatter,
) -> Result {
match error {
ConstrainError::Intrinsic(assert_message_string) => {
ConstrainError::StaticString(assert_message_string) => {
writeln!(f, " '{assert_message_string:?}'")
}
ConstrainError::UserDefined(selector, values) => {
ConstrainError::Dynamic(selector, values) => {
if let Some(constant_string) =
try_to_extract_string_from_error_payload(*selector, values, &function.dfg)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,11 @@ impl<'a> FunctionContext<'a> {
assert_message: &Option<Box<(Expression, HirType)>>,
) -> Result<Option<ConstrainError>, RuntimeError> {
let Some(assert_message_payload) = assert_message else { return Ok(None) };

if let Expression::Literal(ast::Literal::Str(static_string)) = &assert_message_payload.0 {
return Ok(Some(ConstrainError::StaticString(static_string.clone())));
}

let (assert_message_expression, assert_message_typ) = assert_message_payload.as_ref();

let values = self.codegen_expression(assert_message_expression)?.into_value_list(self);
Expand All @@ -713,7 +718,7 @@ impl<'a> FunctionContext<'a> {
self.builder.record_error_type(error_type_id, assert_message_typ.clone());
}
};
Ok(Some(ConstrainError::UserDefined(error_type_id, values)))
Ok(Some(ConstrainError::Dynamic(error_type_id, values)))
}

fn codegen_assign(&mut self, assign: &ast::Assign) -> Result<Values, RuntimeError> {
Expand Down
4 changes: 3 additions & 1 deletion yarn-project/bb-prover/src/avm_proving.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
initExecutionEnvironment,
initHostStorage,
initPersistableStateManager,
resolveAvmTestContractAssertionMessage,
} from '@aztec/simulator/avm/fixtures';

import { jest } from '@jest/globals';
Expand Down Expand Up @@ -256,7 +257,8 @@ const proveAndVerifyAvmTestContract = async (
} else {
// Explicit revert when an assertion failed.
expect(avmResult.reverted).toBe(true);
expect(avmResult.revertReason?.message).toContain(assertionErrString);
expect(avmResult.revertReason).toBeDefined();
expect(resolveAvmTestContractAssertionMessage(functionName, avmResult.revertReason!)).toContain(assertionErrString);
}

const pxResult = trace.toPublicExecutionResult(
Expand Down
8 changes: 8 additions & 0 deletions yarn-project/circuit-types/src/simulation_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,14 @@ export class SimulationError extends Error {
return this.originalMessage;
}

getOriginalMessage() {
return this.originalMessage;
}

setOriginalMessage(message: string) {
this.originalMessage = message;
}

/**
* Enriches the error with the name of a contract that failed.
* @param contractAddress - The address of the contract
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/fixtures/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const privateKey2 = Buffer.from('59c6995e998f97a5a0044966f0945389dc9e86da
/// Common errors
export const U128_UNDERFLOW_ERROR = "Assertion failed: attempt to subtract with underflow 'hi == high'";
export const U128_OVERFLOW_ERROR = "Assertion failed: attempt to add with overflow 'hi == high'";
export const BITSIZE_TOO_BIG_ERROR = "Assertion failed. 'self.__assert_max_bit_size'";
export const BITSIZE_TOO_BIG_ERROR = "Assertion failed: call to assert_max_bit_size 'self.__assert_max_bit_size'";
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/5818): Make these a fixed error after transition.
export const DUPLICATE_NULLIFIER_ERROR = /dropped|duplicate nullifier|reverted/;
export const NO_L1_TO_L2_MSG_ERROR =
Expand Down
14 changes: 13 additions & 1 deletion yarn-project/foundation/src/abi/abi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ export interface FunctionArtifact extends FunctionAbi {
verificationKey?: string;
/** Maps opcodes to source code pointers */
debugSymbols: string;
/**
* Public functions store their static assertion messages externally to the bytecode.
*/
assertMessages?: Record<number, string>;
/** Debug metadata for the function. */
debug?: FunctionDebugMetadata;
}
Expand Down Expand Up @@ -352,6 +356,10 @@ export interface FunctionDebugMetadata {
* Maps the file IDs to the file contents to resolve pointers
*/
files: DebugFileMap;
/**
* Public functions store their static assertion messages externally to the bytecode.
*/
assertMessages?: Record<number, string>;
}

/**
Expand Down Expand Up @@ -390,7 +398,11 @@ export function getFunctionDebugMetadata(
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/5813)
// We only support handling debug info for the contract function entry point.
// So for now we simply index into the first debug info.
return { debugSymbols: programDebugSymbols.debug_infos[0], files: contractArtifact.fileMap };
return {
debugSymbols: programDebugSymbols.debug_infos[0],
files: contractArtifact.fileMap,
assertMessages: functionArtifact.assertMessages,
};
}
return undefined;
}
Expand Down
Loading
Loading