chore: biggroup audit of lookup and rom tables by suyash67 · Pull Request #16895 · AztecProtocol/aztec-packages

suyash67 · 2025-09-09T12:25:53Z

🧾 Audit Context

Cleanup and audit of the lookup tables used in biggroup class. This PR does not change any logic, its mainly structural changes, cleanup and documentation.

🛠️ Changes Made

Added a README to explain how lookup tables are being used in biggroup, and a brief summary of how ROM tables work in barretenberg circuits.
Removed unused methods/structs like: lookup_table_base, batch_lookup_table_base, create_endo_pair_five_lookup_table.
Function comments + fix clang warnings + avoid hard-coded values.
No change to circuit, so no VK changes expected.

✅ Checklist

Audited all methods of the relevant module/class
Audited the interface of the module/class with other (relevant) components
Documented existing functionality and any changes made (as per Doxygen requirements)
Resolved and/or closed all issues/TODOs pertaining to the audited files
Confirmed and documented any security or other issues found (if applicable)
Verified that tests cover all critical paths (and added tests if necessary)
Updated audit tracking for the files audited (check the start of each file you audited)

📌 Notes for Reviewers

NA

…d handle it.

suyash67 · 2025-09-12T11:31:38Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

-        std::array<uint256_t, 8> limb_max; // tracks the maximum limb size represented in each element_table entry
+
+        // Each coordinate is an Fq element, which has 4 binary basis limbs and 1 prime basis limb
+        std::array<twin_rom_table<Builder>, Fq::NUM_LIMBS + 1> coordinates;


Using Fq::NUM_LIMBS instead of hard-coded constants

suyash67 · 2025-09-12T11:32:05Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

        bool use_endomorphism;
    };

    static std::pair<four_bit_table_plookup, four_bit_table_plookup> create_endo_pair_four_bit_table_plookup(


Moved the implementation of this function to biggroup_tables.hpp

suyash67 · 2025-09-12T11:32:50Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

-     * The table KEY is 3 1-bit NAF entries that correspond to scalar multipliers for
-     * base points A, B, C
-     **/
-    template <size_t length> struct lookup_table_base {


lookup_table_base struct isn't used anywhere, its probably a remnant from the ultra-standard era.

suyash67 · 2025-09-12T11:33:23Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

-     * Creates a pair of 5-bit lookup tables, the former corresponding to 5 input points,
-     * the latter corresponding to the endomorphism equivalent of the 5 input points (e.g. x -> \beta * x, y -> -y)
-     **/
-    static std::pair<lookup_table_plookup<5>, lookup_table_plookup<5>> create_endo_pair_five_lookup_table(


create_endo_pair_five_lookup_table isn't being used anywhere, so deleted it.

suyash67 · 2025-09-12T11:36:06Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

-     * Helper class to split a set of points into lookup table subsets
-     *
-     **/
-    struct batch_lookup_table_base {


This struct batch_lookup_table_base also isn't being used anywhere. We now have lookup_table_plookup and batch_lookup_table_plookup which help track all ROM lookup tables used in scalar multiplication in biggroup.

suyash67 · 2025-09-12T11:36:43Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp

        Fq neg_lambda = Fq::msub_div({ x }, { (two_x + x) }, (y + y), { a });
        Fq x_3 = neg_lambda.sqradd({ -(two_x) });
        Fq y_3 = neg_lambda.madd(x_3 - x, { -y });
+        // TODO: do we handle the point at infinity case here?


This TODO will be addressed in upcoming PRs

Maybe it would be good practice for us to use // TODO() for these temporary TODOs that don't quite warrant an issue, just other people know who to talk to if they run into it

suyash67 · 2025-09-12T11:37:21Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_tables.hpp

        element_table[29] = W5;
        element_table[30] = W6;
        element_table[31] = W7;
-    } else if constexpr (length == 7) {


Case length = 7 is never used, removing it.

suyash67 · 2025-09-12T11:38:24Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_tables.hpp

- **/
-template <typename C, class Fq, class Fr, class G>
-template <size_t length>
-element<C, Fq, Fr, G>::lookup_table_base<length>::lookup_table_base(const std::array<element, length>& inputs)


remvoing lookup_table_base impl.

suyash67 · 2025-09-12T11:41:58Z

...g/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/non_native_group_generator.cpp

-        ecc_generator_table<G1>::generator_endo_xhi_table[i] = std::make_pair<bb::fr, bb::fr>(endox2, endox3);
-        ecc_generator_table<G1>::generator_ylo_table[i] = std::make_pair<bb::fr, bb::fr>(y0, y1);
-        ecc_generator_table<G1>::generator_yhi_table[i] = std::make_pair<bb::fr, bb::fr>(y2, y3);
-        ecc_generator_table<G1>::generator_xyprime_table[i] =


Moved generator_xyprime_table and generator_endo_xyprime_table at the top, just to avoid recomputing x * beta (and makes things more cleaner)

suyash67 · 2025-09-12T11:42:09Z

...g/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/non_native_group_generator.cpp

    init = true;
 }

-// map 0 to 255 into 0 to 510 in steps of two


unused functions

ledwards2225

LG! couple of small questions/suggestions

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

ledwards2225 · 2025-09-15T03:08:37Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

+            size_t remaining_points = num_points - (num_fives * 5 + num_sixes * 6);
+
+            // Allocate one quad table if required (and update remaining points)
+            has_quad = (remaining_points >= 4) && (num_points >= 4);


is (num_points >= 4) needed here? seems like it would be caught by the 1st condition but maybe I'm misinterpreting

ledwards2225 · 2025-09-15T03:11:52Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

-     * Helper class to split a set of points into lookup table subsets
-     *
-     **/
-    struct batch_lookup_table_base {


ledwards2225 · 2025-09-15T03:13:29Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp

        Fq neg_lambda = Fq::msub_div({ x }, { (two_x + x) }, (y + y), { a });
        Fq x_3 = neg_lambda.sqradd({ -(two_x) });
        Fq y_3 = neg_lambda.madd(x_3 - x, { -y });
+        // TODO: do we handle the point at infinity case here?


Maybe it would be good practice for us to use // TODO() for these temporary TODOs that don't quite warrant an issue, just other people know who to talk to if they run into it

ledwards2225 · 2025-09-15T03:25:38Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/README.md

+> Note:
+> In the context of biggroup, we need variable-base lookup tables and fixed-base lookup tables. The variable-base lookup tables are used when the base point $P$ is not known at circuit synthesis time and is provided as a circuit witness. In this case, we need to generate the lookup tables on-the-fly based on the input base point $P$. On the other hand, fixed-base lookup tables are used when the base point $P$ is known at circuit synthesis time and can be hardcoded into the circuit (for example group generators). Fixed-base lookup tables are more efficient as they can be precomputed and do not require additional gates to enforce the correctness of the table entries. Variable-base lookup tables are realized using ROM tables (described below) while fixed-base lookup tables are realized using standard lookup tables in the circuit.
+
+### ROM Tables in Barretenberg


This is a nice description - what do you think about moving the non-biggroup specific stuff to a readme on ROM tables?

Right, moved this section to rom tables' README.

BEGIN_COMMIT_OVERRIDE fix: secp256k1 ecdsa mul - fix handling of point at infinity (#16679) fix: secp256k1 ecdsa mul handling of stagger point additions (#16685) chore: biggroup audit of lookup and rom tables (#16895) fix(bb): oversight that disabled batch commits (#17278) docs(bb): add initial cli reference (#17244) chore!: cycle group #6 (#17174) fix: mac bb publish (#17276) END_COMMIT_OVERRIDE

### 🧾 Audit Context Cleanup and audit of the lookup tables used in `biggroup` class. This PR does not change any logic, its mainly structural changes, cleanup and documentation. ### 🛠️ Changes Made - Added a README to explain how lookup tables are being used in biggroup, and a brief summary of how ROM tables work in barretenberg circuits. - Removed unused methods/structs like: `lookup_table_base`, `batch_lookup_table_base`, `create_endo_pair_five_lookup_table`. - Function comments + fix clang warnings + avoid hard-coded values. - No change to circuit, so no VK changes expected. ### ✅ Checklist - [x] Audited all methods of the relevant module/class - [x] Audited the interface of the module/class with other (relevant) components - [x] Documented existing functionality and any changes made (as per Doxygen requirements) - [x] Resolved and/or closed all issues/TODOs pertaining to the audited files - [x] Confirmed and documented any security or other issues found (if applicable) - [x] Verified that tests cover all critical paths (and added tests if necessary) - [ ] Updated audit tracking for the files audited (check the start of each file you audited) ### 📌 Notes for Reviewers NA

suyash67 added 19 commits September 9, 2025 12:24

comments in generator lookup tables.

7622668

cleanup and avoid hard-coded num limb bits.

9cfd789

remove unused fns.

5810139

minor pointers in twin rom table.

1a9afd2

remove redundant condition on zero index, put_constant_variable shoul…

cbfa9f4

…d handle it.

move create_endo_pair_four_bit_table_plookup to biggroup_tables.

0cba4f4

use NUM_LIMBS instead of hard-coded values.

19123ad

fix warnings in structs, add default destructors and move constructors.

0c0f446

fix warnings in function arg names

59a7ef9

cosmetic fixes.

f5ce7e2

dest for element.

c298ab3

remove unused create_endo_pair_five_lookup_table

d0c8e28

remove unused batch_lookup_table_base.

93e507b

fix warning in batch_lookup_table_plookup constr.

e50b1ff

add comments and simplify batch_lookup_table_plookup constr.

0dbf115

minor fix

ab63952

minor fix

f7226b0

draft readme for lookup tables.

3fcc637

add todo

f80affa

suyash67 marked this pull request as ready for review September 9, 2025 12:30

fix format.

8f2db9b

suyash67 added the bberg-int-audit All things related to barretenberg internal audit label Sep 9, 2025

suyash67 added 5 commits September 9, 2025 13:01

remove lookup_table_base as its not used anywhere.

14dba4d

avoid hard-coded values.

92f60b0

comments and fix warnings.

0e50d01

avoid hard-coded values.

faac600

remove (unused) len = 7 case.

6a3e1ef

suyash67 requested a review from iakovenkos September 10, 2025 23:23

suyash67 commented Sep 12, 2025

View reviewed changes

suyash67 requested a review from ledwards2225 September 12, 2025 11:53

ledwards2225 approved these changes Sep 15, 2025

View reviewed changes

address comments.

fd66488

suyash67 merged commit 57aecd9 into merge-train/barretenberg Sep 25, 2025
9 checks passed

suyash67 deleted the sb/audit-biggroup-1 branch September 25, 2025 13:20

AztecBot mentioned this pull request Sep 25, 2025

feat: merge-train/barretenberg #17293

Merged

Conversation

suyash67 commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧾 Audit Context

🛠️ Changes Made

✅ Checklist

📌 Notes for Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ledwards2225 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

suyash67 commented Sep 9, 2025 •

edited

Loading