fix(avm): addressing claude review of interaction builders code#23431
Merged
jeanmon merged 7 commits intoMay 21, 2026
Merged
Conversation
jeanmon
force-pushed
the
jean/avm-23-review-interaction-builder-related-code
branch
from
7e2efeb to
b343c54
Compare
fcarreiro
approved these changes
May 20, 2026
| // outlive a subsequent column write (e.g. when used as a hash-map key) — references returned | ||
| // by get_multiple point into the column storage and can dangle on reallocation. | ||
| template <size_t N> | ||
| std::array<FF, N> get_multiple_as_array(const std::array<ColumnAndShifts, N>& cols, uint32_t row) const |
Contributor
There was a problem hiding this comment.
If possible I prefer this to be a helper somewhere else (e.g., in the interaction builder), that uses get_multiple(). Claude can probably do it, but if it's too much of a problem then this is ok.
Contributor
Author
There was a problem hiding this comment.
Done. It is a good suggestion to change the interface of trace_container.
Contributor
There was a problem hiding this comment.
I guess these changes are ok, but keep an eye on the interactions benchmarks just in case.
added 7 commits
May 21, 2026 07:38
row_idx previously keyed on RefTuple<N> (a tuple of const FF&) pointing into ankerl::unordered_dense column storage. That storage reallocates on insert, so any later column write could leave the keys dangling and silently corrupt destination-selector assignment. Switch to value-owning std::array<FF, N> keys, matching the convention already established in CheckingPermutationBuilder. Add a free helper get_multiple_as_array in interaction_builder.hpp that wraps TraceContainer::get_multiple and copies the resulting refs into an owning std::array.
…inition::add Since interaction names are formed by concatenating settings names without separators, two different setting packs could produce the same key and silently overwrite each other via operator[]. Switch to emplace and assert the insert actually happened, so collisions surface in debug builds.
first.size() was being read after std::move(first), when first is in a valid-but-unspecified state and typically reports size 0. The reserve therefore undercounted and didn't actually pre-allocate the final capacity. Compute total_size before the move so reserve does its job.
…safe stable_partition's predicate evaluated job.get() on each element. In principle the partition implementation is allowed to call the predicate after moving an element, at which point unique_ptr::get() returns nullptr and the job is misclassified as not-first-occurrence. Capture the is-first-occurrence flag up front by tagging each job with the bool returned by seen_fingerprints.insert, and partition on the tag instead. Folds the previous two passes over jobs into one and drops the intermediate pointer-keyed set.
…ontents The file defines LookupIntoIndexedByRow and is included via the LookupIntoIndexedByRow enum value, but the filename still referenced the old "clk" naming. Rename it to lookup_into_indexed_by_row.hpp.
When build_fn threw, the shared_future stayed in the cache holding the exception. Every subsequent get_or_build for that key hit the cache and re-threw the cached exception forever, and the same exception was broadcast to every concurrent waiter. Erase the entry on failure so the next caller can retry, and guard the catch-handler's set_exception so a secondary throw cannot strand waiters.
jeanmon
force-pushed
the
jean/avm-23-review-interaction-builder-related-code
branch
from
b343c54 to
7b05f74
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detailed of the report is in the following gist: https://gist.github.com/AztecBot/b8be80df63afe353b576d5e9b13e70f2
We applied some mitigations for H1, M2, L1, L2, L4, L5