feat(avm)!: opcode error for infallible opcodes (#16629)

IlyasRidhuan · web-flow · commit 5ecbaf26c799 · 2025-08-28T14:10:27.000Z
This PR constrains `execution.sel_opcode_error` to be `0` for opcodes that cannot fail during the opcode execution stage. I gathered the following list of "infallible" opcodes looking at `execution.cpp` and seeing which ones do not throw `OpcodeExecutionException`: * [x] CAST * [x] SET * [x] MOV * [x] CALL * [x] STATICCALL * [x] RETURNDATASIZE * [x] RETURN * [x] REVERT * [x] JUMP * [x] JUMPI * [x] INTERNALCALL * [x] DEBUGLOG * [x] SUCCESSCOPY * [x] SLOAD (was already done) * [x] NOTEHASHEXISTS (was already done) * [x] NULLIFIEREXISTS (was already done) * [x] L1TOL2MESSAGEEXISTS (was already done) NOTE: Calls/returns use `sel_error` in a way that might be wrong, and tracegen for them is certainly wrong. I made some small changes but mostly added a new entry in our structural list to fix this. Closes #15375.
diff --git a/barretenberg/cpp/pil/vm2/alu.pil b/barretenberg/cpp/pil/vm2/alu.pil
@@ -513,9 +513,9 @@ sel_op_truncate * (1 - sel_op_truncate) = 0;
 // Note that we enforce the output tag through this dispatching lookup by having ia_tag matching both mem_tag_reg[1] and rop[2].
 #[EXEC_DISPATCHING_CAST]
 execution.sel_execute_cast {
-    execution.register[0], execution.rop[2], execution.subtrace_operation_id, execution.register[1], execution.mem_tag_reg[1]
+    execution.register[0], execution.rop[2], execution.subtrace_operation_id, execution.register[1], execution.mem_tag_reg[1], execution.sel_opcode_error
 } in sel_op_truncate {
-    ia, ia_tag, op_id, ic, ia_tag
+    ia, ia_tag, op_id, ic, ia_tag, /*sel_opcode_error=*/ precomputed.zero
 };
 
 // SET DISPATCHING
@@ -534,9 +534,9 @@ execution.sel_execute_cast {
 // Note that we enforce the output tag through this dispatching lookup by having ia_tag matching both mem_tag_reg[0] and rop[1].
 #[EXEC_DISPATCHING_SET]
 execution.sel_execute_set {
-    execution.rop[2], execution.rop[1], execution.subtrace_operation_id, execution.register[0], execution.mem_tag_reg[0]
+    execution.rop[2], execution.rop[1], execution.subtrace_operation_id, execution.register[0], execution.mem_tag_reg[0], execution.sel_opcode_error
 } in sel_op_truncate {
-    ia, ia_tag, op_id, ic, ic_tag
+    ia, ia_tag, op_id, ic, ic_tag, /*sel_opcode_error=*/ precomputed.zero
 };
 
 // 3 cases for truncation:
diff --git a/barretenberg/cpp/pil/vm2/context.pil b/barretenberg/cpp/pil/vm2/context.pil
@@ -1,5 +1,7 @@
 include "context_stack.pil";
 
+// TODO: Usage of sel_error in this whole file has to be reconsidered.
+
 // This is a virtual gadget, which is part of the execution trace.
 // This subtrace is focused on managing the changes to the context.
 // By default (i.e. when not executing a context-changing opcode),
@@ -12,12 +14,12 @@ namespace execution;
 
     // Guaranteed to be boolean because sel_execute_call & sel_execute_static_call are mutually exclusive
     pol commit sel_enter_call;
-    // Handle error separately since it takes priority and may occur during an sel_x_call
-    sel_enter_call = (sel_execute_call + sel_execute_static_call) * (1 - sel_error);
+    // The following selectors will be 0 if there is an error during execution (before the opcode execution step).
+    sel_enter_call = sel_execute_call + sel_execute_static_call;
     // CALL & precomputed.first_row are mutually exclusive
     sel_enter_call * precomputed.first_row = 0;
 
-    // sel_exit_call is used to flag if we are returning or reverting or we error
+    // sel_exit_call is used to flag if we are returning or reverting or there has been ANY error during execution
     // sel_execute_revert & sel_execute_return are mutually exclusive
     pol commit sel_exit_call;
     sel_exit_call = 1 - (1 - sel_execute_revert - sel_execute_return) * (1 - sel_error);
diff --git a/barretenberg/cpp/pil/vm2/execution.pil b/barretenberg/cpp/pil/vm2/execution.pil
@@ -642,6 +642,15 @@ sel * (1 - sel_execute_emit_unencrypted_log) * (prev_num_unencrypted_logs - num_
 #[NUM_L2_TO_L1_MESSAGES_NOT_CHANGED]
 sel * (1 - sel_execute_send_l2_to_l1_msg) * (prev_num_l2_to_l1_messages - num_l2_to_l1_messages) = 0;
 
+// Some opcodes cannot fail during opcode execution. They should not be able to set sel_opcode_error.
+// Note that some of these opcodes can fail in the stages before execution.
+// This list only takes into account the opcodes that are handled by the execution subtrace.
+// Infallible opcodes which are handled by other traces should constrain sel_opcode_error there.
+(sel_execute_mov + sel_execute_returndata_size + sel_execute_jump +
+ sel_execute_jumpi + sel_execute_debug_log + sel_execute_success_copy +
+ sel_execute_call + sel_execute_static_call + sel_execute_internal_call +
+ sel_execute_return + sel_execute_revert) * sel_opcode_error = 0;
+
 /**************************************************************************************************
  *  Temporality group 6: Register write.
  **************************************************************************************************/
diff --git a/barretenberg/cpp/pil/vm2/opcodes/internal_call.pil b/barretenberg/cpp/pil/vm2/opcodes/internal_call.pil
@@ -13,7 +13,9 @@ namespace execution;
     // When we encounter this case, the internal call information is reset on the next row (i.e. internal_call_id = 1, return_id = 0 and next_internal_call_id = 2)
     pol RESET_NEXT_CALL_ID =  sel_enter_call + enqueued_call_start';
 
-    // sel_exit_call includes the error case, so we gate sel_execute_internal_call and sel_execute_internal_return by (1 - sel_error). This makes them all mututally exclusive.
+    // sel_exit_call includes external exit cases (revert, return, any execution error).
+    // sel_execute_internal_return could be 1 and sel_opcode_error could be 1 at the same time (which would set sel_error=1 and sel_exit_call=1),
+    // so we need to gate them by (1 - sel_error) here. We gate both of them out of excessive caution.
     // When we encounter this case, the internal call information in the next row is constrained to change (incremented, unwound, etc)
     pol NEW_NEXT_CALL_ID = (sel_execute_internal_call + sel_execute_internal_return) * (1 - sel_error) + sel_exit_call;
 
diff --git a/barretenberg/cpp/pil/vm2/opcodes/nullifier_exists.pil b/barretenberg/cpp/pil/vm2/opcodes/nullifier_exists.pil
@@ -1,3 +1,5 @@
+include "../constants_gen.pil";
+
  /**
  * This virtual gadget implements the NullifierExists opcode, which checks if a nullifier exists
  * in the nullifier tree for a given contract address.
@@ -34,7 +36,7 @@ namespace execution; // this is a virtual gadget that shares rows with the execu
         // Inputs
         /*nullifier=*/ register[0],                      // input: nullifier to check
         prev_nullifier_tree_root,                        // input: tree root from context
-        /*should_silo=1*/ sel_execute_nullifier_exists,   // input: should_silo = 1 (always silo for contract nullifiers)
+        /*should_silo=1*/ sel_execute_nullifier_exists,  // input: should_silo = 1 (always silo for contract nullifiers)
         /*contract_address=*/ register[1]                // input: contract address for siloing
     } in nullifier_check.sel {
         // Outputs
diff --git a/barretenberg/cpp/pil/vm2/opcodes/sload.pil b/barretenberg/cpp/pil/vm2/opcodes/sload.pil
@@ -21,9 +21,6 @@ namespace execution; // this is a virtual gadget that shares rows with the execu
     #[skippable_if]
     sel_execute_sload = 0; // from execution.pil.
 
-    #[SLOAD_SUCCESS]
-    sel_execute_sload * sel_opcode_error = 0;
-
     #[STORAGE_READ]
     sel_execute_sload {
         contract_address, // From context
@@ -40,3 +37,5 @@ namespace execution; // this is a virtual gadget that shares rows with the execu
     #[SLOAD_FF_OUTPUT_TAG]
     sel_execute_sload * (constants.MEM_TAG_FF - mem_tag_reg[1]) = 0;
 
+    #[SLOAD_SUCCESS]
+    sel_execute_sload * sel_opcode_error = 0;
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/context.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/context.hpp
@@ -15,7 +15,7 @@ template <typename FF_> class contextImpl {
     using FF = FF_;
 
     static constexpr std::array<size_t, 69> SUBRELATION_PARTIAL_LENGTHS = {
-        3, 3, 3, 3, 4, 3, 3, 4, 5, 5, 5, 5, 5, 6, 5, 5, 5, 5, 5, 5, 3, 5, 5, 5, 5, 5, 5, 5, 6, 5, 5, 6, 5, 5, 5,
+        2, 3, 3, 3, 4, 3, 3, 4, 5, 5, 5, 5, 5, 6, 5, 5, 5, 5, 5, 5, 3, 5, 5, 5, 5, 5, 5, 5, 6, 5, 5, 6, 5, 5, 5,
         5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 3, 3, 3, 4, 4, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 4, 5, 5
     };
 
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/context_impl.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/context_impl.hpp
@@ -39,8 +39,7 @@ void contextImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     {
         using Accumulator = typename std::tuple_element_t<0, ContainerOverSubrelations>;
         auto tmp = (in.get(C::execution_sel_enter_call) -
-                    (in.get(C::execution_sel_execute_call) + in.get(C::execution_sel_execute_static_call)) *
-                        (FF(1) - in.get(C::execution_sel_error)));
+                    (in.get(C::execution_sel_execute_call) + in.get(C::execution_sel_execute_static_call)));
         tmp *= scaling_factor;
         std::get<0>(evals) += typename Accumulator::View(tmp);
     }
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/execution.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/execution.hpp
@@ -14,10 +14,10 @@ template <typename FF_> class executionImpl {
   public:
     using FF = FF_;
 
-    static constexpr std::array<size_t, 87> SUBRELATION_PARTIAL_LENGTHS = {
-        3, 3, 3, 3, 2, 4, 3, 3, 3, 4, 3, 3, 3, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3,
-        3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 3,
-        3, 3, 3, 3, 3, 4, 3, 5, 6, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 3, 2
+    static constexpr std::array<size_t, 88> SUBRELATION_PARTIAL_LENGTHS = {
+        3, 3, 3, 3, 2, 4, 3, 3, 3, 4, 3, 3, 3, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3,
+        3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 3, 3, 3,
+        3, 3, 3, 4, 3, 5, 6, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 3, 3, 2
     };
 
     template <typename AllEntities> inline static bool skip(const AllEntities& in)
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/execution_impl.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/execution_impl.hpp
@@ -684,20 +684,32 @@ void executionImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     }
     {
         using Accumulator = typename std::tuple_element_t<85, ContainerOverSubrelations>;
-        auto tmp = (in.get(C::execution_sel_should_write_registers) -
-                    in.get(C::execution_sel_should_execute_opcode) * (FF(1) - in.get(C::execution_sel_opcode_error)));
+        auto tmp = (in.get(C::execution_sel_execute_mov) + in.get(C::execution_sel_execute_returndata_size) +
+                    in.get(C::execution_sel_execute_jump) + in.get(C::execution_sel_execute_jumpi) +
+                    in.get(C::execution_sel_execute_debug_log) + in.get(C::execution_sel_execute_success_copy) +
+                    in.get(C::execution_sel_execute_call) + in.get(C::execution_sel_execute_static_call) +
+                    in.get(C::execution_sel_execute_internal_call) + in.get(C::execution_sel_execute_return) +
+                    in.get(C::execution_sel_execute_revert)) *
+                   in.get(C::execution_sel_opcode_error);
         tmp *= scaling_factor;
         std::get<85>(evals) += typename Accumulator::View(tmp);
     }
     {
         using Accumulator = typename std::tuple_element_t<86, ContainerOverSubrelations>;
+        auto tmp = (in.get(C::execution_sel_should_write_registers) -
+                    in.get(C::execution_sel_should_execute_opcode) * (FF(1) - in.get(C::execution_sel_opcode_error)));
+        tmp *= scaling_factor;
+        std::get<86>(evals) += typename Accumulator::View(tmp);
+    }
+    {
+        using Accumulator = typename std::tuple_element_t<87, ContainerOverSubrelations>;
         auto tmp = (in.get(C::execution_sel_error) -
                     (in.get(C::execution_sel_bytecode_retrieval_failure) +
                      in.get(C::execution_sel_instruction_fetching_failure) + in.get(C::execution_sel_addressing_error) +
                      in.get(C::execution_sel_register_read_error) + in.get(C::execution_sel_out_of_gas) +
                      in.get(C::execution_sel_opcode_error)));
         tmp *= scaling_factor;
-        std::get<86>(evals) += typename Accumulator::View(tmp);
+        std::get<87>(evals) += typename Accumulator::View(tmp);
     }
 }
 
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_alu.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_alu.hpp
@@ -266,23 +266,20 @@ using lookup_alu_shifts_two_pow_relation = lookup_relation_base<FF_, lookup_alu_
 struct lookup_alu_exec_dispatching_cast_settings_ {
     static constexpr std::string_view NAME = "LOOKUP_ALU_EXEC_DISPATCHING_CAST";
     static constexpr std::string_view RELATION_NAME = "alu";
-    static constexpr size_t LOOKUP_TUPLE_SIZE = 5;
+    static constexpr size_t LOOKUP_TUPLE_SIZE = 6;
     static constexpr Column SRC_SELECTOR = Column::execution_sel_execute_cast;
     static constexpr Column DST_SELECTOR = Column::alu_sel_op_truncate;
     static constexpr Column COUNTS = Column::lookup_alu_exec_dispatching_cast_counts;
     static constexpr Column INVERSES = Column::lookup_alu_exec_dispatching_cast_inv;
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> SRC_COLUMNS = {
-        ColumnAndShifts::execution_register_0_,
-        ColumnAndShifts::execution_rop_2_,
-        ColumnAndShifts::execution_subtrace_operation_id,
-        ColumnAndShifts::execution_register_1_,
-        ColumnAndShifts::execution_mem_tag_reg_1_
+        ColumnAndShifts::execution_register_0_,           ColumnAndShifts::execution_rop_2_,
+        ColumnAndShifts::execution_subtrace_operation_id, ColumnAndShifts::execution_register_1_,
+        ColumnAndShifts::execution_mem_tag_reg_1_,        ColumnAndShifts::execution_sel_opcode_error
+    };
+    static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = {
+        ColumnAndShifts::alu_ia, ColumnAndShifts::alu_ia_tag, ColumnAndShifts::alu_op_id,
+        ColumnAndShifts::alu_ic, ColumnAndShifts::alu_ia_tag, ColumnAndShifts::precomputed_zero
     };
-    static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = { ColumnAndShifts::alu_ia,
-                                                                                    ColumnAndShifts::alu_ia_tag,
-                                                                                    ColumnAndShifts::alu_op_id,
-                                                                                    ColumnAndShifts::alu_ic,
-                                                                                    ColumnAndShifts::alu_ia_tag };
 };
 
 using lookup_alu_exec_dispatching_cast_settings = lookup_settings<lookup_alu_exec_dispatching_cast_settings_>;
@@ -294,7 +291,7 @@ using lookup_alu_exec_dispatching_cast_relation = lookup_relation_base<FF_, look
 struct lookup_alu_exec_dispatching_set_settings_ {
     static constexpr std::string_view NAME = "LOOKUP_ALU_EXEC_DISPATCHING_SET";
     static constexpr std::string_view RELATION_NAME = "alu";
-    static constexpr size_t LOOKUP_TUPLE_SIZE = 5;
+    static constexpr size_t LOOKUP_TUPLE_SIZE = 6;
     static constexpr Column SRC_SELECTOR = Column::execution_sel_execute_set;
     static constexpr Column DST_SELECTOR = Column::alu_sel_op_truncate;
     static constexpr Column COUNTS = Column::lookup_alu_exec_dispatching_set_counts;
@@ -304,13 +301,13 @@ struct lookup_alu_exec_dispatching_set_settings_ {
         ColumnAndShifts::execution_rop_1_,
         ColumnAndShifts::execution_subtrace_operation_id,
         ColumnAndShifts::execution_register_0_,
-        ColumnAndShifts::execution_mem_tag_reg_0_
+        ColumnAndShifts::execution_mem_tag_reg_0_,
+        ColumnAndShifts::execution_sel_opcode_error
+    };
+    static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = {
+        ColumnAndShifts::alu_ia, ColumnAndShifts::alu_ia_tag, ColumnAndShifts::alu_op_id,
+        ColumnAndShifts::alu_ic, ColumnAndShifts::alu_ic_tag, ColumnAndShifts::precomputed_zero
     };
-    static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = { ColumnAndShifts::alu_ia,
-                                                                                    ColumnAndShifts::alu_ia_tag,
-                                                                                    ColumnAndShifts::alu_op_id,
-                                                                                    ColumnAndShifts::alu_ic,
-                                                                                    ColumnAndShifts::alu_ic_tag };
 };
 
 using lookup_alu_exec_dispatching_set_settings = lookup_settings<lookup_alu_exec_dispatching_set_settings_>;
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/sload.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/sload.hpp
@@ -38,16 +38,16 @@ template <typename FF> class sload : public Relation<sloadImpl<FF>> {
     {
         switch (index) {
         case 0:
-            return "SLOAD_SUCCESS";
-        case 1:
             return "SLOAD_FF_OUTPUT_TAG";
+        case 1:
+            return "SLOAD_SUCCESS";
         }
         return std::to_string(index);
     }
 
     // Subrelation indices constants, to be used in tests.
-    static constexpr size_t SR_SLOAD_SUCCESS = 0;
-    static constexpr size_t SR_SLOAD_FF_OUTPUT_TAG = 1;
+    static constexpr size_t SR_SLOAD_FF_OUTPUT_TAG = 0;
+    static constexpr size_t SR_SLOAD_SUCCESS = 1;
 };
 
 } // namespace bb::avm2
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/sload_impl.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/sload_impl.hpp
@@ -18,16 +18,16 @@ void sloadImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
 
     const auto constants_MEM_TAG_FF = FF(0);
 
-    { // SLOAD_SUCCESS
+    { // SLOAD_FF_OUTPUT_TAG
         using Accumulator = typename std::tuple_element_t<0, ContainerOverSubrelations>;
-        auto tmp = in.get(C::execution_sel_execute_sload) * in.get(C::execution_sel_opcode_error);
+        auto tmp =
+            in.get(C::execution_sel_execute_sload) * (constants_MEM_TAG_FF - in.get(C::execution_mem_tag_reg_1_));
         tmp *= scaling_factor;
         std::get<0>(evals) += typename Accumulator::View(tmp);
     }
-    { // SLOAD_FF_OUTPUT_TAG
+    { // SLOAD_SUCCESS
         using Accumulator = typename std::tuple_element_t<1, ContainerOverSubrelations>;
-        auto tmp =
-            in.get(C::execution_sel_execute_sload) * (constants_MEM_TAG_FF - in.get(C::execution_mem_tag_reg_1_));
+        auto tmp = in.get(C::execution_sel_execute_sload) * in.get(C::execution_sel_opcode_error);
         tmp *= scaling_factor;
         std::get<1>(evals) += typename Accumulator::View(tmp);
     }
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp
@@ -228,9 +228,6 @@ void Execution::shl(ContextInterface& context, MemoryAddress a_addr, MemoryAddre
         set_output(opcode, c);
     } catch (const AluException& e) {
         throw OpcodeExecutionException("SHL Exception: " + std::string(e.what()));
-    } catch (const std::exception& e) {
-        // We have some err not handled by TAG_ERROR (this is a coding error, we should not get here):
-        throw e;
     }
 }
 
@@ -250,9 +247,6 @@ void Execution::shr(ContextInterface& context, MemoryAddress a_addr, MemoryAddre
         set_output(opcode, c);
     } catch (const AluException& e) {
         throw OpcodeExecutionException("SHR Exception: " + std::string(e.what()));
-    } catch (const std::exception& e) {
-        // We have some err not handled by TAG_ERROR (this is a coding error, we should not get here):
-        throw e;
     }
 }
 
diff --git a/barretenberg/cpp/src/barretenberg/vm2/tracegen/execution_trace.cpp b/barretenberg/cpp/src/barretenberg/vm2/tracegen/execution_trace.cpp

Original file line number	Diff line number	Diff line change
`@@ -39,8 +39,7 @@ void contextImpl<FF_>::accumulate(ContainerOverSubrelations& evals,`
`39`	`39`	`{`
`40`	`40`	`using Accumulator = typename std::tuple_element_t<0, ContainerOverSubrelations>;`
`41`	`41`	`auto tmp = (in.get(C::execution_sel_enter_call) -`
`42`		`- (in.get(C::execution_sel_execute_call) + in.get(C::execution_sel_execute_static_call)) *`
`43`		`- (FF(1) - in.get(C::execution_sel_error)));`
	`42`	`+ (in.get(C::execution_sel_execute_call) + in.get(C::execution_sel_execute_static_call)));`
`44`	`43`	`tmp *= scaling_factor;`
`45`	`44`	`std::get<0>(evals) += typename Accumulator::View(tmp);`
`46`	`45`	`}`