chore: pippenger int audit by iakovenkos · Pull Request #19302 · AztecProtocol/aztec-packages

iakovenkos · 2026-01-05T11:41:01Z

clean up + docs+ a couple of edge case tests

…i/pippenger-audit-0

…ing output

iakovenkos · 2026-01-16T16:38:24Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-    uint32_t result = lo + (hi << lo_slice_bits);
-    return result;
+    size_t lo_bit = (hi_bit < slice_size) ? 0 : hi_bit - slice_size;
+    return scalar.get_bit_slice_raw(lo_bit, hi_bit);


Moved this method to the field core primitives. it's very efficient and can't be replaced with existing primitives. I tried creating uint256_t and slicing it, it resulted in 4-6% regression

iakovenkos · 2026-01-16T16:38:50Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-            cached_cost = total_cost;
-            target_bit_slice = bit_slice;
+    // Cost model: total_cost = num_rounds * (num_points + num_buckets * BUCKET_ACCUMULATION_COST)
+    auto compute_cost = [&](size_t bits) {


A bit of renaming here

iakovenkos · 2026-01-16T16:44:38Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

    const size_t num_rounds = numeric::ceil_div(NUM_BITS_IN_FIELD, bits_per_slice);
    for (size_t i = 0; i < num_rounds; ++i) {
-        round_output = evaluate_pippenger_round(msm_data, i, affine_data, bucket_data, round_output, bits_per_slice);
+        evaluate_pippenger_round(msm_data, i, affine_data, bucket_data, msm_result, bits_per_slice);


don't love void methods with a bunch of params, but i think it's clear that the method modifies msm_result

iakovenkos · 2026-01-16T16:46:06Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-    result += round_output;
-    return result;
+    // Accumulate into running total
+    accumulate_round_result(msm_accumulator, bucket_result, round_index, bits_per_slice);


avoiding duplication here

iakovenkos · 2026-01-16T16:49:38Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

+ * @param scratch_it Current scratch space iterator (modified in place)
+ */
+template <typename Curve>
+__attribute__((always_inline)) static inline void process_single_point(


simply isolating shared logic from consume_point_schedule

iakovenkos · 2026-01-16T16:50:00Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

+}
+
+/**
+ * @brief Process a pair of points/buckets using branchless conditional moves


iakovenkos · 2026-01-16T16:52:20Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-                                        MSM<Curve>::BucketAccumulators& bucket_data,
-                                        size_t num_input_points_processed,
-                                        size_t num_queued_affine_points) noexcept
+                                        MSM<Curve>::BucketAccumulators& bucket_data) noexcept


switched from recursive to iterative structure here

iakovenkos · 2026-01-20T18:10:48Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

 template <typename Curve>
 void MSM<Curve>::transform_scalar_and_get_nonzero_scalar_indices(std::span<typename Curve::ScalarField> scalars,
-                                                                 std::vector<uint32_t>& consolidated_indices) noexcept
+                                                                 std::vector<uint32_t>& nonzero_scalar_indices) noexcept


renaming + added docs

iakovenkos · 2026-01-20T18:11:25Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

    size_t thread_accumulated_work = 0;
    size_t current_thread_idx = 0;
    for (size_t i = 0; i < num_msms; ++i) {
-        BB_ASSERT_DEBUG(i < msm_scalar_indices.size());


redundant assert

iakovenkos · 2026-01-20T18:12:36Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

            const size_t available_thread_work = total_thread_work - thread_accumulated_work;
+            const size_t work_to_assign = std::min(available_thread_work, msm_work_remaining);
+
+            work_units[current_thread_idx].push_back(MSMWorkUnit{


A bit cleaner when using min

iakovenkos · 2026-01-20T18:14:48Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

- * @return constexpr size_t
- */
-template <typename Curve> size_t MSM<Curve>::get_optimal_log_num_buckets(const size_t num_points) noexcept
+template <typename Curve> uint32_t MSM<Curve>::get_optimal_log_num_buckets(const size_t num_points) noexcept


slightly improved the clarity here

iakovenkos · 2026-01-20T18:15:12Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

 template <typename Curve> bool MSM<Curve>::use_affine_trick(const size_t num_points, const size_t num_buckets) noexcept
 {
-    if (num_points < 128) {
+    if (num_points < AFFINE_TRICK_THRESHOLD) {


all constants are now living in the header

iakovenkos · 2026-01-20T18:17:34Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

- * @param scratch_space coordinate field scratch space needed for batched inversion
- **/
 template <typename Curve>
 void MSM<Curve>::add_affine_points(typename Curve::AffineElement* points,


Deduplicated in the follow-up

iakovenkos · 2026-01-20T18:19:17Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

- */
 template <typename Curve>
-typename Curve::Element MSM<Curve>::small_pippenger_low_memory_with_transformed_scalars(MSMData& msm_data) noexcept
+typename Curve::Element MSM<Curve>::jacobian_pippenger_with_transformed_scalars(MSMData& msm_data) noexcept


Seems clearer this way

iakovenkos · 2026-01-20T18:20:56Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

+
+    for (uint32_t round = 0; round < num_rounds; ++round) {
+        // Populate buckets using Jacobian accumulation
+        for (size_t i = 0; i < size; ++i) {


Basically unfolded evaluate_small_pippenger_round, the impl is pretty concise

iakovenkos · 2026-01-20T18:23:29Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-    for (size_t i = 0; i < num_rounds; ++i) {
-        round_output = evaluate_pippenger_round(msm_data, i, affine_data, bucket_data, round_output, bits_per_slice);
-    }
+    // Per-call allocation for WASM compatibility (thread_local causes issues in WASM)


Not sure it was thread_local specifically or its combination with accidentally disabled multi-threading in Pippenger. But ivc integration test were failing, seemed worth documenting

iakovenkos · 2026-01-20T18:25:22Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-            } else {
-                bucket_data.buckets[bucket_index] = points[nonzero_scalar_indices[i]];
-                bucket_data.bucket_exists.set(bucket_index, true);
+    for (uint32_t round = 0; round < num_rounds; ++round) {


again, unfolded pippenger_round, just to be able to see the entire algo in one loop

iakovenkos · 2026-01-20T18:28:51Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-                                        MSM<Curve>::BucketAccumulators& bucket_data,
-                                        size_t num_input_points_processed,
-                                        size_t num_queued_affine_points) noexcept
+void MSM<Curve>::batch_accumulate_points_into_buckets(std::span<const uint64_t> point_schedule,


somewhat more math-oriented name, makes it easier to match with the abstract description

unfolded recursion

both loops share bucket/point processing logic

iakovenkos · 2026-01-20T18:33:31Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

-    // unfortnately we need to remove const on this data type to prevent duplicating _scalars (which is typically
-    // large) We need to convert `_scalars` out of montgomery form for the MSM. We then convert the scalars back
-    // into Montgomery form at the end of the algorithm. NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/1449): handle const correctness.


Tried to make it cleaner but couldn't find something that would look way cleaner. Maybe can take a look in a followup PR. But added tests checking that batch msm preserves constness.

…i/pippenger-audit-0

barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp

ledwards2225

Overall LGTM - some great cleanup. Few issues with the get_bit_slice_raw method I think and some other minor comments

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/bitvector.hpp

barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp

federicobarbacovi · 2026-01-23T09:43:18Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.test.cpp

+        AffineElement result = scalar_multiplication::MSM<Curve>::msm(points, scalar_span);
+
+        AffineElement expected(base_point * scalar_sum);
+        EXPECT_EQ(result, expected);


Is the fact that it works expected even though handle_edge_cases is false?

wasn't using enough points to trigger pippenger, actually, thanks for catching this

federicobarbacovi · 2026-01-23T12:32:46Z

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/README.md

+
+### Point Scheduling (Affine Variant Only)
+
+Entries are packed as `(point_index << 32) | bucket_index` and sorted via **in-place MSD radix sort**. Sorting groups points by bucket, enabling efficient batch processing. The sort also detects entries with `bucket_index == 0` during the final radix pass, allowing zero-bucket entries to be skipped without a separate scan.


Maybe it's worth stressing that as c = 8, this is effectively appending the bucket index at the end of the index we use to pack the points

federicobarbacovi

Great work! Just some minor comments

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp

…i/pippenger-audit-0

BEGIN_COMMIT_OVERRIDE feat: support JSON input files for bb verify command (#19800) fix: update bootstrap.sh to use new JSON field names chore: Update `index.js` so that `HAS_ZK` and `PUBLIC_INPUTS` variables must always be set in tests (#19884) chore: pippenger int audit (#19302) chore: deduplicate batch affine addition trick (#19788) chore: transcript+codec+poseidon2 fixes (#19419) chore!: explicitly constrain inputs and intermediate witnesses (#19826) fix: exclude nlohmann/json from WASM builds in json_output.hpp chore: translator circuit builder and flavor audit (#19798) Revert "fix: exclude nlohmann/json from WASM builds in json_output.hpp" Revert "feat: support JSON input files for bb verify command (#19800)" Revert "fix: update bootstrap.sh to use new JSON field names" END_COMMIT_OVERRIDE

clean up + docs+ a couple of edge case tests Closes AztecProtocol/barretenberg#486 --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>

iakovenkos added 23 commits December 23, 2025 12:34

initial clean up

d163891

recursive -> iterative

75ef963

add first approximation of docs + rm redundant alias

2f9c59d

tackle issue 1449

45e1a64

tackle issue 1449

79e17e2

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

6e24ce6

…i/pippenger-audit-0

small refactor

3bd30ca

reapply centralized montgomery conversion

7ef589a

clean up

9a771a6

get_offset_generator out of the loop

6072169

revert some branching

47cae52

fixing magic constants + reusing existing stuff

e6f9c8f

more const updates

794a038

introduce point schedule entry

37c3d8b

consolidated --> nonzero_scalar_indices

a43c966

clean up get_work_units

cc17b30

batch msm clean up

22f583b

evaluate_pippenger_round mutates in-place instead of returning confus…

8916b60

…ing output

use uint32_t where possible

ac4f0ab

unfold recursion

5e8cae1

use common helper to process buckets

c487e40

share logic to produce single point edge case

c8142f0

rm redundant args

8f0dbfc

iakovenkos commented Jan 16, 2026

View reviewed changes

iakovenkos commented Jan 20, 2026

View reviewed changes

iakovenkos requested a review from ledwards2225 January 20, 2026 18:36

fix build

15b9521

iakovenkos removed the ci-full Run all master checks. label Jan 21, 2026

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

113a58a

…i/pippenger-audit-0

federicobarbacovi reviewed Jan 22, 2026

View reviewed changes

barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp Outdated Show resolved Hide resolved

ledwards2225 approved these changes Jan 22, 2026

View reviewed changes

federicobarbacovi reviewed Jan 23, 2026

View reviewed changes

federicobarbacovi approved these changes Jan 23, 2026

View reviewed changes

barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.cpp Outdated Show resolved Hide resolved

iakovenkos added 3 commits January 23, 2026 14:43

move scalar slicing back to pippenger

e5d0055

address more comments

65c92dc

Merge remote-tracking branch 'origin/merge-train/barretenberg' into s…

6c3dcfa

…i/pippenger-audit-0

iakovenkos merged commit 644847c into merge-train/barretenberg Jan 23, 2026
8 checks passed

iakovenkos deleted the si/pippenger-audit-0 branch January 23, 2026 15:37

AztecBot mentioned this pull request Jan 23, 2026

feat: merge-train/barretenberg #19887

Merged

suyash67 mentioned this pull request Jan 26, 2026

chore: translator circuit builder and flavor audit #19798

Merged

7 tasks

danielntmd pushed a commit that referenced this pull request Jan 27, 2026

chore: pippenger int audit (#19302)

977e6ee

clean up + docs+ a couple of edge case tests Closes AztecProtocol/barretenberg#486 --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>


		### Point Scheduling (Affine Variant Only)

		Entries are packed as `(point_index << 32) \| bucket_index` and sorted via in-place MSD radix sort. Sorting groups points by bucket, enabling efficient batch processing. The sort also detects entries with `bucket_index == 0` during the final radix pass, allowing zero-bucket entries to be skipped without a separate scan.

Conversation

iakovenkos commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iakovenkos Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

iakovenkos Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

ledwards2225 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

federicobarbacovi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iakovenkos commented Jan 5, 2026 •

edited

Loading

iakovenkos Jan 16, 2026 •

edited

Loading

iakovenkos Jan 20, 2026 •

edited

Loading