address comments from peer review

Author: sirasistant

barretenberg/cpp/pil/vm2/bytecode/bc_retrieval.pil

@@ -97,22 +97,22 @@ pol commit remaining_bytecodes_inv;
 #[NO_REMAINING_BYTECODES]
 sel * (REMAINING_BYTECODES * (no_remaining_bytecodes * (1 - remaining_bytecodes_inv) + remaining_bytecodes_inv) - 1 + no_remaining_bytecodes) = 0;
 
-pol commit new_bytecode;
+pol commit is_new_class;
 
-#[NEW_BYTECODE_CHECK]
+#[IS_NEW_CLASS_CHECK]
 instance_exists {
     current_class_id,
-    new_bytecode,
+    is_new_class,
     prev_retrieved_bytecodes_tree_root
 } in retrieved_bytecodes_tree_check.sel {
     retrieved_bytecodes_tree_check.class_id,
     retrieved_bytecodes_tree_check.leaf_not_exists,
     retrieved_bytecodes_tree_check.root
 };
 
-sel * (1 - instance_exists) * new_bytecode = 0;
+sel * (1 - instance_exists) * is_new_class = 0;
 
-pol TOO_MANY_BYTECODES = no_remaining_bytecodes * new_bytecode;
+pol TOO_MANY_BYTECODES = no_remaining_bytecodes * is_new_class;
 
 // We error if instance doesn't exist or if we have too many bytecodes.
 sel * (instance_exists * (1 - TOO_MANY_BYTECODES) - (1 - error)) = 0;

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/bc_retrieval.test.cpp

@@ -73,7 +73,7 @@ TEST(BytecodeRetrievalConstrainingTest, SuccessfulRetrieval)
                                   .public_data_tree_root = public_data_tree_root,
                                   .retrieved_bytecodes_snapshot_before = snapshot_before,
                                   .retrieved_bytecodes_snapshot_after = snapshot_after,
-                                  .new_bytecode = true,
+                                  .is_new_class = true,
                               } },
                               trace);
 
@@ -114,7 +114,7 @@ TEST(BytecodeRetrievalConstrainingTest, TooManyBytecodes)
                                   .public_data_tree_root = public_data_tree_root,
                                   .retrieved_bytecodes_snapshot_before = snapshot_before,
                                   .retrieved_bytecodes_snapshot_after = snapshot_after,
-                                  .new_bytecode = true,
+                                  .is_new_class = true,
                                   .limit_error = true,
                               } },
                               trace);

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/storage_write.test.cpp

@@ -212,7 +212,7 @@ TEST(SStoreConstrainingTest, Interactions)
         .root = 42,
         .nextAvailableLeafIndex = 128,
     };
-    AppendOnlyTreeSnapshot written_slots_tree_before = written_public_data_slots_tree_check.snapshot();
+    AppendOnlyTreeSnapshot written_slots_tree_before = written_public_data_slots_tree_check.get_snapshot();
 
     EXPECT_CALL(poseidon2, hash(_)).WillRepeatedly([](const std::vector<FF>& inputs) {
         return RawPoseidon2::hash(inputs);
@@ -242,7 +242,7 @@ TEST(SStoreConstrainingTest, Interactions)
                                                                {},
                                                                false);
     written_public_data_slots_tree_check.insert(contract_address, slot);
-    auto written_slots_tree_after = written_public_data_slots_tree_check.snapshot();
+    auto written_slots_tree_after = written_public_data_slots_tree_check.get_snapshot();
 
     TestTraceContainer trace({
         {

barretenberg/cpp/src/barretenberg/vm2/generated/columns.hpp

@@ -250,7 +250,7 @@ struct AvmFlavorVariables {
         lookup_bc_hashing_iv_is_len_relation<FF_>,
         lookup_bc_retrieval_class_id_derivation_relation<FF_>,
         lookup_bc_retrieval_contract_instance_retrieval_relation<FF_>,
-        lookup_bc_retrieval_new_bytecode_check_relation<FF_>,
+        lookup_bc_retrieval_is_new_class_check_relation<FF_>,
         lookup_bc_retrieval_retrieved_bytecodes_insertion_relation<FF_>,
         lookup_bitwise_byte_operations_relation<FF_>,
         lookup_bitwise_dispatch_exec_bitwise_relation<FF_>,

barretenberg/cpp/src/barretenberg/vm2/generated/flavor_variables.hpp

@@ -22,7 +22,7 @@ void bc_retrievalImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
                                                     constants_AVM_RETRIEVED_BYTECODES_TREE_INITIAL_SIZE) -
                                                    in.get(C::bc_retrieval_prev_retrieved_bytecodes_tree_size));
     const auto bc_retrieval_TOO_MANY_BYTECODES =
-        in.get(C::bc_retrieval_no_remaining_bytecodes) * in.get(C::bc_retrieval_new_bytecode);
+        in.get(C::bc_retrieval_no_remaining_bytecodes) * in.get(C::bc_retrieval_is_new_class);
 
     {
         using Accumulator = typename std::tuple_element_t<0, ContainerOverSubrelations>;
@@ -64,7 +64,7 @@ void bc_retrievalImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     {
         using Accumulator = typename std::tuple_element_t<5, ContainerOverSubrelations>;
         auto tmp = in.get(C::bc_retrieval_sel) * (FF(1) - in.get(C::bc_retrieval_instance_exists)) *
-                   in.get(C::bc_retrieval_new_bytecode);
+                   in.get(C::bc_retrieval_is_new_class);
         tmp *= scaling_factor;
         std::get<5>(evals) += typename Accumulator::View(tmp);
     }

barretenberg/cpp/src/barretenberg/vm2/generated/relations/bc_retrieval_impl.hpp

@@ -26,7 +26,7 @@
 namespace bb::avm2 {
 
 INSTANTIATE_LOOKUP(lookup_bc_retrieval_contract_instance_retrieval_relation);
-INSTANTIATE_LOOKUP(lookup_bc_retrieval_new_bytecode_check_relation);
+INSTANTIATE_LOOKUP(lookup_bc_retrieval_is_new_class_check_relation);
 INSTANTIATE_LOOKUP(lookup_bc_retrieval_class_id_derivation_relation);
 INSTANTIATE_LOOKUP(lookup_bc_retrieval_retrieved_bytecodes_insertion_relation);
 

barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_bc_retrieval.cpp

@@ -43,19 +43,19 @@ template <typename FF_>
 using lookup_bc_retrieval_contract_instance_retrieval_relation =
     lookup_relation_base<FF_, lookup_bc_retrieval_contract_instance_retrieval_settings>;
 
-/////////////////// lookup_bc_retrieval_new_bytecode_check ///////////////////
+/////////////////// lookup_bc_retrieval_is_new_class_check ///////////////////
 
-struct lookup_bc_retrieval_new_bytecode_check_settings_ {
-    static constexpr std::string_view NAME = "LOOKUP_BC_RETRIEVAL_NEW_BYTECODE_CHECK";
+struct lookup_bc_retrieval_is_new_class_check_settings_ {
+    static constexpr std::string_view NAME = "LOOKUP_BC_RETRIEVAL_IS_NEW_CLASS_CHECK";
     static constexpr std::string_view RELATION_NAME = "bc_retrieval";
     static constexpr size_t LOOKUP_TUPLE_SIZE = 3;
     static constexpr Column SRC_SELECTOR = Column::bc_retrieval_instance_exists;
     static constexpr Column DST_SELECTOR = Column::retrieved_bytecodes_tree_check_sel;
-    static constexpr Column COUNTS = Column::lookup_bc_retrieval_new_bytecode_check_counts;
-    static constexpr Column INVERSES = Column::lookup_bc_retrieval_new_bytecode_check_inv;
+    static constexpr Column COUNTS = Column::lookup_bc_retrieval_is_new_class_check_counts;
+    static constexpr Column INVERSES = Column::lookup_bc_retrieval_is_new_class_check_inv;
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> SRC_COLUMNS = {
         ColumnAndShifts::bc_retrieval_current_class_id,
-        ColumnAndShifts::bc_retrieval_new_bytecode,
+        ColumnAndShifts::bc_retrieval_is_new_class,
         ColumnAndShifts::bc_retrieval_prev_retrieved_bytecodes_tree_root
     };
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = {
@@ -65,11 +65,11 @@ struct lookup_bc_retrieval_new_bytecode_check_settings_ {
     };
 };
 
-using lookup_bc_retrieval_new_bytecode_check_settings =
-    lookup_settings<lookup_bc_retrieval_new_bytecode_check_settings_>;
+using lookup_bc_retrieval_is_new_class_check_settings =
+    lookup_settings<lookup_bc_retrieval_is_new_class_check_settings_>;
 template <typename FF_>
-using lookup_bc_retrieval_new_bytecode_check_relation =
-    lookup_relation_base<FF_, lookup_bc_retrieval_new_bytecode_check_settings>;
+using lookup_bc_retrieval_is_new_class_check_relation =
+    lookup_relation_base<FF_, lookup_bc_retrieval_is_new_class_check_settings>;
 
 /////////////////// lookup_bc_retrieval_class_id_derivation ///////////////////
 

barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_bc_retrieval.hpp

@@ -14,79 +14,69 @@ namespace bb::avm2::simulation {
 
 BytecodeId TxBytecodeManager::get_bytecode(const AztecAddress& address)
 {
+
     // Use shared ContractInstanceManager for contract instance retrieval and validation
     // This handles nullifier checks, address derivation, and update validation
-    AppendOnlyTreeSnapshot snapshot_before = retrieved_bytecodes_tree_check.snapshot();
     auto tree_states = merkle_db.get_tree_state();
+    AppendOnlyTreeSnapshot before_snapshot = retrieved_bytecodes_tree_check.get_snapshot();
+
+    BytecodeRetrievalEvent retrieval_event = {
+        .bytecode_id = FF(0), // Use default ID for error cases
+        .address = address,
+        .nullifier_root = tree_states.nullifierTree.tree.root,
+        .public_data_tree_root = tree_states.publicDataTree.tree.root,
+        .retrieved_bytecodes_snapshot_before = before_snapshot,
+        .retrieved_bytecodes_snapshot_after = before_snapshot,
+    };
 
     auto maybe_instance = contract_instance_manager.get_contract_instance(address);
 
     if (!maybe_instance.has_value()) {
+        retrieval_event.instance_not_found_error = true;
         // Contract instance not found - emit error event and throw
-        retrieval_events.emit({
-            .bytecode_id = FF(0), // Use default ID for error case
-            .address = address,
-            .nullifier_root = tree_states.nullifierTree.tree.root,
-            .public_data_tree_root = tree_states.publicDataTree.tree.root,
-            .retrieved_bytecodes_snapshot_before = snapshot_before,
-            .retrieved_bytecodes_snapshot_after = snapshot_before,
-            .instance_not_found_error = true,
-        });
+        retrieval_events.emit(std::move(retrieval_event));
         vinfo("Contract ", field_to_string(address), " is not deployed!");
-        throw BytecodeNotFoundError("Contract " + field_to_string(address) + " is not deployed");
+        throw BytecodeRetrievalError("Contract " + field_to_string(address) + " is not deployed");
     }
 
     ContractInstance instance = maybe_instance.value();
     ContractClassId current_class_id = instance.current_class_id;
+    retrieval_event.current_class_id = current_class_id;
+
+    bool is_new_class = !retrieved_bytecodes_tree_check.contains(current_class_id);
+    retrieval_event.is_new_class = is_new_class;
 
-    bool new_bytecode = !retrieved_bytecodes_tree_check.contains(current_class_id);
     uint32_t retrieved_bytecodes_count = retrieved_bytecodes_tree_check.size();
 
-    if (new_bytecode && retrieved_bytecodes_count == MAX_PUBLIC_CALLS_TO_UNIQUE_CONTRACT_CLASS_IDS) {
-        retrieval_events.emit({
-            .bytecode_id = FF(0), // Use default ID for error case
-            .address = address,
-            .current_class_id = current_class_id,
-            .nullifier_root = tree_states.nullifierTree.tree.root,
-            .public_data_tree_root = tree_states.publicDataTree.tree.root,
-            .retrieved_bytecodes_snapshot_before = snapshot_before,
-            .retrieved_bytecodes_snapshot_after = snapshot_before,
-            .new_bytecode = new_bytecode,
-            .limit_error = true,
-        });
-        throw BytecodeRetrievalLimitReachedError("Can't retrieve more than " +
-                                                 std::to_string(MAX_PUBLIC_CALLS_TO_UNIQUE_CONTRACT_CLASS_IDS) +
-                                                 " bytecodes per tx");
+    if (is_new_class && retrieved_bytecodes_count >= MAX_PUBLIC_CALLS_TO_UNIQUE_CONTRACT_CLASS_IDS) {
+        retrieval_event.limit_error = true;
+        retrieval_events.emit(std::move(retrieval_event));
+        throw BytecodeRetrievalError("Can't retrieve more than " +
+                                     std::to_string(MAX_PUBLIC_CALLS_TO_UNIQUE_CONTRACT_CLASS_IDS) +
+                                     " bytecodes per tx");
     }
 
     retrieved_bytecodes_tree_check.insert(current_class_id);
-    AppendOnlyTreeSnapshot snapshot_after = retrieved_bytecodes_tree_check.snapshot();
+    AppendOnlyTreeSnapshot snapshot_after = retrieved_bytecodes_tree_check.get_snapshot();
+    retrieval_event.retrieved_bytecodes_snapshot_after = snapshot_after;
 
     // Contract class retrieval and class ID validation
     std::optional<ContractClass> maybe_klass = contract_db.get_contract_class(current_class_id);
     // Note: we don't need to silo and check the class id because the deployer contract guarrantees
     // that if a contract instance exists, the class has been registered.
     assert(maybe_klass.has_value());
     auto& klass = maybe_klass.value();
+    retrieval_event.contract_class = klass; // WARNING: this class has the whole bytecode.
     info("Bytecode for ", address, " successfully retrieved!");
 
     // Bytecode hashing and decomposition, deduplicated by bytecode_id (commitment)
     BytecodeId bytecode_id = klass.public_bytecode_commitment;
+    retrieval_event.bytecode_id = bytecode_id;
 
     // Check if we've already processed this bytecode. If so, don't do hashing and decomposition again!
     if (bytecodes.contains(bytecode_id)) {
         // Already processed this bytecode - just emit retrieval event and return
-        retrieval_events.emit({
-            .bytecode_id = bytecode_id,
-            .address = address,
-            .current_class_id = current_class_id,
-            .contract_class = klass,
-            .nullifier_root = tree_states.nullifierTree.tree.root,
-            .public_data_tree_root = tree_states.publicDataTree.tree.root,
-            .retrieved_bytecodes_snapshot_before = snapshot_before,
-            .retrieved_bytecodes_snapshot_after = snapshot_after,
-            .new_bytecode = new_bytecode,
-        });
+        retrieval_events.emit(std::move(retrieval_event));
         return bytecode_id;
     }
 
@@ -102,17 +92,7 @@ BytecodeId TxBytecodeManager::get_bytecode(const AztecAddress& address)
     // We now save the bytecode so that we don't repeat this process.
     bytecodes.emplace(bytecode_id, std::move(shared_bytecode));
 
-    retrieval_events.emit({
-        .bytecode_id = bytecode_id,
-        .address = address,
-        .current_class_id = current_class_id,
-        .contract_class = klass, // WARNING: this class has the whole bytecode.
-        .nullifier_root = tree_states.nullifierTree.tree.root,
-        .public_data_tree_root = tree_states.publicDataTree.tree.root,
-        .retrieved_bytecodes_snapshot_before = snapshot_before,
-        .retrieved_bytecodes_snapshot_after = snapshot_after,
-        .new_bytecode = new_bytecode,
-    });
+    retrieval_events.emit(std::move(retrieval_event));
 
     return bytecode_id;
 }

barretenberg/cpp/src/barretenberg/vm2/simulation/bytecode_manager.cpp

@@ -24,14 +24,8 @@
 
 namespace bb::avm2::simulation {
 
-struct BytecodeNotFoundError : public std::runtime_error {
-    BytecodeNotFoundError(const std::string& message)
-        : std::runtime_error(message)
-    {}
-};
-
-struct BytecodeRetrievalLimitReachedError : public std::runtime_error {
-    BytecodeRetrievalLimitReachedError(const std::string& message)
+struct BytecodeRetrievalError : public std::runtime_error {
+    BytecodeRetrievalError(const std::string& message)
         : std::runtime_error(message)
     {}
 };