fix(avm): addressing claude review of interaction builders code

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/interaction_builder.cpp

@@ -8,20 +8,25 @@ namespace bb::avm2::tracegen {
 
 void order_jobs_by_destination_columns(std::vector<std::unique_ptr<InteractionBuilderInterface>>& jobs)
 {
-    // Identify first occurrences of each fingerprint.
+    // Tag each job with whether its destination-columns fingerprint is being seen for the first
+    // time. The tag is captured up front so the partition predicate doesn't depend on the
+    // unique_ptrs, which the partition implementation may null out via moves.
     unordered_flat_set<size_t> seen_fingerprints;
-    unordered_flat_set<InteractionBuilderInterface*> first_occurrence_jobs;
+    std::vector<std::pair<bool, std::unique_ptr<InteractionBuilderInterface>>> tagged;
+    tagged.reserve(jobs.size());
 
-    for (const auto& job : jobs) {
+    for (auto& job : jobs) {
         auto fp = job->get_destination_columns_fingerprint();
         auto [_, inserted] = seen_fingerprints.insert(fp);
-        if (inserted) {
-            first_occurrence_jobs.insert(job.get());
-        }
+        tagged.emplace_back(inserted, std::move(job));
     }
 
     // Stable partition: first occurrences come first.
-    std::ranges::stable_partition(jobs, [&](const auto& job) { return first_occurrence_jobs.contains(job.get()); });
+    std::ranges::stable_partition(tagged, [](const auto& t) { return t.first; });
+
+    for (size_t i = 0; i < tagged.size(); ++i) {
+        jobs[i] = std::move(tagged[i].second);
+    }
 }
 
 } // namespace bb::avm2::tracegen

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/interaction_builder.hpp

@@ -1,9 +1,13 @@
 #pragma once
 
+#include <array>
 #include <memory>
 #include <optional>
+#include <utility>
 #include <vector>
 
+#include "barretenberg/common/tuple.hpp"
+#include "barretenberg/vm2/common/field.hpp"
 #include "barretenberg/vm2/generated/columns.hpp"
 #include "barretenberg/vm2/tracegen/lib/shared_index_cache.hpp"
 #include "barretenberg/vm2/tracegen/trace_container.hpp"
@@ -20,6 +24,20 @@ template <size_t N, size_t... Is> struct RefTupleHelper<N, std::index_sequence<I
 } // namespace detail
 template <size_t N> using RefTuple = typename detail::RefTupleHelper<N>::type;
 
+// Same as TraceContainer::get_multiple but copies the values into an owning std::array. Use when
+// the result must outlive a subsequent column write (e.g. when stored as a hash-map key): the
+// references returned by get_multiple point into column storage and can dangle on reallocation.
+template <size_t N>
+std::array<FF, N> get_multiple_as_array(const TraceContainer& trace,
+                                        const std::array<ColumnAndShifts, N>& cols,
+                                        uint32_t row)
+{
+    auto refs = trace.get_multiple(cols, row);
+    return [&]<size_t... Is>(std::index_sequence<Is...>) {
+        return std::array<FF, N>{ flat_tuple::get<Is>(refs)... };
+    }(std::make_index_sequence<N>{});
+}
+
 class InteractionBuilderInterface {
   public:
     virtual ~InteractionBuilderInterface() = default;
@@ -33,8 +51,9 @@ class InteractionBuilderInterface {
 // A concatenate that works with movable objects.
 template <typename T> std::vector<T> concatenate_jobs(std::vector<T>&& first, auto&&... rest)
 {
+    const size_t total_size = first.size() + (rest.size() + ...);
     std::vector<T> result = std::move(first);
-    result.reserve(first.size() + (rest.size() + ...));
+    result.reserve(total_size);
     (std::move(rest.begin(), rest.end(), std::back_inserter(result)), ...);
     return result;
 }

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/interaction_def.hpp

@@ -5,10 +5,11 @@
 #include <unordered_map>
 #include <vector>
 
+#include "barretenberg/common/assert.hpp"
 #include "barretenberg/vm2/tracegen/lib/interaction_builder.hpp"
 #include "barretenberg/vm2/tracegen/lib/lookup_builder.hpp"
 #include "barretenberg/vm2/tracegen/lib/lookup_into_bitwise.hpp"
-#include "barretenberg/vm2/tracegen/lib/lookup_into_indexed_by_clk.hpp"
+#include "barretenberg/vm2/tracegen/lib/lookup_into_indexed_by_row.hpp"
 #include "barretenberg/vm2/tracegen/lib/lookup_into_p_decomposition.hpp"
 #include "barretenberg/vm2/tracegen/lib/multi_permutation_builder.hpp"
 #include "barretenberg/vm2/tracegen/lib/permutation_builder.hpp"
@@ -34,8 +35,12 @@ class InteractionDefinition {
     template <InteractionType type, typename... InteractionSettings> InteractionDefinition& add(auto&&... args)
     {
         std::string name = (std::string(InteractionSettings::NAME) + ...);
-        interactions[name] =
-            get_interaction_factory<type, InteractionSettings...>(std::forward<decltype(args)>(args)...);
+        auto [_, inserted] = interactions.emplace(
+            name, get_interaction_factory<type, InteractionSettings...>(std::forward<decltype(args)>(args)...));
+
+        // Safeguard detecting a collision in the interaction names (we do not use separators to have an injective
+        // serialization).
+        BB_ASSERT_DEBUG(inserted, "InteractionDefinition::add: collision in interaction name: " + name);
         return *this;
     }
 

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/multi_permutation_builder.hpp

@@ -55,7 +55,7 @@ template <typename... PermutationSettings_> class MultiPermutationBuilder : publ
     {
         // Find each source tuple in the destination table, and set a 1 in the destination selector.
         trace.visit_column(PermutationSettings::SRC_SELECTOR, [&](uint32_t row, const FF&) {
-            auto src_values = trace.get_multiple(PermutationSettings::SRC_COLUMNS, row);
+            auto src_values = get_multiple_as_array(trace, PermutationSettings::SRC_COLUMNS, row);
             auto index_it = row_idx.find(src_values);
             if (index_it == row_idx.end() || index_it->second.empty()) {
                 throw std::runtime_error("Failed setting selectors for " + std::string(PermutationSettings::NAME) +
@@ -82,8 +82,7 @@ template <typename... PermutationSettings_> class MultiPermutationBuilder : publ
         // and add the row number to the index. See the comment on `row_idx` for more details.
         row_idx.reserve(trace.get_column_rows(dst_table_selector));
         trace.visit_column(dst_table_selector, [&](uint32_t row, const FF&) {
-            auto dst_values = trace.get_multiple(DST_COLUMNS, row);
-            row_idx[dst_values].push_back(row);
+            row_idx[get_multiple_as_array(trace, DST_COLUMNS, row)].push_back(row);
         });
     }
 
@@ -97,11 +96,12 @@ template <typename... PermutationSettings_> class MultiPermutationBuilder : publ
     // We need an extra "destination TABLE selector" which is the selector of the whole table.
     Column dst_table_selector;
 
-    // In a permutation (or lookup) you are trying to find a src suple of values
+    // In a permutation (or lookup) you are trying to find a src tuple of values
     // (a, b, c, ...) in some destination table. That is, you want a row number in the destination table.
     // The following map contains (a, b, c, ...) -> [row_number_1, row_number_2, ...].
     // That is, you can efficiently find all the rows in the destination table that match the src tuple.
-    unordered_flat_map<RefTuple<DST_COLUMNS.size()>, /*rows*/ std::vector<uint32_t>> row_idx;
+    // Keys are stored as owning value arrays (see get_multiple_as_array in interaction_builder.hpp for rationale).
+    unordered_flat_map<std::array<FF, COLUMNS_PER_SET>, /*rows*/ std::vector<uint32_t>> row_idx;
 };
 
 } // namespace bb::avm2::tracegen

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/shared_index_cache.hpp

@@ -80,7 +80,21 @@ class SharedIndexCache {
             promise->set_value(index);
             return *index;
         } catch (...) {
-            promise->set_exception(std::current_exception());
+            // Evict the failed entry so a subsequent get_or_build can retry rather than
+            // re-throwing the cached exception forever (and broadcasting it to all jobs
+            // that share this destination).
+            {
+                std::unique_lock lock(mutex_);
+                cache_.erase(key);
+            }
+            // Wake any threads already waiting on this future with the build error.
+            // Swallow any secondary failure here (e.g. promise_already_satisfied if
+            // set_value partially completed) so the original exception always
+            // propagates and we never leave waiters blocked.
+            try {
+                promise->set_exception(std::current_exception());
+            } catch (...) {
+            }
             throw;
         }
     }

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/test_interaction_builder.hpp

@@ -53,7 +53,7 @@ template <typename BaseBuilder> class AddChecksToBuilder : public BaseBuilder {
 template <typename PermutationSettings>
 class CheckingPermutationBuilder : public PermutationBuilder<PermutationSettings> {
   public:
-    // Use array for storage in map keys (tuples of references can't be stored).
+    // Owning value array; see get_multiple_as_array in interaction_builder.hpp for why we can't key on RefTuple.
     using ArrayTuple = std::array<FF, PermutationSettings::COLUMNS_PER_SET>;
 
     void process(TraceContainer& trace) override
@@ -63,13 +63,11 @@ class CheckingPermutationBuilder : public PermutationBuilder<PermutationSettings
         // Collect the source and destination tuples.
         source_tuples.clear();
         trace.visit_column(PermutationSettings::SRC_SELECTOR, [&](uint32_t row, const FF&) {
-            auto src_values = trace.get_multiple(PermutationSettings::SRC_COLUMNS, row);
-            source_tuples[to_array(src_values)].insert(row);
+            source_tuples[get_multiple_as_array(trace, PermutationSettings::SRC_COLUMNS, row)].insert(row);
         });
         destination_tuples.clear();
         trace.visit_column(PermutationSettings::DST_SELECTOR, [&](uint32_t row, const FF&) {
-            auto dst_values = trace.get_multiple(PermutationSettings::DST_COLUMNS, row);
-            destination_tuples[to_array(dst_values)].insert(row);
+            destination_tuples[get_multiple_as_array(trace, PermutationSettings::DST_COLUMNS, row)].insert(row);
         });
 
         auto build_error_message =
@@ -115,14 +113,6 @@ class CheckingPermutationBuilder : public PermutationBuilder<PermutationSettings
     }
 
   private:
-    // Helper to convert tuple of references to array for storage.
-    template <typename... Ts> static ArrayTuple to_array(const flat_tuple::tuple<Ts...>& tup)
-    {
-        return [&]<size_t... Is>(std::index_sequence<Is...>) {
-            return ArrayTuple{ flat_tuple::get<Is>(tup)... };
-        }(std::make_index_sequence<sizeof...(Ts)>{});
-    }
-
     unordered_flat_map<ArrayTuple, std::unordered_set<uint32_t>> source_tuples;
     unordered_flat_map<ArrayTuple, std::unordered_set<uint32_t>> destination_tuples;
 };