feat!: trap with revert data (#5732)

This PR adds revert data for user-defined messages in the TRAP opcode. Not all messages are returned as revert data, since compiler intrinsic messages are spammed all over SSA and codegening revert data for all those makes brillig function size blow up and impacts performance in a hard way due to deserializing load. This is currently only for static assert messages, since dynamic ones are implemented as an oracle and probably need a rework to be able to return them as revert data. --------- Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
AztecProtocol · Apr 16, 2024 · f849575 · f849575
1 parent af49a29
commit f849575
Show file tree

Hide file tree

Showing 27 changed files with 336 additions and 124 deletions.
diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs
@@ -70,7 +70,11 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec<u8> {
                 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<u8> {
                     ],
                 });
             }
-            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<u8> {
                     ..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<u8> {
                     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<u8> {
                 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,65 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec<u8> {
                 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*/ } => {
-                // TODO(https://github.com/noir-lang/noir/issues/3113): Trap should support return data
+            BrilligOpcode::Trap {
+                revert_data_offset,
+                revert_data_size,
+            } => {
                 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

diff --git 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<uint8_t> bincodeSerialize() const;
         static Trap bincodeDeserialize(std::vector<uint8_t>);
@@ -6060,6 +6063,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;
 }
 
@@ -6086,14 +6095,19 @@ template <>
 template <typename Serializer>
 void serde::Serializable<Program::BrilligOpcode::Trap>::serialize(const Program::BrilligOpcode::Trap& obj,
                                                                   Serializer& serializer)
-{}
+{
+    serde::Serializable<decltype(obj.revert_data_offset)>::serialize(obj.revert_data_offset, serializer);
+    serde::Serializable<decltype(obj.revert_data_size)>::serialize(obj.revert_data_size, serializer);
+}
 
 template <>
 template <typename Deserializer>
 Program::BrilligOpcode::Trap serde::Deserializable<Program::BrilligOpcode::Trap>::deserialize(
     Deserializer& deserializer)
 {
     Program::BrilligOpcode::Trap obj;
+    obj.revert_data_offset = serde::Deserializable<decltype(obj.revert_data_offset)>::deserialize(deserializer);
+    obj.revert_data_size = serde::Deserializable<decltype(obj.revert_data_size)>::deserialize(deserializer);
     return obj;
 }
 

diff --git 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/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;
+    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
@@ -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 = 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/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<uint8_t> bincodeSerialize() const;
             static Trap bincodeDeserialize(std::vector<uint8_t>);
@@ -5014,6 +5017,8 @@ Program::BrilligOpcode::BlackBox serde::Deserializable<Program::BrilligOpcode::B
 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;
     }
 
@@ -5037,12 +5042,16 @@ namespace Program {
 template <>
 template <typename Serializer>
 void serde::Serializable<Program::BrilligOpcode::Trap>::serialize(const Program::BrilligOpcode::Trap &obj, Serializer &serializer) {
+    serde::Serializable<decltype(obj.revert_data_offset)>::serialize(obj.revert_data_offset, serializer);
+    serde::Serializable<decltype(obj.revert_data_size)>::serialize(obj.revert_data_size, serializer);
 }
 
 template <>
 template <typename Deserializer>
 Program::BrilligOpcode::Trap serde::Deserializable<Program::BrilligOpcode::Trap>::deserialize(Deserializer &deserializer) {
     Program::BrilligOpcode::Trap obj;
+    obj.revert_data_offset = serde::Deserializable<decltype(obj.revert_data_offset)>::deserialize(deserializer);
+    obj.revert_data_size = serde::Deserializable<decltype(obj.revert_data_size)>::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
@@ -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,31 @@ 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(|memory_value| {
+                                    memory_value
+                                        .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
@@ -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<OpcodeLocation> },
+    #[error("Failed to solve brillig function{}", .message.as_ref().map(|m| format!(", reason: {}", m)).unwrap_or_default())]
+    BrilligFunctionFailed { message: Option<String>, call_stack: Vec<OpcodeLocation> },
     #[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
@@ -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"