chore: audit Permutation argument 2 #18535
Conversation
barretenberg/cpp/src/barretenberg/honk/composer/permutation_lib.hpp
Outdated
Show resolved
Hide resolved
notnotraju
added
ci-full
notnotraju
changed the title
ActiveRegionData, a remnant of the structured trace
notnotraju
changed the title
| {} | ||
| }; | ||
|
|
||
| template <size_t NUM_WIRES, bool generalized> struct PermutationMapping { |
There was a problem hiding this comment.
generalized toggled whether we compute the id polynomials or not. This was controlled by an IsUltraOrMega<Flavor>, which is all we'll use this for, so I think the logic is simpler when we remove this template parameter.
barretenberg/cpp/src/barretenberg/honk/composer/permutation_lib.hpp
Outdated
Show resolved
Hide resolved
| auto row_idx = get_active_range_poly_idx(i); | ||
| if constexpr (IsUltraOrMegaHonk<Flavor>) { | ||
| row = full_polynomials.get_row_for_permutation_arg(row_idx); | ||
| row = full_polynomials.get_row_for_permutation_arg(i); |
There was a problem hiding this comment.
is any other flavor using this method?
There was a problem hiding this comment.
I think ECCVM and Translator? (E.g., see eccvm_prover.cpp and translator_prover.cpp)
There was a problem hiding this comment.
yeah, translator_flavor doesn't have the sigma wires, and has a different compute_grand_product_numerator (resp. denominator) method. In particular, there is no get_row_for_permutation_arg, nor do I think there could be without changing the translator itself. I think we therefore do need the IsUltraOrMega concept :-/
| // For Ultra/Mega, the first row is an inactive zero row thus the grand prod takes value 1 at both i = 0 and i = 1 | ||
| // For Ultra/Mega, the first row is a zero row thus the grand prod takes value 1 at both i = 0 and i = 1 | ||
| if constexpr (IsUltraOrMegaHonk<Flavor>) { | ||
| grand_product_polynomial.at(1) = 1; |
There was a problem hiding this comment.
it's the same 1 that was confusing in permutation_lib.hpp. can we make sure we are using this consistently?
There was a problem hiding this comment.
z_perm.at(1) == 1 follows automatically for ultra/mega, so just removed.
There was a problem hiding this comment.
(as we aren't killing IsUltraOrMega, I could add this as a constexpr conditional BB_ASSERT_EQ; I initially thought grand_product_polynomial.at(1) == 1 was true for all flavors, and I put in an assert, but I got an error in one of the noir protocol circuits.)
iakovenkos
left a comment
iakovenkos
left a comment
There was a problem hiding this comment.
please make the usage of z_perm start index consistent across different methods + make sure we still need IsUltraOrMega concept
| using CyclicPermutation = std::vector<cycle_node>; | ||
|
|
||
| namespace { | ||
|
|
There was a problem hiding this comment.
for easier reasoning-about-indices
🧾 Audit Context
Remove dead code, mostly having to do with the structured trace.
🛠️ Changes Made
✅ Checklist
📌 Notes for Reviewers