From e02f0e4a748b6500b57dcb40d974709eb0c028e1 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 12 Apr 2024 13:41:50 +0000 Subject: [PATCH 01/12] feat: trap with data --- .../dsl/acir_format/serde/acir.hpp | 16 +++++++- .../noir-repo/acvm-repo/acir/codegen/acir.cpp | 9 +++++ .../acvm-repo/acvm/src/pwg/brillig.rs | 26 ++++++++++++- noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs | 4 +- noir/noir-repo/acvm-repo/acvm/tests/solver.rs | 4 +- .../acvm-repo/acvm_js/src/execute.rs | 15 +++----- .../acvm-repo/brillig/src/opcodes.rs | 7 +++- .../noir-repo/acvm-repo/brillig_vm/src/lib.rs | 37 +++++++++++++++---- .../src/brillig/brillig_gen/brillig_block.rs | 16 ++++---- .../brillig/brillig_gen/brillig_directive.rs | 3 -- .../noirc_evaluator/src/brillig/brillig_ir.rs | 2 +- .../src/brillig/brillig_ir/artifact.rs | 18 +-------- .../brillig_ir/codegen_control_flow.rs | 27 ++++++++++++++ .../src/brillig/brillig_ir/debug_show.rs | 12 ++++-- .../src/brillig/brillig_ir/instructions.rs | 29 +++------------ .../ssa/acir_gen/acir_ir/generated_acir.rs | 6 --- noir/noir-repo/tooling/nargo/src/errors.rs | 4 +- .../tooling/nargo/src/ops/execute.rs | 10 ++++- 18 files changed, 153 insertions(+), 92 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp index 3e01fd1f1559..5607b84ca7a8 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp @@ -966,6 +966,9 @@ struct BrilligOpcode { }; struct Trap { + uint64_t revert_data_offset; + uint64_t revert_data_size; + friend bool operator==(const Trap&, const Trap&); std::vector bincodeSerialize() const; static Trap bincodeDeserialize(std::vector); @@ -5990,6 +5993,12 @@ namespace Program { inline bool operator==(const BrilligOpcode::Trap& lhs, const BrilligOpcode::Trap& rhs) { + if (!(lhs.revert_data_offset == rhs.revert_data_offset)) { + return false; + } + if (!(lhs.revert_data_size == rhs.revert_data_size)) { + return false; + } return true; } @@ -6016,7 +6025,10 @@ template <> template void serde::Serializable::serialize(const Program::BrilligOpcode::Trap& obj, Serializer& serializer) -{} +{ + serde::Serializable::serialize(obj.revert_data_offset, serializer); + serde::Serializable::serialize(obj.revert_data_size, serializer); +} template <> template @@ -6024,6 +6036,8 @@ Program::BrilligOpcode::Trap serde::Deserializable Deserializer& deserializer) { Program::BrilligOpcode::Trap obj; + obj.revert_data_offset = serde::Deserializable::deserialize(deserializer); + obj.revert_data_size = serde::Deserializable::deserialize(deserializer); return obj; } diff --git a/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp b/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp index 7cd9fbefba09..223c4e3a468c 100644 --- a/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp +++ b/noir/noir-repo/acvm-repo/acir/codegen/acir.cpp @@ -922,6 +922,9 @@ namespace Program { }; struct Trap { + uint64_t revert_data_offset; + uint64_t revert_data_size; + friend bool operator==(const Trap&, const Trap&); std::vector bincodeSerialize() const; static Trap bincodeDeserialize(std::vector); @@ -4952,6 +4955,8 @@ Program::BrilligOpcode::BlackBox serde::Deserializable template void serde::Serializable::serialize(const Program::BrilligOpcode::Trap &obj, Serializer &serializer) { + serde::Serializable::serialize(obj.revert_data_offset, serializer); + serde::Serializable::serialize(obj.revert_data_size, serializer); } template <> template Program::BrilligOpcode::Trap serde::Deserializable::deserialize(Deserializer &deserializer) { Program::BrilligOpcode::Trap obj; + obj.revert_data_offset = serde::Deserializable::deserialize(deserializer); + obj.revert_data_size = serde::Deserializable::deserialize(deserializer); return obj; } diff --git a/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs b/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs index 81e752d5656e..87d51dd7e023 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs @@ -11,7 +11,7 @@ use acir::{ FieldElement, }; use acvm_blackbox_solver::BlackBoxFunctionSolver; -use brillig_vm::{MemoryValue, VMStatus, VM}; +use brillig_vm::{FailureReason, MemoryValue, VMStatus, VM}; use crate::{pwg::OpcodeNotSolvable, OpcodeResolutionError}; @@ -159,7 +159,29 @@ impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> { match vm_status { VMStatus::Finished { .. } => Ok(BrilligSolverStatus::Finished), VMStatus::InProgress => Ok(BrilligSolverStatus::InProgress), - VMStatus::Failure { message, call_stack } => { + VMStatus::Failure { reason, call_stack } => { + let message = match reason { + FailureReason::RuntimeError { message } => Some(message), + 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 { + None + } else { + let memory = self.vm.get_memory(); + let bytes = memory + [revert_data_offset..(revert_data_offset + revert_data_size)] + .iter() + .map(|x| { + x.try_into().expect("Assert message character is not a byte") + }) + .collect(); + Some( + String::from_utf8(bytes) + .expect("Assert message is not valid UTF-8"), + ) + } + } + }; Err(OpcodeResolutionError::BrilligFunctionFailed { message, call_stack: call_stack diff --git a/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs b/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs index bb98eda2689b..6864256f14f1 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs @@ -122,8 +122,8 @@ pub enum OpcodeResolutionError { IndexOutOfBounds { opcode_location: ErrorLocation, index: u32, array_size: u32 }, #[error("Failed to solve blackbox function: {0}, reason: {1}")] BlackBoxFunctionFailed(BlackBoxFunc, String), - #[error("Failed to solve brillig function, reason: {message}")] - BrilligFunctionFailed { message: String, call_stack: Vec }, + #[error("Failed to solve brillig function{}", .message.as_ref().map(|m| format!(", reason: {}", m)).unwrap_or_default())] + BrilligFunctionFailed { message: Option, call_stack: Vec }, #[error("Attempted to call `main` with a `Call` opcode")] AcirMainCallAttempted { opcode_location: ErrorLocation }, #[error("{results_size:?} result values were provided for {outputs_size:?} call output witnesses, most likely due to bad ACIR codegen")] diff --git a/noir/noir-repo/acvm-repo/acvm/tests/solver.rs b/noir/noir-repo/acvm-repo/acvm/tests/solver.rs index a708db5b0300..ec7a6467ff5d 100644 --- a/noir/noir-repo/acvm-repo/acvm/tests/solver.rs +++ b/noir/noir-repo/acvm-repo/acvm/tests/solver.rs @@ -549,7 +549,7 @@ fn unsatisfied_opcode_resolved_brillig() { let jmp_if_opcode = BrilligOpcode::JumpIf { condition: MemoryAddress::from(2), location: location_of_stop }; - let trap_opcode = BrilligOpcode::Trap; + let trap_opcode = BrilligOpcode::Trap { revert_data_offset: 0, revert_data_size: 0 }; let stop_opcode = BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }; let brillig_opcode = Opcode::Brillig(Brillig { @@ -597,7 +597,7 @@ fn unsatisfied_opcode_resolved_brillig() { assert_eq!( solver_status, ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed { - message: "explicit trap hit in brillig".to_string(), + message: None, call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }] }), "The first opcode is not satisfiable, expected an error indicating this" diff --git a/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs b/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs index c97b8ea1a665..9e7d34f80124 100644 --- a/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs +++ b/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs @@ -231,7 +231,7 @@ impl<'a, B: BlackBoxFunctionSolver> ProgramExecutor<'a, B> { unreachable!("Execution should not stop while in `InProgress` state.") } ACVMStatus::Failure(error) => { - let (assert_message, call_stack) = match &error { + let (assert_message, call_stack): (Option<&str>, _) = match &error { OpcodeResolutionError::UnsatisfiedConstrain { opcode_location: ErrorLocation::Resolved(opcode_location), } @@ -242,15 +242,10 @@ impl<'a, B: BlackBoxFunctionSolver> ProgramExecutor<'a, B> { circuit.get_assert_message(*opcode_location), Some(vec![*opcode_location]), ), - OpcodeResolutionError::BrilligFunctionFailed { call_stack, .. } => { - let failing_opcode = call_stack - .last() - .expect("Brillig error call stacks cannot be empty"); - ( - circuit.get_assert_message(*failing_opcode), - Some(call_stack.clone()), - ) - } + OpcodeResolutionError::BrilligFunctionFailed { + call_stack, + message, + } => (message.as_ref().map(|x| x.as_str()), Some(call_stack.clone())), _ => (None, None), }; diff --git a/noir/noir-repo/acvm-repo/brillig/src/opcodes.rs b/noir/noir-repo/acvm-repo/brillig/src/opcodes.rs index d13453519862..468fd88db45d 100644 --- a/noir/noir-repo/acvm-repo/brillig/src/opcodes.rs +++ b/noir/noir-repo/acvm-repo/brillig/src/opcodes.rs @@ -177,8 +177,11 @@ pub enum BrilligOpcode { source: MemoryAddress, }, BlackBox(BlackBoxOp), - /// Used to denote execution failure - Trap, + /// Used to denote execution failure, returning data after the offset + Trap { + revert_data_offset: usize, + revert_data_size: usize, + }, /// Stop execution, returning data after the offset Stop { return_data_offset: usize, diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs index 26d5da675769..d83870dc410a 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/lib.rs @@ -32,6 +32,12 @@ mod memory; /// The error call stack contains the opcode indexes of the call stack at the time of failure, plus the index of the opcode that failed. pub type ErrorCallStack = Vec; +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum FailureReason { + Trap { revert_data_offset: usize, revert_data_size: usize }, + RuntimeError { message: String }, +} + #[derive(Debug, PartialEq, Eq, Clone)] pub enum VMStatus { Finished { @@ -40,7 +46,7 @@ pub enum VMStatus { }, InProgress, Failure { - message: String, + reason: FailureReason, call_stack: ErrorCallStack, }, /// The VM process is not solvable as a [foreign call][Opcode::ForeignCall] has been @@ -138,13 +144,28 @@ impl<'a, B: BlackBoxFunctionSolver> VM<'a, B> { self.status(VMStatus::InProgress); } + fn get_error_stack(&self) -> Vec { + let mut error_stack: Vec<_> = self.call_stack.clone(); + error_stack.push(self.program_counter); + error_stack + } + /// Sets the current status of the VM to `fail`. /// Indicating that the VM encountered a `Trap` Opcode /// or an invalid state. + fn trap(&mut self, revert_data_offset: usize, revert_data_size: usize) -> VMStatus { + self.status(VMStatus::Failure { + call_stack: self.get_error_stack(), + reason: FailureReason::Trap { revert_data_offset, revert_data_size }, + }); + self.status.clone() + } + fn fail(&mut self, message: String) -> VMStatus { - let mut error_stack: Vec<_> = self.call_stack.clone(); - error_stack.push(self.program_counter); - self.status(VMStatus::Failure { call_stack: error_stack, message }); + self.status(VMStatus::Failure { + call_stack: self.get_error_stack(), + reason: FailureReason::RuntimeError { message }, + }); self.status.clone() } @@ -281,7 +302,9 @@ impl<'a, B: BlackBoxFunctionSolver> VM<'a, B> { } self.increment_program_counter() } - Opcode::Trap => self.fail("explicit trap hit in brillig".to_string()), + Opcode::Trap { revert_data_offset, revert_data_size } => { + self.trap(*revert_data_offset, *revert_data_size) + } Opcode::Stop { return_data_offset, return_data_size } => { self.finish(*return_data_offset, *return_data_size) } @@ -684,7 +707,7 @@ mod tests { let jump_opcode = Opcode::Jump { location: 3 }; - let trap_opcode = Opcode::Trap; + let trap_opcode = Opcode::Trap { revert_data_offset: 0, revert_data_size: 0 }; let not_equal_cmp_opcode = Opcode::BinaryFieldOp { op: BinaryFieldOp::Equals, @@ -731,7 +754,7 @@ mod tests { assert_eq!( status, VMStatus::Failure { - message: "explicit trap hit in brillig".to_string(), + reason: FailureReason::Trap { revert_data_offset: 0, revert_data_size: 0 }, call_stack: vec![2] } ); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index cf2501ab1c03..350cff1f1dda 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -282,7 +282,7 @@ impl<'block> BrilligBlock<'block> { condition, ); - self.brillig_context.constrain_instruction(condition, assert_message); + self.brillig_context.codegen_constrain(condition, assert_message); self.brillig_context.deallocate_single_addr(condition); } Instruction::Allocate => { @@ -670,7 +670,7 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.constrain_instruction(condition, assert_message.clone()); + self.brillig_context.codegen_constrain(condition, assert_message.clone()); self.brillig_context.deallocate_single_addr(condition); self.brillig_context.deallocate_single_addr(left); self.brillig_context.deallocate_single_addr(right); @@ -802,7 +802,7 @@ impl<'block> BrilligBlock<'block> { ); self.brillig_context - .constrain_instruction(condition, Some("Array index out of bounds".to_owned())); + .codegen_constrain(condition, Some("Array index out of bounds".to_owned())); if should_deallocate_size { self.brillig_context.deallocate_single_addr(size_as_register); @@ -1503,10 +1503,8 @@ impl<'block> BrilligBlock<'block> { condition, BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.constrain_instruction( - condition, - Some("attempt to add with overflow".to_string()), - ); + self.brillig_context + .codegen_constrain(condition, Some("attempt to add with overflow".to_string())); self.brillig_context.deallocate_single_addr(condition); } (BrilligBinaryOp::Sub, false) => { @@ -1519,7 +1517,7 @@ impl<'block> BrilligBlock<'block> { condition, BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.constrain_instruction( + self.brillig_context.codegen_constrain( condition, Some("attempt to subtract with overflow".to_string()), ); @@ -1549,7 +1547,7 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::UnsignedDiv, ); ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); - ctx.constrain_instruction( + ctx.codegen_constrain( condition, Some("attempt to multiply with overflow".to_string()), ); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs index 4b97a61491d0..03990e398955 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs @@ -53,7 +53,6 @@ pub(crate) fn directive_invert() -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 1 }, ], - assert_messages: Default::default(), locations: Default::default(), } } @@ -111,7 +110,6 @@ pub(crate) fn directive_quotient(bit_size: u32) -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 2 }, ], - assert_messages: Default::default(), locations: Default::default(), } } else { @@ -164,7 +162,6 @@ pub(crate) fn directive_quotient(bit_size: u32) -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 2 }, ], - assert_messages: Default::default(), locations: Default::default(), } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index e5c731be6798..fdb03abe59f1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -262,7 +262,7 @@ pub(crate) mod tests { // uses unresolved jumps which requires a block to be constructed in SSA and // we don't need this for Brillig IR tests context.push_opcode(BrilligOpcode::JumpIf { condition: r_equality, location: 8 }); - context.push_opcode(BrilligOpcode::Trap); + context.push_opcode(BrilligOpcode::Trap { revert_data_offset: 0, revert_data_size: 0 }); context.stop_instruction(); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 8ce15ba4e73f..b5bf4d45e7e3 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -21,7 +21,6 @@ pub(crate) enum BrilligParameter { pub(crate) struct GeneratedBrillig { pub(crate) byte_code: Vec, pub(crate) locations: BTreeMap, - pub(crate) assert_messages: BTreeMap, } #[derive(Default, Debug, Clone)] @@ -29,8 +28,6 @@ pub(crate) struct GeneratedBrillig { /// It includes the bytecode of the function and all the metadata that allows linking with other functions. pub(crate) struct BrilligArtifact { pub(crate) byte_code: Vec, - /// A map of bytecode positions to assertion messages - pub(crate) assert_messages: BTreeMap, /// The set of jumps that need to have their locations /// resolved. unresolved_jumps: Vec<(JumpInstructionPosition, UnresolvedJumpLocation)>, @@ -75,11 +72,7 @@ impl BrilligArtifact { /// Resolves all jumps and generates the final bytecode pub(crate) fn finish(mut self) -> GeneratedBrillig { self.resolve_jumps(); - GeneratedBrillig { - byte_code: self.byte_code, - locations: self.locations, - assert_messages: self.assert_messages, - } + GeneratedBrillig { byte_code: self.byte_code, locations: self.locations } } /// Gets the first unresolved function call of this artifact. @@ -142,10 +135,6 @@ impl BrilligArtifact { .push((position_in_bytecode + offset, label_id.clone())); } - for (position_in_bytecode, message) in &obj.assert_messages { - self.assert_messages.insert(position_in_bytecode + offset, message.clone()); - } - for (position_in_bytecode, call_stack) in obj.locations.iter() { self.locations.insert(position_in_bytecode + offset, call_stack.clone()); } @@ -257,9 +246,4 @@ impl BrilligArtifact { pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) { self.call_stack = call_stack; } - - pub(crate) fn add_assert_message_to_last_opcode(&mut self, message: String) { - let position = self.index_of_next_opcode() - 1; - self.assert_messages.insert(position, message); - } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs index 116eaa5103fb..8e9d791212e5 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs @@ -138,4 +138,31 @@ impl BrilligContext { self.enter_section(end_section); } + + /// Emits brillig bytecode to jump to a trap condition if `condition` + /// is false. + pub(crate) fn codegen_constrain( + &mut self, + condition: SingleAddrVariable, + assert_message: Option, + ) { + assert!(condition.bit_size == 1); + + self.codegen_if_not(condition.address, |ctx| { + let (revert_data_offset, revert_data_size) = + if let Some(assert_message) = assert_message { + let bytes = assert_message.as_bytes(); + for (i, byte) in bytes.iter().enumerate() { + ctx.const_instruction( + SingleAddrVariable::new(MemoryAddress(i), 8), + (*byte as usize).into(), + ); + } + (0, bytes.len()) + } else { + (0, 0) + }; + ctx.trap_instruction(revert_data_offset, revert_data_size); + }); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index 4ca1144b6a49..41a6d1873e48 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -113,10 +113,14 @@ impl DebugShow { DebugShow { enable_debug_trace } } - /// Emits brillig bytecode to jump to a trap condition if `condition` - /// is false. - pub(crate) fn constrain_instruction(&self, condition: MemoryAddress) { - debug_println!(self.enable_debug_trace, " ASSERT {} != 0", condition); + /// Emits a `trap` instruction. + pub(crate) fn trap_instruction(&self, revert_data_offset: usize, revert_data_size: usize) { + debug_println!( + self.enable_debug_trace, + " TRAP {}..{}", + revert_data_offset, + revert_data_offset + revert_data_size + ); } /// Emits a `mov` instruction. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs index f305eb81b01e..901ccc58036f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs @@ -215,29 +215,6 @@ impl BrilligContext { ); } - /// Emits brillig bytecode to jump to a trap condition if `condition` - /// is false. - pub(crate) fn constrain_instruction( - &mut self, - condition: SingleAddrVariable, - assert_message: Option, - ) { - self.debug_show.constrain_instruction(condition.address); - - assert!(condition.bit_size == 1); - - let (next_section, next_label) = self.reserve_next_section_label(); - self.add_unresolved_jump( - BrilligOpcode::JumpIf { condition: condition.address, location: 0 }, - next_label, - ); - self.push_opcode(BrilligOpcode::Trap); - if let Some(assert_message) = assert_message { - self.obj.add_assert_message_to_last_opcode(assert_message); - } - self.enter_section(next_section); - } - /// Adds a unresolved `Jump` to the bytecode. fn add_unresolved_jump( &mut self, @@ -488,6 +465,12 @@ impl BrilligContext { offset, }); } + + pub(super) fn trap_instruction(&mut self, revert_data_offset: usize, revert_data_size: usize) { + self.debug_show.trap_instruction(revert_data_offset, revert_data_size); + + self.push_opcode(BrilligOpcode::Trap { revert_data_offset, revert_data_size }); + } } /// Type to encapsulate the binary operation types in Brillig diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index ba4e03bff955..fa3ffb4052b4 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -559,12 +559,6 @@ impl GeneratedAcir { call_stack, ); } - for (brillig_index, message) in generated_brillig.assert_messages { - self.assert_messages.insert( - OpcodeLocation::Brillig { acir_index: self.opcodes.len() - 1, brillig_index }, - message, - ); - } } pub(crate) fn last_acir_opcode_location(&self) -> OpcodeLocation { diff --git a/noir/noir-repo/tooling/nargo/src/errors.rs b/noir/noir-repo/tooling/nargo/src/errors.rs index ff238d79a461..f1e77d47a73a 100644 --- a/noir/noir-repo/tooling/nargo/src/errors.rs +++ b/noir/noir-repo/tooling/nargo/src/errors.rs @@ -67,7 +67,9 @@ impl NargoError { | OpcodeResolutionError::UnsatisfiedConstrain { .. } | OpcodeResolutionError::AcirMainCallAttempted { .. } | OpcodeResolutionError::AcirCallOutputsMismatch { .. } => None, - OpcodeResolutionError::BrilligFunctionFailed { message, .. } => Some(message), + OpcodeResolutionError::BrilligFunctionFailed { message, .. } => { + message.as_ref().map(|s| s.as_str()) + } OpcodeResolutionError::BlackBoxFunctionFailed(_, reason) => Some(reason), }, } diff --git a/noir/noir-repo/tooling/nargo/src/ops/execute.rs b/noir/noir-repo/tooling/nargo/src/ops/execute.rs index 6d328d65119b..5ee0c6a3891c 100644 --- a/noir/noir-repo/tooling/nargo/src/ops/execute.rs +++ b/noir/noir-repo/tooling/nargo/src/ops/execute.rs @@ -70,14 +70,20 @@ impl<'a, B: BlackBoxFunctionSolver, F: ForeignCallExecutor> ProgramExecutor<'a, return Err(NargoError::ExecutionError(match call_stack { Some(call_stack) => { // First check whether we have a runtime assertion message that should be resolved on an ACVM failure - // If we do not have a runtime assertion message, we should check whether the circuit has any hardcoded - // messages associated with a specific `OpcodeLocation`. + // If we do not have a runtime assertion message, we check wether the error is a brillig error with a user-defined message, + // and finally we should check whether the circuit has any hardcoded messages associated with a specific `OpcodeLocation`. // Otherwise return the provided opcode resolution error. if let Some(assert_message) = assert_message { ExecutionError::AssertionFailed( assert_message.to_owned(), call_stack, ) + } else if let OpcodeResolutionError::BrilligFunctionFailed { + message: Some(message), + .. + } = &error + { + ExecutionError::AssertionFailed(message.to_owned(), call_stack) } else if let Some(assert_message) = circuit.get_assert_message( *call_stack.last().expect("Call stacks should not be empty"), ) { From 3a0efcbfd671042b3fd9a3536c1515d6cf4397a1 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 12 Apr 2024 13:55:02 +0000 Subject: [PATCH 02/12] chore: update transpiler --- avm-transpiler/src/transpile.rs | 121 ++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 29 deletions(-) diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs index 9a1c5d9ae916..15eaebaf51ba 100644 --- a/avm-transpiler/src/transpile.rs +++ b/avm-transpiler/src/transpile.rs @@ -70,7 +70,11 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { lhs, rhs, } => { - assert!(is_integral_bit_size(*bit_size), "BinaryIntOp bit size should be integral: {:?}", brillig_instr); + assert!( + is_integral_bit_size(*bit_size), + "BinaryIntOp bit size should be integral: {:?}", + brillig_instr + ); let avm_opcode = match op { BinaryIntOp::Add => AvmOpcode::ADD, BinaryIntOp::Sub => AvmOpcode::SUB, @@ -102,20 +106,27 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { ], }); } - BrilligOpcode::CalldataCopy { destination_address, size, offset } => { + BrilligOpcode::CalldataCopy { + destination_address, + size, + offset, + } => { avm_instrs.push(AvmInstruction { opcode: AvmOpcode::CALLDATACOPY, indirect: Some(ALL_DIRECT), operands: vec![ AvmOperand::U32 { value: *offset as u32, // cdOffset (calldata offset) - }, AvmOperand::U32 { + }, + AvmOperand::U32 { value: *size as u32, - }, AvmOperand::U32 { + }, + AvmOperand::U32 { value: destination_address.to_usize() as u32, // dstOffset - }], - ..Default::default() - }); + }, + ], + ..Default::default() + }); } BrilligOpcode::Jump { location } => { let avm_loc = brillig_pcs_to_avm_pcs[*location]; @@ -146,14 +157,22 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { ..Default::default() }); } - BrilligOpcode::Const { destination, value, bit_size } => { + BrilligOpcode::Const { + destination, + value, + bit_size, + } => { handle_const(&mut avm_instrs, destination, value, bit_size); } BrilligOpcode::Mov { destination, source, } => { - avm_instrs.push(generate_mov_instruction(Some(ALL_DIRECT), source.to_usize() as u32, destination.to_usize() as u32)); + avm_instrs.push(generate_mov_instruction( + Some(ALL_DIRECT), + source.to_usize() as u32, + destination.to_usize() as u32, + )); } BrilligOpcode::ConditionalMov { source_a, @@ -165,10 +184,18 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { opcode: AvmOpcode::CMOV, indirect: Some(ALL_DIRECT), operands: vec![ - AvmOperand::U32 { value: source_a.to_usize() as u32 }, - AvmOperand::U32 { value: source_b.to_usize() as u32 }, - AvmOperand::U32 { value: condition.to_usize() as u32 }, - AvmOperand::U32 { value: destination.to_usize() as u32 }, + AvmOperand::U32 { + value: source_a.to_usize() as u32, + }, + AvmOperand::U32 { + value: source_b.to_usize() as u32, + }, + AvmOperand::U32 { + value: condition.to_usize() as u32, + }, + AvmOperand::U32 { + value: destination.to_usize() as u32, + }, ], ..Default::default() }); @@ -177,13 +204,21 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { destination, source_pointer, } => { - avm_instrs.push(generate_mov_instruction(Some(ZEROTH_OPERAND_INDIRECT), source_pointer.to_usize() as u32, destination.to_usize() as u32)); + avm_instrs.push(generate_mov_instruction( + Some(ZEROTH_OPERAND_INDIRECT), + source_pointer.to_usize() as u32, + destination.to_usize() as u32, + )); } BrilligOpcode::Store { destination_pointer, source, } => { - avm_instrs.push(generate_mov_instruction(Some(FIRST_OPERAND_INDIRECT), source.to_usize() as u32, destination_pointer.to_usize() as u32)); + avm_instrs.push(generate_mov_instruction( + Some(FIRST_OPERAND_INDIRECT), + source.to_usize() as u32, + destination_pointer.to_usize() as u32, + )); } BrilligOpcode::Call { location } => { let avm_loc = brillig_pcs_to_avm_pcs[*location]; @@ -199,38 +234,66 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { opcode: AvmOpcode::INTERNALRETURN, ..Default::default() }), - BrilligOpcode::Stop { return_data_offset, return_data_size } => { + BrilligOpcode::Stop { + return_data_offset, + return_data_size, + } => { avm_instrs.push(AvmInstruction { opcode: AvmOpcode::RETURN, indirect: Some(ALL_DIRECT), operands: vec![ - AvmOperand::U32 { value: *return_data_offset as u32 }, - AvmOperand::U32 { value: *return_data_size as u32 }, + AvmOperand::U32 { + value: *return_data_offset as u32, + }, + AvmOperand::U32 { + value: *return_data_size as u32, + }, ], ..Default::default() }); } - BrilligOpcode::Trap { /*return_data_offset, return_data_size*/ } => { + BrilligOpcode::Trap { + revert_data_offset, + revert_data_size, + } => { // TODO(https://github.com/noir-lang/noir/issues/3113): Trap should support return data avm_instrs.push(AvmInstruction { opcode: AvmOpcode::REVERT, indirect: Some(ALL_DIRECT), operands: vec![ - //AvmOperand::U32 { value: *return_data_offset as u32}, - //AvmOperand::U32 { value: *return_data_size as u32}, - AvmOperand::U32 { value: 0 }, - AvmOperand::U32 { value: 0 }, + AvmOperand::U32 { + value: *revert_data_offset as u32, + }, + AvmOperand::U32 { + value: *revert_data_size as u32, + }, ], ..Default::default() }); - }, - BrilligOpcode::Cast { destination, source, bit_size } => { - avm_instrs.push(generate_cast_instruction(source.to_usize() as u32, destination.to_usize() as u32, tag_from_bit_size(*bit_size))); } - BrilligOpcode::ForeignCall { function, destinations, inputs, destination_value_types:_, input_value_types:_ } => { + BrilligOpcode::Cast { + destination, + source, + bit_size, + } => { + avm_instrs.push(generate_cast_instruction( + source.to_usize() as u32, + destination.to_usize() as u32, + tag_from_bit_size(*bit_size), + )); + } + BrilligOpcode::ForeignCall { + function, + destinations, + inputs, + destination_value_types: _, + input_value_types: _, + } => { handle_foreign_call(&mut avm_instrs, function, destinations, inputs); - }, - BrilligOpcode::BlackBox(operation) => handle_black_box_function(&mut avm_instrs, operation), + } + BrilligOpcode::BlackBox(operation) => { + handle_black_box_function(&mut avm_instrs, operation) + } _ => panic!( "Transpiler doesn't know how to process {:?} brillig instruction", brillig_instr From d931931ae23a564bcaed38b1dd5b899a10e1e647 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 12 Apr 2024 14:20:25 +0000 Subject: [PATCH 03/12] chore: update deployer addr --- l1-contracts/src/core/libraries/ConstantsGen.sol | 2 +- .../noir-protocol-circuits/crates/types/src/constants.nr | 2 +- yarn-project/circuits.js/src/constants.gen.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/l1-contracts/src/core/libraries/ConstantsGen.sol b/l1-contracts/src/core/libraries/ConstantsGen.sol index ecc56722a511..49eb0976e869 100644 --- a/l1-contracts/src/core/libraries/ConstantsGen.sol +++ b/l1-contracts/src/core/libraries/ConstantsGen.sol @@ -83,7 +83,7 @@ library Constants { uint256 internal constant DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631; uint256 internal constant DEPLOYER_CONTRACT_ADDRESS = - 0x1df42e0457430b8d294d920181cc72ae0e3c5f8afd8d62d461bd26773cfdf3c1; + 0x0b217585168f564dbf0e5d81b5ab0b76f5dcdb9e713822f488079ea27caa4cbb; uint256 internal constant L1_TO_L2_MESSAGE_ORACLE_CALL_LENGTH = 17; uint256 internal constant MAX_NOTE_FIELDS_LENGTH = 20; uint256 internal constant GET_NOTE_ORACLE_RETURN_LENGTH = 23; diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index a25b45b2226c..e2d5287d2d75 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -118,7 +118,7 @@ global REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUE = 0xe7af8166354 // CONTRACT INSTANCE CONSTANTS // sha224sum 'struct ContractInstanceDeployed' global DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631; -global DEPLOYER_CONTRACT_ADDRESS = 0x1df42e0457430b8d294d920181cc72ae0e3c5f8afd8d62d461bd26773cfdf3c1; +global DEPLOYER_CONTRACT_ADDRESS = 0x0b217585168f564dbf0e5d81b5ab0b76f5dcdb9e713822f488079ea27caa4cbb; // NOIR CONSTANTS - constants used only in yarn-packages/noir-contracts // Some are defined here because Noir doesn't yet support globals referencing other globals yet. diff --git a/yarn-project/circuits.js/src/constants.gen.ts b/yarn-project/circuits.js/src/constants.gen.ts index 5a8cb29928c9..7565522bf676 100644 --- a/yarn-project/circuits.js/src/constants.gen.ts +++ b/yarn-project/circuits.js/src/constants.gen.ts @@ -68,7 +68,7 @@ export const REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUE = 0xe7af816635466f128568edb04c9fa024f6c87fb9010fdbffa68b3d99n; export const DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631n; -export const DEPLOYER_CONTRACT_ADDRESS = 0x1df42e0457430b8d294d920181cc72ae0e3c5f8afd8d62d461bd26773cfdf3c1n; +export const DEPLOYER_CONTRACT_ADDRESS = 0x0b217585168f564dbf0e5d81b5ab0b76f5dcdb9e713822f488079ea27caa4cbbn; export const L1_TO_L2_MESSAGE_ORACLE_CALL_LENGTH = 17; export const MAX_NOTE_FIELDS_LENGTH = 20; export const GET_NOTE_ORACLE_RETURN_LENGTH = 23; From bbc7b706e36fb05326358452dd42c45866b52aa1 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 12 Apr 2024 15:48:20 +0000 Subject: [PATCH 04/12] test: fix strings --- docs/docs/developers/debugging/aztecnr-errors.md | 2 +- yarn-project/end-to-end/src/e2e_dapp_subscription.test.ts | 2 +- .../src/e2e_token_contract/reading_constants.test.ts | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/docs/developers/debugging/aztecnr-errors.md b/docs/docs/developers/debugging/aztecnr-errors.md index 5d5fd8053a90..67b6496612dd 100644 --- a/docs/docs/developers/debugging/aztecnr-errors.md +++ b/docs/docs/developers/debugging/aztecnr-errors.md @@ -67,7 +67,7 @@ This error occurs when your contract is trying to get a secret via the `get_secr This error might occur when you register an account only as a recipient and not as an account. To address the error, register the account by calling `server.registerAccount(...)`. -#### `Failed to solve brillig function, reason: explicit trap hit in brillig 'self._is_some` +#### `Failed to solve brillig function 'self._is_some` You may encounter this error when trying to send a transaction that is using an invalid contract. The contract may compile without errors and you only encounter this when sending the transaction. diff --git a/yarn-project/end-to-end/src/e2e_dapp_subscription.test.ts b/yarn-project/end-to-end/src/e2e_dapp_subscription.test.ts index df3cf484e453..1ce2e32d3bdb 100644 --- a/yarn-project/end-to-end/src/e2e_dapp_subscription.test.ts +++ b/yarn-project/end-to-end/src/e2e_dapp_subscription.test.ts @@ -222,7 +222,7 @@ describe('e2e_dapp_subscription', () => { // subscribe again. This will overwrite the subscription await subscribe(new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), MAX_FEE, 0); await expect(dappIncrement()).rejects.toThrow( - "Failed to solve brillig function, reason: explicit trap hit in brillig '(context.block_number()) as u64 < expiry_block_number as u64'", + "Failed to solve brillig function '(context.block_number()) as u64 < expiry_block_number as u64'", ); }); diff --git a/yarn-project/end-to-end/src/e2e_token_contract/reading_constants.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/reading_constants.test.ts index dfdacb8ab2a9..b7a5089217f6 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/reading_constants.test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/reading_constants.test.ts @@ -87,7 +87,7 @@ describe('e2e_token_contract reading constants', () => { await reader.methods.check_symbol_public(t.asset.address, TOKEN_SYMBOL).send().wait(); await expect(reader.methods.check_symbol_public(t.asset.address, 'WRONG_SYMBOL').simulate()).rejects.toThrow( - "Failed to solve brillig function, reason: explicit trap hit in brillig 'symbol.is_eq(_what)'", + "Failed to solve brillig function 'symbol.is_eq(_what)'", ); }); @@ -109,7 +109,7 @@ describe('e2e_token_contract reading constants', () => { await reader.methods.check_decimals_public(t.asset.address, TOKEN_DECIMALS).send().wait(); await expect(reader.methods.check_decimals_public(t.asset.address, 99).simulate()).rejects.toThrow( - "Failed to solve brillig function, reason: explicit trap hit in brillig 'ret == what'", + "Failed to solve brillig function 'ret == what'", ); }); }); From f562989562972b329fb6d6a10ac01333083b8b50 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 15 Apr 2024 09:10:17 +0000 Subject: [PATCH 05/12] chore: test performance with only user-level asserts --- .../src/brillig/brillig_gen/brillig_block.rs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 350cff1f1dda..a8230f16c1b9 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -642,7 +642,7 @@ impl<'block> BrilligBlock<'block> { ); self.brillig_context.deallocate_register(source_size_as_register); } - Instruction::RangeCheck { value, max_bit_size, assert_message } => { + Instruction::RangeCheck { value, max_bit_size, .. } => { let value = self.convert_ssa_single_addr_value(*value, dfg); // SSA generates redundant range checks. A range check with a max bit size >= value.bit_size will always pass. if value.bit_size > *max_bit_size { @@ -670,7 +670,7 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.codegen_constrain(condition, assert_message.clone()); + self.brillig_context.codegen_constrain(condition, None); self.brillig_context.deallocate_single_addr(condition); self.brillig_context.deallocate_single_addr(left); self.brillig_context.deallocate_single_addr(right); @@ -801,8 +801,7 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::LessThan, ); - self.brillig_context - .codegen_constrain(condition, Some("Array index out of bounds".to_owned())); + self.brillig_context.codegen_constrain(condition, None); if should_deallocate_size { self.brillig_context.deallocate_single_addr(size_as_register); @@ -1503,8 +1502,7 @@ impl<'block> BrilligBlock<'block> { condition, BrilligBinaryOp::LessThanEquals, ); - self.brillig_context - .codegen_constrain(condition, Some("attempt to add with overflow".to_string())); + self.brillig_context.codegen_constrain(condition, None); self.brillig_context.deallocate_single_addr(condition); } (BrilligBinaryOp::Sub, false) => { @@ -1517,10 +1515,7 @@ impl<'block> BrilligBlock<'block> { condition, BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.codegen_constrain( - condition, - Some("attempt to subtract with overflow".to_string()), - ); + self.brillig_context.codegen_constrain(condition, None); self.brillig_context.deallocate_single_addr(condition); } (BrilligBinaryOp::Mul, false) => { @@ -1547,10 +1542,7 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::UnsignedDiv, ); ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); - ctx.codegen_constrain( - condition, - Some("attempt to multiply with overflow".to_string()), - ); + ctx.codegen_constrain(condition, None); ctx.deallocate_single_addr(condition); ctx.deallocate_single_addr(division); }); From 3a95d2e982b9c8ff039f96dab22b588c667ff59e Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 15 Apr 2024 10:38:32 +0000 Subject: [PATCH 06/12] Revert "chore: test performance with only user-level asserts" This reverts commit f562989562972b329fb6d6a10ac01333083b8b50. --- .../src/brillig/brillig_gen/brillig_block.rs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index a8230f16c1b9..350cff1f1dda 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -642,7 +642,7 @@ impl<'block> BrilligBlock<'block> { ); self.brillig_context.deallocate_register(source_size_as_register); } - Instruction::RangeCheck { value, max_bit_size, .. } => { + Instruction::RangeCheck { value, max_bit_size, assert_message } => { let value = self.convert_ssa_single_addr_value(*value, dfg); // SSA generates redundant range checks. A range check with a max bit size >= value.bit_size will always pass. if value.bit_size > *max_bit_size { @@ -670,7 +670,7 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.codegen_constrain(condition, None); + self.brillig_context.codegen_constrain(condition, assert_message.clone()); self.brillig_context.deallocate_single_addr(condition); self.brillig_context.deallocate_single_addr(left); self.brillig_context.deallocate_single_addr(right); @@ -801,7 +801,8 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::LessThan, ); - self.brillig_context.codegen_constrain(condition, None); + self.brillig_context + .codegen_constrain(condition, Some("Array index out of bounds".to_owned())); if should_deallocate_size { self.brillig_context.deallocate_single_addr(size_as_register); @@ -1502,7 +1503,8 @@ impl<'block> BrilligBlock<'block> { condition, BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.codegen_constrain(condition, None); + self.brillig_context + .codegen_constrain(condition, Some("attempt to add with overflow".to_string())); self.brillig_context.deallocate_single_addr(condition); } (BrilligBinaryOp::Sub, false) => { @@ -1515,7 +1517,10 @@ impl<'block> BrilligBlock<'block> { condition, BrilligBinaryOp::LessThanEquals, ); - self.brillig_context.codegen_constrain(condition, None); + self.brillig_context.codegen_constrain( + condition, + Some("attempt to subtract with overflow".to_string()), + ); self.brillig_context.deallocate_single_addr(condition); } (BrilligBinaryOp::Mul, false) => { @@ -1542,7 +1547,10 @@ impl<'block> BrilligBlock<'block> { BrilligBinaryOp::UnsignedDiv, ); ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); - ctx.codegen_constrain(condition, None); + ctx.codegen_constrain( + condition, + Some("attempt to multiply with overflow".to_string()), + ); ctx.deallocate_single_addr(condition); ctx.deallocate_single_addr(division); }); From 6bda0d2708e832230584aabe62374dc4ee7fdd25 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 15 Apr 2024 11:13:17 +0000 Subject: [PATCH 07/12] feat: use revert data only for user-defined messages --- .../acvm-repo/acvm_js/src/execute.rs | 11 +++++++- .../src/brillig/brillig_gen/brillig_block.rs | 25 +++++++++++++------ .../brillig/brillig_gen/brillig_directive.rs | 3 +++ .../src/brillig/brillig_ir/artifact.rs | 18 ++++++++++++- .../brillig_ir/codegen_control_flow.rs | 19 +++++++++++++- .../ssa/acir_gen/acir_ir/generated_acir.rs | 6 +++++ .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 11 +++++--- .../noirc_evaluator/src/ssa/ir/instruction.rs | 18 ++++++++++--- .../noirc_evaluator/src/ssa/ir/printer.rs | 9 ++++--- .../src/ssa/opt/defunctionalize.rs | 15 +++++------ .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 10 +++++--- 11 files changed, 114 insertions(+), 31 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs b/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs index 9e7d34f80124..e8ff5887e790 100644 --- a/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs +++ b/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs @@ -245,7 +245,16 @@ impl<'a, B: BlackBoxFunctionSolver> ProgramExecutor<'a, B> { OpcodeResolutionError::BrilligFunctionFailed { call_stack, message, - } => (message.as_ref().map(|x| x.as_str()), Some(call_stack.clone())), + } => { + let revert_message = message.as_ref().map(|x| x.as_str()); + let failing_opcode = call_stack + .last() + .expect("Brillig error call stacks cannot be empty"); + ( + revert_message.or(circuit.get_assert_message(*failing_opcode)), + Some(call_stack.clone()), + ) + } _ => (None, None), }; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 350cff1f1dda..788d518b8296 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -5,7 +5,7 @@ use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; use crate::ssa::ir::dfg::CallStack; -use crate::ssa::ir::instruction::ConstrainError; +use crate::ssa::ir::instruction::{ConstrainError, UserDefinedError}; use crate::ssa::ir::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, @@ -248,10 +248,15 @@ impl<'block> BrilligBlock<'block> { self.convert_ssa_binary(binary, dfg, result_var); } Instruction::Constrain(lhs, rhs, assert_message) => { - let assert_message = if let Some(error) = assert_message { + let (has_revert_data, static_assert_message) = if let Some(error) = assert_message { match error.as_ref() { - ConstrainError::Static(string) => Some(string.clone()), - ConstrainError::Dynamic(call_instruction) => { + ConstrainError::Intrinsic(string) => (false, Some(string.clone())), + ConstrainError::UserDefined(UserDefinedError::Static(string)) => { + (true, Some(string.clone())) + } + ConstrainError::UserDefined(UserDefinedError::Dynamic( + call_instruction, + )) => { let Instruction::Call { func, arguments } = call_instruction else { unreachable!("expected a call instruction") }; @@ -264,11 +269,11 @@ impl<'block> BrilligBlock<'block> { // Dynamic assert messages are handled in the generated function call. // We then don't need to attach one to the constrain instruction. - None + (false, None) } } } else { - None + (false, None) }; let condition = SingleAddrVariable { @@ -281,8 +286,12 @@ impl<'block> BrilligBlock<'block> { dfg, condition, ); - - self.brillig_context.codegen_constrain(condition, assert_message); + if has_revert_data { + self.brillig_context + .codegen_constrain_with_revertdata(condition, static_assert_message); + } else { + self.brillig_context.codegen_constrain(condition, static_assert_message); + } self.brillig_context.deallocate_single_addr(condition); } Instruction::Allocate => { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs index 03990e398955..4b97a61491d0 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs @@ -53,6 +53,7 @@ pub(crate) fn directive_invert() -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 1 }, ], + assert_messages: Default::default(), locations: Default::default(), } } @@ -110,6 +111,7 @@ pub(crate) fn directive_quotient(bit_size: u32) -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 2 }, ], + assert_messages: Default::default(), locations: Default::default(), } } else { @@ -162,6 +164,7 @@ pub(crate) fn directive_quotient(bit_size: u32) -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 2 }, ], + assert_messages: Default::default(), locations: Default::default(), } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index b5bf4d45e7e3..8ce15ba4e73f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -21,6 +21,7 @@ pub(crate) enum BrilligParameter { pub(crate) struct GeneratedBrillig { pub(crate) byte_code: Vec, pub(crate) locations: BTreeMap, + pub(crate) assert_messages: BTreeMap, } #[derive(Default, Debug, Clone)] @@ -28,6 +29,8 @@ pub(crate) struct GeneratedBrillig { /// It includes the bytecode of the function and all the metadata that allows linking with other functions. pub(crate) struct BrilligArtifact { pub(crate) byte_code: Vec, + /// A map of bytecode positions to assertion messages + pub(crate) assert_messages: BTreeMap, /// The set of jumps that need to have their locations /// resolved. unresolved_jumps: Vec<(JumpInstructionPosition, UnresolvedJumpLocation)>, @@ -72,7 +75,11 @@ impl BrilligArtifact { /// Resolves all jumps and generates the final bytecode pub(crate) fn finish(mut self) -> GeneratedBrillig { self.resolve_jumps(); - GeneratedBrillig { byte_code: self.byte_code, locations: self.locations } + GeneratedBrillig { + byte_code: self.byte_code, + locations: self.locations, + assert_messages: self.assert_messages, + } } /// Gets the first unresolved function call of this artifact. @@ -135,6 +142,10 @@ impl BrilligArtifact { .push((position_in_bytecode + offset, label_id.clone())); } + for (position_in_bytecode, message) in &obj.assert_messages { + self.assert_messages.insert(position_in_bytecode + offset, message.clone()); + } + for (position_in_bytecode, call_stack) in obj.locations.iter() { self.locations.insert(position_in_bytecode + offset, call_stack.clone()); } @@ -246,4 +257,9 @@ impl BrilligArtifact { pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) { self.call_stack = call_stack; } + + pub(crate) fn add_assert_message_to_last_opcode(&mut self, message: String) { + let position = self.index_of_next_opcode() - 1; + self.assert_messages.insert(position, message); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs index 8e9d791212e5..140c5b1dfba1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs @@ -141,7 +141,7 @@ impl BrilligContext { /// Emits brillig bytecode to jump to a trap condition if `condition` /// is false. - pub(crate) fn codegen_constrain( + pub(crate) fn codegen_constrain_with_revertdata( &mut self, condition: SingleAddrVariable, assert_message: Option, @@ -165,4 +165,21 @@ impl BrilligContext { ctx.trap_instruction(revert_data_offset, revert_data_size); }); } + + /// Emits brillig bytecode to jump to a trap condition if `condition` + /// is false. + pub(crate) fn codegen_constrain( + &mut self, + condition: SingleAddrVariable, + assert_message: Option, + ) { + assert!(condition.bit_size == 1); + + self.codegen_if_not(condition.address, |ctx| { + ctx.trap_instruction(0, 0); + if let Some(assert_message) = assert_message { + ctx.obj.add_assert_message_to_last_opcode(assert_message); + } + }); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index fa3ffb4052b4..ba4e03bff955 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -559,6 +559,12 @@ impl GeneratedAcir { call_stack, ); } + for (brillig_index, message) in generated_brillig.assert_messages { + self.assert_messages.insert( + OpcodeLocation::Brillig { acir_index: self.opcodes.len() - 1, brillig_index }, + message, + ); + } } pub(crate) fn last_acir_opcode_location(&self) -> OpcodeLocation { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 9f2cec5c9490..4f6ad667d976 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -7,7 +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::instruction::{ConstrainError, UserDefinedError}; use super::{ ir::{ dfg::DataFlowGraph, @@ -472,8 +472,13 @@ impl Context { let assert_message = if let Some(error) = assert_message { match error.as_ref() { - ConstrainError::Static(string) => Some(string.clone()), - ConstrainError::Dynamic(call_instruction) => { + ConstrainError::Intrinsic(string) + | ConstrainError::UserDefined(UserDefinedError::Static(string)) => { + Some(string.clone()) + } + ConstrainError::UserDefined(UserDefinedError::Dynamic( + call_instruction, + )) => { self.convert_ssa_call(call_instruction, dfg, ssa, brillig, &[])?; None } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 2b23cc1c1e80..964a036858d2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -341,9 +341,9 @@ impl Instruction { let lhs = f(*lhs); let rhs = f(*rhs); let assert_message = assert_message.as_ref().map(|error| match error.as_ref() { - ConstrainError::Dynamic(call_instr) => { + ConstrainError::UserDefined(UserDefinedError::Dynamic(call_instr)) => { let new_instr = call_instr.map_values(f); - Box::new(ConstrainError::Dynamic(new_instr)) + Box::new(ConstrainError::UserDefined(UserDefinedError::Dynamic(new_instr))) } _ => error.clone(), }); @@ -405,7 +405,9 @@ impl Instruction { f(*lhs); f(*rhs); if let Some(error) = assert_error.as_ref() { - if let ConstrainError::Dynamic(call_instr) = error.as_ref() { + if let ConstrainError::UserDefined(UserDefinedError::Dynamic(call_instr)) = + error.as_ref() + { call_instr.for_each_value(f); } } @@ -591,6 +593,14 @@ impl Instruction { #[derive(Debug, PartialEq, Eq, Hash, Clone)] pub(crate) enum ConstrainError { // These are errors which have been hardcoded during SSA gen + Intrinsic(String), + // These are errors issued by the user + UserDefined(UserDefinedError), +} + +#[derive(Debug, PartialEq, Eq, Hash, Clone)] +pub(crate) enum UserDefinedError { + // These are errors which come from static strings specified by a Noir program Static(String), // These are errors which come from runtime expressions specified by a Noir program // We store an `Instruction` as we want this Instruction to be atomic in SSA with @@ -600,7 +610,7 @@ pub(crate) enum ConstrainError { impl From for ConstrainError { fn from(value: String) -> Self { - ConstrainError::Static(value) + ConstrainError::Intrinsic(value) } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs index fc13ab7307ae..a1bcb437683a 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -9,7 +9,9 @@ use iter_extended::vecmap; use super::{ basic_block::BasicBlockId, function::Function, - instruction::{ConstrainError, Instruction, InstructionId, TerminatorInstruction}, + instruction::{ + ConstrainError, Instruction, InstructionId, TerminatorInstruction, UserDefinedError, + }, value::ValueId, }; @@ -201,10 +203,11 @@ fn display_constrain_error( f: &mut Formatter, ) -> Result { match error { - ConstrainError::Static(assert_message_string) => { + ConstrainError::Intrinsic(assert_message_string) + | ConstrainError::UserDefined(UserDefinedError::Static(assert_message_string)) => { writeln!(f, "{assert_message_string:?}") } - ConstrainError::Dynamic(assert_message_call) => { + ConstrainError::UserDefined(UserDefinedError::Dynamic(assert_message_call)) => { display_instruction_inner(function, assert_message_call, f) } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs index ca6527eb0ecc..a517b365c3f8 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -14,7 +14,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, function::{Function, FunctionId, Signature}, - instruction::{BinaryOp, ConstrainError, Instruction}, + instruction::{BinaryOp, ConstrainError, Instruction, UserDefinedError}, types::{NumericType, Type}, value::{Value, ValueId}, }, @@ -93,10 +93,9 @@ impl DefunctionalizationContext { // Constrain instruction potentially hold a call instruction themselves // thus we need to account for them. Instruction::Constrain(_, _, Some(constrain_error)) => { - if let ConstrainError::Dynamic(Instruction::Call { - func: target_func_id, - arguments, - }) = constrain_error.as_ref() + if let ConstrainError::UserDefined(UserDefinedError::Dynamic( + Instruction::Call { func: target_func_id, arguments }, + )) = constrain_error.as_ref() { (*target_func_id, arguments) } else { @@ -138,8 +137,10 @@ impl DefunctionalizationContext { if let Instruction::Constrain(lhs, rhs, constrain_error_call) = instruction { let new_error_call = if let Some(error) = constrain_error_call { match error.as_ref() { - ConstrainError::Dynamic(_) => { - Some(Box::new(ConstrainError::Dynamic(new_instruction))) + ConstrainError::UserDefined(UserDefinedError::Dynamic(_)) => { + Some(Box::new(ConstrainError::UserDefined( + UserDefinedError::Dynamic(new_instruction), + ))) } _ => None, } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 3fe52f7f0e5e..6155aeea9afc 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -29,7 +29,9 @@ use super::{ function_builder::data_bus::DataBus, ir::{ function::RuntimeType, - instruction::{BinaryOp, ConstrainError, Instruction, TerminatorInstruction}, + instruction::{ + BinaryOp, ConstrainError, Instruction, TerminatorInstruction, UserDefinedError, + }, types::Type, value::ValueId, }, @@ -707,7 +709,9 @@ impl<'a> FunctionContext<'a> { if let ast::Expression::Literal(ast::Literal::Str(assert_message)) = assert_message_expr.as_ref() { - return Ok(Some(Box::new(ConstrainError::Static(assert_message.to_string())))); + return Ok(Some(Box::new(ConstrainError::UserDefined(UserDefinedError::Static( + assert_message.to_string(), + ))))); } let ast::Expression::Call(call) = assert_message_expr.as_ref() else { @@ -733,7 +737,7 @@ impl<'a> FunctionContext<'a> { } let instr = Instruction::Call { func, arguments }; - Ok(Some(Box::new(ConstrainError::Dynamic(instr)))) + Ok(Some(Box::new(ConstrainError::UserDefined(UserDefinedError::Dynamic(instr))))) } fn codegen_assign(&mut self, assign: &ast::Assign) -> Result { From 3ba8a433e84f71970fe41f774d4ea9037f2fc66c Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 15 Apr 2024 11:59:46 +0000 Subject: [PATCH 08/12] refactor: renames and comments --- avm-transpiler/src/transpile.rs | 1 - .../src/brillig/brillig_gen/brillig_block.rs | 8 ++++---- .../brillig/brillig_ir/codegen_control_flow.rs | 4 ++-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 6 +++--- .../noirc_evaluator/src/ssa/ir/instruction.rs | 15 +++++++++------ .../noirc_evaluator/src/ssa/ir/printer.rs | 7 ++++--- .../src/ssa/opt/defunctionalize.rs | 14 +++++++------- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 10 +++++----- 8 files changed, 34 insertions(+), 31 deletions(-) diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs index 15eaebaf51ba..8234f8721e23 100644 --- a/avm-transpiler/src/transpile.rs +++ b/avm-transpiler/src/transpile.rs @@ -256,7 +256,6 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { revert_data_offset, revert_data_size, } => { - // TODO(https://github.com/noir-lang/noir/issues/3113): Trap should support return data avm_instrs.push(AvmInstruction { opcode: AvmOpcode::REVERT, indirect: Some(ALL_DIRECT), diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 788d518b8296..369d8e78cdaa 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -5,7 +5,7 @@ use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; use crate::ssa::ir::dfg::CallStack; -use crate::ssa::ir::instruction::{ConstrainError, UserDefinedError}; +use crate::ssa::ir::instruction::{ConstrainError, UserDefinedConstrainError}; use crate::ssa::ir::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, @@ -251,10 +251,10 @@ impl<'block> BrilligBlock<'block> { let (has_revert_data, static_assert_message) = if let Some(error) = assert_message { match error.as_ref() { ConstrainError::Intrinsic(string) => (false, Some(string.clone())), - ConstrainError::UserDefined(UserDefinedError::Static(string)) => { + ConstrainError::UserDefined(UserDefinedConstrainError::Static(string)) => { (true, Some(string.clone())) } - ConstrainError::UserDefined(UserDefinedError::Dynamic( + ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic( call_instruction, )) => { let Instruction::Call { func, arguments } = call_instruction else { @@ -288,7 +288,7 @@ impl<'block> BrilligBlock<'block> { ); if has_revert_data { self.brillig_context - .codegen_constrain_with_revertdata(condition, static_assert_message); + .codegen_constrain_with_revert_data(condition, static_assert_message); } else { self.brillig_context.codegen_constrain(condition, static_assert_message); } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs index 140c5b1dfba1..f8f39f03df4d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs @@ -140,8 +140,8 @@ impl BrilligContext { } /// Emits brillig bytecode to jump to a trap condition if `condition` - /// is false. - pub(crate) fn codegen_constrain_with_revertdata( + /// is false. The trap will include the given message as revert data. + pub(crate) fn codegen_constrain_with_revert_data( &mut self, condition: SingleAddrVariable, assert_message: Option, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 4f6ad667d976..76cc8acf878f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -7,7 +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, UserDefinedError}; +use super::ir::instruction::{ConstrainError, UserDefinedConstrainError}; use super::{ ir::{ dfg::DataFlowGraph, @@ -473,10 +473,10 @@ impl Context { let assert_message = if let Some(error) = assert_message { match error.as_ref() { ConstrainError::Intrinsic(string) - | ConstrainError::UserDefined(UserDefinedError::Static(string)) => { + | ConstrainError::UserDefined(UserDefinedConstrainError::Static(string)) => { Some(string.clone()) } - ConstrainError::UserDefined(UserDefinedError::Dynamic( + ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic( call_instruction, )) => { self.convert_ssa_call(call_instruction, dfg, ssa, brillig, &[])?; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 964a036858d2..d48f92227faa 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -341,9 +341,11 @@ impl Instruction { let lhs = f(*lhs); let rhs = f(*rhs); let assert_message = assert_message.as_ref().map(|error| match error.as_ref() { - ConstrainError::UserDefined(UserDefinedError::Dynamic(call_instr)) => { + ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic(call_instr)) => { let new_instr = call_instr.map_values(f); - Box::new(ConstrainError::UserDefined(UserDefinedError::Dynamic(new_instr))) + Box::new(ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic( + new_instr, + ))) } _ => error.clone(), }); @@ -405,8 +407,9 @@ impl Instruction { f(*lhs); f(*rhs); if let Some(error) = assert_error.as_ref() { - if let ConstrainError::UserDefined(UserDefinedError::Dynamic(call_instr)) = - error.as_ref() + if let ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic( + call_instr, + )) = error.as_ref() { call_instr.for_each_value(f); } @@ -595,11 +598,11 @@ pub(crate) enum ConstrainError { // These are errors which have been hardcoded during SSA gen Intrinsic(String), // These are errors issued by the user - UserDefined(UserDefinedError), + UserDefined(UserDefinedConstrainError), } #[derive(Debug, PartialEq, Eq, Hash, Clone)] -pub(crate) enum UserDefinedError { +pub(crate) enum UserDefinedConstrainError { // These are errors which come from static strings specified by a Noir program Static(String), // These are errors which come from runtime expressions specified by a Noir program diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs index a1bcb437683a..d17d2989341d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -10,7 +10,8 @@ use super::{ basic_block::BasicBlockId, function::Function, instruction::{ - ConstrainError, Instruction, InstructionId, TerminatorInstruction, UserDefinedError, + ConstrainError, Instruction, InstructionId, TerminatorInstruction, + UserDefinedConstrainError, }, value::ValueId, }; @@ -204,10 +205,10 @@ fn display_constrain_error( ) -> Result { match error { ConstrainError::Intrinsic(assert_message_string) - | ConstrainError::UserDefined(UserDefinedError::Static(assert_message_string)) => { + | ConstrainError::UserDefined(UserDefinedConstrainError::Static(assert_message_string)) => { writeln!(f, "{assert_message_string:?}") } - ConstrainError::UserDefined(UserDefinedError::Dynamic(assert_message_call)) => { + ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic(assert_message_call)) => { display_instruction_inner(function, assert_message_call, f) } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs index a517b365c3f8..aa0368cc2dd2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -14,7 +14,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, function::{Function, FunctionId, Signature}, - instruction::{BinaryOp, ConstrainError, Instruction, UserDefinedError}, + instruction::{BinaryOp, ConstrainError, Instruction, UserDefinedConstrainError}, types::{NumericType, Type}, value::{Value, ValueId}, }, @@ -93,7 +93,7 @@ impl DefunctionalizationContext { // Constrain instruction potentially hold a call instruction themselves // thus we need to account for them. Instruction::Constrain(_, _, Some(constrain_error)) => { - if let ConstrainError::UserDefined(UserDefinedError::Dynamic( + if let ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic( Instruction::Call { func: target_func_id, arguments }, )) = constrain_error.as_ref() { @@ -137,11 +137,11 @@ impl DefunctionalizationContext { if let Instruction::Constrain(lhs, rhs, constrain_error_call) = instruction { let new_error_call = if let Some(error) = constrain_error_call { match error.as_ref() { - ConstrainError::UserDefined(UserDefinedError::Dynamic(_)) => { - Some(Box::new(ConstrainError::UserDefined( - UserDefinedError::Dynamic(new_instruction), - ))) - } + ConstrainError::UserDefined( + UserDefinedConstrainError::Dynamic(_), + ) => Some(Box::new(ConstrainError::UserDefined( + UserDefinedConstrainError::Dynamic(new_instruction), + ))), _ => None, } } else { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 6155aeea9afc..59ce4e4f754b 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -30,7 +30,7 @@ use super::{ ir::{ function::RuntimeType, instruction::{ - BinaryOp, ConstrainError, Instruction, TerminatorInstruction, UserDefinedError, + BinaryOp, ConstrainError, Instruction, TerminatorInstruction, UserDefinedConstrainError, }, types::Type, value::ValueId, @@ -709,9 +709,9 @@ impl<'a> FunctionContext<'a> { if let ast::Expression::Literal(ast::Literal::Str(assert_message)) = assert_message_expr.as_ref() { - return Ok(Some(Box::new(ConstrainError::UserDefined(UserDefinedError::Static( - assert_message.to_string(), - ))))); + return Ok(Some(Box::new(ConstrainError::UserDefined( + UserDefinedConstrainError::Static(assert_message.to_string()), + )))); } let ast::Expression::Call(call) = assert_message_expr.as_ref() else { @@ -737,7 +737,7 @@ impl<'a> FunctionContext<'a> { } let instr = Instruction::Call { func, arguments }; - Ok(Some(Box::new(ConstrainError::UserDefined(UserDefinedError::Dynamic(instr))))) + Ok(Some(Box::new(ConstrainError::UserDefined(UserDefinedConstrainError::Dynamic(instr))))) } fn codegen_assign(&mut self, assign: &ast::Assign) -> Result { From 1eaa311bdbe772c4c5b165c4c73706d138a79b5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Mon, 15 Apr 2024 17:53:17 +0200 Subject: [PATCH 09/12] Update noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs Co-authored-by: Maxim Vezenov --- noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs b/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs index 87d51dd7e023..d141513efbe4 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs @@ -171,8 +171,8 @@ impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> { let bytes = memory [revert_data_offset..(revert_data_offset + revert_data_size)] .iter() - .map(|x| { - x.try_into().expect("Assert message character is not a byte") + .map(|memory_value| { + memory_value.try_into().expect("Assert message character is not a byte") }) .collect(); Some( From 60089a4fadc56352ed017f87fa2e8b828d1b8041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Mon, 15 Apr 2024 17:53:28 +0200 Subject: [PATCH 10/12] Update noir/noir-repo/acvm-repo/acvm_js/src/execute.rs Co-authored-by: Maxim Vezenov --- noir/noir-repo/acvm-repo/acvm_js/src/execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs b/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs index e8ff5887e790..8bc562959929 100644 --- a/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs +++ b/noir/noir-repo/acvm-repo/acvm_js/src/execute.rs @@ -246,7 +246,7 @@ impl<'a, B: BlackBoxFunctionSolver> ProgramExecutor<'a, B> { call_stack, message, } => { - let revert_message = message.as_ref().map(|x| x.as_str()); + let revert_message = message.as_ref().map(String::as_str); let failing_opcode = call_stack .last() .expect("Brillig error call stacks cannot be empty"); From d7554ab92b85d48ce77f64363bf0f253e946f80b Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 15 Apr 2024 15:58:20 +0000 Subject: [PATCH 11/12] style: fmt --- noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs b/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs index d141513efbe4..9982fa890c22 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs @@ -172,7 +172,9 @@ impl<'b, B: BlackBoxFunctionSolver> BrilligSolver<'b, B> { [revert_data_offset..(revert_data_offset + revert_data_size)] .iter() .map(|memory_value| { - memory_value.try_into().expect("Assert message character is not a byte") + memory_value + .try_into() + .expect("Assert message character is not a byte") }) .collect(); Some( From 5029825fac85e0de82d12d946156235ce36a85a4 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Tue, 16 Apr 2024 07:32:46 +0000 Subject: [PATCH 12/12] chore: update deployer addr --- l1-contracts/src/core/libraries/ConstantsGen.sol | 2 +- .../noir-protocol-circuits/crates/types/src/constants.nr | 2 +- yarn-project/circuits.js/src/constants.gen.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/l1-contracts/src/core/libraries/ConstantsGen.sol b/l1-contracts/src/core/libraries/ConstantsGen.sol index 49eb0976e869..b23000adb51e 100644 --- a/l1-contracts/src/core/libraries/ConstantsGen.sol +++ b/l1-contracts/src/core/libraries/ConstantsGen.sol @@ -83,7 +83,7 @@ library Constants { uint256 internal constant DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631; uint256 internal constant DEPLOYER_CONTRACT_ADDRESS = - 0x0b217585168f564dbf0e5d81b5ab0b76f5dcdb9e713822f488079ea27caa4cbb; + 0x1b02447505c1781a416a5f44bc5be922f0d2f709e0996877f673a86bd49f79f4; uint256 internal constant L1_TO_L2_MESSAGE_ORACLE_CALL_LENGTH = 17; uint256 internal constant MAX_NOTE_FIELDS_LENGTH = 20; uint256 internal constant GET_NOTE_ORACLE_RETURN_LENGTH = 23; diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index 50b8da0aa4e0..a97ddbf91217 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -118,7 +118,7 @@ global REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUE = 0xe7af8166354 // CONTRACT INSTANCE CONSTANTS // sha224sum 'struct ContractInstanceDeployed' global DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631; -global DEPLOYER_CONTRACT_ADDRESS = 0x0b217585168f564dbf0e5d81b5ab0b76f5dcdb9e713822f488079ea27caa4cbb; +global DEPLOYER_CONTRACT_ADDRESS = 0x1b02447505c1781a416a5f44bc5be922f0d2f709e0996877f673a86bd49f79f4; // NOIR CONSTANTS - constants used only in yarn-packages/noir-contracts // Some are defined here because Noir doesn't yet support globals referencing other globals yet. diff --git a/yarn-project/circuits.js/src/constants.gen.ts b/yarn-project/circuits.js/src/constants.gen.ts index 34818ffb7d8d..e69612799748 100644 --- a/yarn-project/circuits.js/src/constants.gen.ts +++ b/yarn-project/circuits.js/src/constants.gen.ts @@ -68,7 +68,7 @@ export const REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUE = 0xe7af816635466f128568edb04c9fa024f6c87fb9010fdbffa68b3d99n; export const DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631n; -export const DEPLOYER_CONTRACT_ADDRESS = 0x1d144e236f9df5ca2e46d274585f322f16502b33a91cd24bce24c93b73dde3c5n; +export const DEPLOYER_CONTRACT_ADDRESS = 0x1b02447505c1781a416a5f44bc5be922f0d2f709e0996877f673a86bd49f79f4n; export const L1_TO_L2_MESSAGE_ORACLE_CALL_LENGTH = 17; export const MAX_NOTE_FIELDS_LENGTH = 20; export const GET_NOTE_ORACLE_RETURN_LENGTH = 23;