feat: Pippenger refactor full 11 may - #23297
Merged
Merged
Conversation
ledwards2225
changed the title
iakovenkos
marked this pull request as ready for review
May 19, 2026 12:01
iakovenkos
requested review from
IlyasRidhuan,
MirandaWood and
jeanmon
as code owners
May 19, 2026 12:01
iakovenkos
force-pushed
the
lde/zacs-pippenger
branch
from
May 21, 2026 09:08
758407a to
b1fe65a
Compare
ci full green, some deduplication
Restore the legacy mutex and atomic thread pools (removed by the Pippenger rewrite) and make the parallel_for backend runtime-selectable via the BB_PARALLEL_POOL env var, read once at first dispatch: unset / other -> generation (the rewrite's generation-counter pool; default) mutex -> legacy std::mutex/condition_variable pool atomic -> legacy lock-free atomic pool Default is unchanged (generation), so branch behavior is identical unless the env var is set. The toggle exists so one binary can A/B the pools per-device without a rebuild — needed because benchmarks show the pool choice is platform- dependent: a wash on native (all three within +-3%, generation slightly jitterier) but generation is decisively faster under wasmtime (+7-35% vs mutex, +6-49% vs atomic). The browser/V8 device matrix is the deciding target and is captured as a follow-up task brief. Also add PippengerSparsity microbenches: single-MSM pippenger_round_parallel across dyadic sizes 2^15..2^19 under two scalar distributions — Dense80 (80% random nonzero, 20% zero; dedup off) DupHeavy (50% unique / 25% dup A / 5% dup B / 20% zero; dedup on) The DupHeavy profile drives the heavily-threaded Phase A dedup pre-pass, the stage most sensitive to pool dispatch. Scalars are built from a per-(profile, size) deterministically-seeded RNG so every repetition and every pool build sees byte-identical inputs — the pool A/B is properly paired and run-to-run variance reflects scheduler noise only, not input variation. BROWSERSTACK_POOL_TASK.md briefs an agent with BrowserStack access to run the microbench A/B across the real-V8 device matrix and decide the WASM default.
Add a runtime override for the Pippenger window-sizing oversubscription factor: BB_MSM_OVERSUB=<n> forces window_bits_tuning_oversub_factor() to a constant (read once; 0/unset = built-in 1/2/4 default). The default factor was co-tuned with the now-rejected generation thread pool, so it needs re-validating against the shipping mutex pool. Native benching is inconclusive by construction: the EC2 box has 192 physical cores, so at HC=8 even 4x oversubscription (32 logical threads) never actually oversubscribes relative to hardware — oversub 1/2/4 all land within +-2%. The failure mode (logical threads > physical cores -> scheduler thrash) only appears on low-core devices under real V8. BROWSERSTACK_OVERSUB_TASK.md briefs an agent with BrowserStack access to sweep the factor across the device matrix under the mutex pool and decide whether the policy should be core-count-aware.
This reverts commit 6d493cd.
Back out the Pippenger rewrite's threading changes entirely. Real V8 device benches (BrowserStack) plus native microbenches showed the new generation-counter pool loses on WASM/V8 and merely ties on native (while being jitterier), so it is not justified on any platform. Rather than keep it as an opt-in, remove it. This restores barretenberg/cpp/src/barretenberg/common/ to the merge-train state: the generation pool and the BB_PARALLEL_POOL toggle are gone; thread.cpp dispatches parallel_for to parallel_for_mutex_pool (the merge-train default); the omp/queued/ spawning/atomic/mutex backends are all present again. Pippenger calls only parallel_for, so it now uses the mutex pool automatically with no MSM-side change. Net effect: the Pippenger work no longer touches threading -- zero common/ diff vs merge-train, and the WASM-crash workaround the generation pool required is no longer needed.
The env-gated sanity-verify skip was a benchmarking convenience added during the Pippenger rewrite; it is unrelated to the MSM work and doesn't belong in this PR. Restore bbapi_chonk.cpp to the merge-train behavior: ChonkProve always verifies the generated proof as a sanity check. Benchmarking that needs to isolate proving time can drop the verify locally without shipping the flag.
Removing the variable-window split left two inert vestiges: - choose_window_bits' `use_rebalance` parameter was static_cast<void>'d in both the wasm and native branches (never read) and every caller passed `true`. It was the knob selecting the split's rebalance variant. Drop the parameter and the dead casts/args. - build_var_window_schedule's `fill_windows` lambda carried an `out_offset` parameter and a `(w == 0) ? 0 : ...` branch that only fired when filling a second (hi) region of a split schedule. With split gone it is always called with out_offset 0, so the branch is unreachable. Collapse to a single-region fill loop. Also rename the stale `window_bits_unsplit` parameter to `window_bits` (there is no split counterpart anymore). Behavior-preserving: the schedule is uniform-width (last window truncated) as before. All ecc MSM + arena-fit tests pass.
The task brief asked an external agent to A/B the generation-counter thread pool against the legacy pools on the V8 device matrix. That investigation is complete: real V8 data showed the generation pool loses on browsers, and the pool was removed entirely (common/ restored to merge-train state, mutex is the default). The brief documents a comparison whose subject no longer exists in the tree.
Move the input-scalar dedup pre-pass (~550 lines) out of scalar_multiplication.cpp
into a dedicated header. Pure code motion — every function is inline / templated,
the compiler sees byte-identical bodies at byte-identical call sites, codegen is
unchanged. Bench confirms: all 10 PippengerSparsity cases within +-1.2% of
pre-extraction medians (noise band on the EC2 box; the DupHeavy profile, which
exercises the dedup path, shows a uniform +0.8..+1.1% drift consistent with
between-session noise).
Extracted:
- The 32-bit schedule encoding constants (SCHEDULE_SIGN_BIT, DEDUP_REDIRECT_BIT,
DEDUP_SKIP_BIT, SCHEDULE_INDEX_MASK, DEDUP_INVALID_EXTRA) and their static_asserts;
these are co-defined as one encoding scheme, the two dedup bits exist because of
dedup, and the index mask is derived from where they were carved out.
- The scalar-fingerprint hash helpers (dedup_scalar_fingerprint, dedup_fingerprint_slot).
- DedupResult and PhaseAScratch structs.
- dedup_tree_reduce_in_place (the chunked batched-affine cluster reduction).
- dedup_phase_a_worker_hash (the main Phase A worker: hash table over long-scalar
entries, cluster scan, redirect-lookup writes).
- dedup_patch_schedule_window (Stage 4b's redirect-aware schedule rewrite).
What stays in scalar_multiplication.cpp:
- Stage 4 / Stage 6a schedule consumers that check the DEDUP_* bits (the protocol
layer is unchanged; they see the constants via the new include).
- The six `if (dedup_active)` dispatch sites (arena sizer + main loop) that decide
whether to budget and run Phase A.
- The shared copy_affine64, ThreadScratch, and non-dedup tree_reduce_in_place (Phase A
has its own self-contained tree-reduce, so no shared dep).
86 ecc MSM tests pass.
…oduction K
The previous scenario set had a literal TODO admitting the K values were not
representative — Translator K=4 when production K is 10, MegaOink K=11 when
commit_to_wires actually batches 17 polys, no ECCVM coverage. Replace with the
actual K + N + sparsity values derived from the prover call sites:
Translator_K10_2_17 — execute_wire_and_sorted_constraints_commitments_round:
5 ConcatenatedPolynomials + 5 OrderedRangeConstraints,
all at MINI_CIRCUIT * CONCAT = 2^13 * 16 = 2^17.
MegaOink_K17_2_17 — OinkProver::commit_to_wires (Mega): 3 base wires +
4 ecc_op_wires + 10 databus polys.
DatabusOnly_K10_2_14_mostly0 — databus sub-pattern in isolation at a smaller size.
ECCVM_dense_K60_2_15 — ECCVMProver::execute_wire_commitments_round, the
ECCVM_transcript_K25_2_15 ~60 dense wires + ~25 transcript wires split out.
Transcript wires are populated only to op-queue size
(2^12) in a 2^15 dyadic allocation, so ~87.5% zero.
BN254 used as a proxy for the Grumpkin batch: at
N=2^15 both curves sit above their native GLV threshold
(2^13), so the dispatcher's GLV-prefix-sharing
optimisation is OFF either way and the batched/per-MSM
ratio transfers.
Native A/B (EC2, HC=8, 5 reps) shows the multi-MSM batched driver does not beat
the per-MSM fallback at any production K + N: all five scenarios within +-1.6%,
most within the stddev band. Mechanism: at every production N (>= 2^14) the
per-MSM GLV decision falls above the native threshold (2^13), so the dispatcher's
only cross-MSM amortisation (the shared GLV-doubled prefix) never fires.
Removed: the K >= T threadsplit-fastpath scenarios that were exploratory and not
tied to any prover; the K=14 Translator variant that didn't match production.
iakovenkos
force-pushed
the
lde/zacs-pippenger
branch
from
May 29, 2026 10:09
9f33cba to
5cf8f5e
Compare
Stage 1 / Stage 4 extract scalar window digits via a 4-wide SIMD path (store_constantine_packed_digits_x4_*) for full 64-scalar chunks, falling back to the scalar get_constantine_packed_digit for the remainder. The SIMD path's header claims a net win but it was never measured in isolation, and its fast-path-rate tradeoff (localised 77%->50% at c=14) could land differently on V8/WASM than native. Add msm_force_scalar_digits(): runtime BB_MSM_SCALAR_DIGITS env (native/wasmtime) or compile-time BB_MSM_FORCE_SCALAR_DIGITS (browser, since env vars don't reach browser wasm). Read once; the guard sits outside the per-scalar inner loop so the shipped SIMD path pays nothing. Both stages route everything through the proven scalar tail when forced — all 86 ecc MSM tests pass in both modes (identical results). Native A/B (EC2, HC=8, 5 reps, PippengerSparsity): SIMD x4 is a small consistent win, ~1-2% across sizes/profiles (digit extraction is a small slice of total MSM time; Stage 6a dominates). Real but modest. The open question is V8/ARM, where WASM SIMD lowering differs from native -- captured in BROWSERSTACK_SIMD_DIGITS_TASK.md.
Reinstates the pre-rewrite Pippenger MSM behind the public MSM facade so it can
be toggled on (BB_MSM_LEGACY=1) if a critical bug surfaces in the rewrite during
the soak period, and removed cleanly in ~a month.
Structure (chosen so the PR diff vs merge-train reads cleanly):
* scalar_multiplication.{hpp,cpp} keep the canonical path and host the LEGACY
implementation with bodies byte-identical to merge-train — only wrapped in the
`bb::scalar_multiplication::legacy` sub-namespace (namespace line + the two
explicit instantiations are the entire body delta) — plus the public facade.
* The round-parallel rewrite moves verbatim to scalar_multiplication_fast.{hpp,cpp}
with its public entry points renamed `pippenger_fast` / `pippenger_unsafe_fast`
/ `MSM_fast` (internal helpers — pippenger_round_parallel, arena, constantine,
dedup — keep their names; they don't collide with legacy).
* process_buckets.{hpp,cpp} and bitvector.hpp are restored verbatim (the rewrite
had deleted them; legacy depends on them).
Facade: pippenger / pippenger_unsafe / MSM::msm / MSM::batch_multi_scalar_mul keep
the rewrite's signatures (callers untouched) and dispatch on use_legacy_msm()
(env BB_MSM_LEGACY, read once): default -> the `_fast` rewrite; legacy -> the
`legacy::` implementation, adapting arguments (legacy has no dedup pre-pass; its
batch entry takes per-MSM point spans).
Default behavior is unchanged (fast path). Removal later = delete the `legacy::`
blocks, process_buckets/bitvector, and the dispatch guards.
Verified: ecc_tests (86) and scalar_multiplication safe-mode (18) pass in BOTH
modes; bb and pippenger_bench link.
…de/zacs-pippenger
The toggle (msm_force_scalar_digits / BB_MSM_SCALAR_DIGITS / BB_MSM_FORCE_SCALAR_DIGITS) was bench-only infra for A/B-testing the 4-wide SIMD digit path against the scalar path. The SIMD path is the shipped default; drop the toggle and its two Stage 1 / Stage 4 guard checks, restoring the unconditional SIMD loop. Also drops the now-unused <cstdlib> include.
This was referenced Jun 4, 2026
iakovenkos
added a commit
that referenced
this pull request
Jun 5, 2026
…th Constantine recoder (#23691) Isolates the scalar-multiplication-**independent** ECC changes from the Pippenger refactor (#23297) into their own reviewable layer, stacked on top of the Constantine recoder PR (#23562). ## Stack `merge-train/barretenberg` ← #23562 (Constantine recoder) ← **this PR** ← #23297 (Pippenger) This PR depends on #23562 because it consumes the shared signed-Booth primitives in `ecc/groups/booth_recode.hpp`. It must merge after #23562. ## What's here - **element**: add `straus_msm`; rewrite `batch_mul_with_endomorphism` / `operator*` onto carry-less signed-Booth window slices, replacing the old `EndomorphismWnaf` lookup-table path. - **field**: short-circuit `split_into_endomorphism_scalars` for small `k`. - **wnaf**: drop the runtime `fixed_wnaf` overload — orphaned once `EndomorphismWnaf` (its only caller) was removed. - **dedup vs booth_recode.hpp**: element_impl no longer carries its own copy of `BoothSliceParams` / `compute_booth_slice_params`; the generic branchless `booth_packed_digit` reader now lives in `booth_recode.hpp` as a shared primitive. element_impl keeps only its element-specific GLV-endo window schedule + EC math. The Pippenger MSM path keeps its own perf-tuned reader. - **tests**: `straus_msm` correctness/edge cases and `batch_mul` K2 bit-width coverage. ## Testing `ecc_tests` builds and `StrausMsm*` + `BatchMul*` pass on the endomorphism curves (bn254, grumpkin); secp curves skip the endo tests by design.
…de/zacs-pippenger # Conflicts: # barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp # barretenberg/cpp/src/barretenberg/ecc/groups/element.test.cpp # barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp
iakovenkos
self-requested a review
June 5, 2026 12:22
iakovenkos
approved these changes
Jun 5, 2026
iakovenkos
added a commit
that referenced
this pull request
Jun 5, 2026
…#23887) Stacks on #23297 (base = `lde/zacs-pippenger`). Adds edge-case unit tests, fuzzers, and one out-of-bounds fix for the new round-parallel Pippenger / MSM, focused on the areas the refactor is most fragile in: **arena sizing** and **parameter-dependent dispatch**. Test-only except for a one-line OOB fix in an existing test. Validated natively **and under AddressSanitizer + `_GLIBCXX_DEBUG`**. ## Bug found & fixed **Out-of-bounds scalar read in `test_batch_multi_scalar_mul_large_dense`** (existing in #23297). It indexed `scalars[k*8192 + i]` up to 32767 against the 31013-element fixture. Release builds did silent UB (both result and reference read the same out-of-bounds garbage, so the assertion still passed — the bug was invisible); the debug/ASAN allocator aborts. Wrapped with `% num_points`, matching the sibling ragged test. ## Tests added **Arena (the documented "drift" bug class — sizer vs live allocator):** - `ArenaLayoutFitsAcrossDispatchSpace` — sweeps ~14k combos of thread count (incl. odd/non-pow2), N at every dispatch boundary, **`dedup=true` and `ext_glv=true`** (neither was ever checked against the sizer), and bit budgets, plus 1600 deterministic pseudo-random N. Any miss is a guaranteed Zone overflow. - `EffectiveNumBitsBandSmallScalars` — drives real MSMs in the native non-GLV mid-band (2^13<n<2^17) where the sizer's defensive bit-budget sweep is skipped, so an under-count surfaces as an OOB write rather than passing silently. **Dispatch / GLV / dedup:** - `DispatchFuzz` — randomized differential fuzzer over (N, start_index, thread count, scalar distribution, dedup) vs naive. - `GlvExtremeMagnitudeScalars` — magnitude-maximizing scalars (r−1, (r−1)/2, λ±1, the k2-negative-fix region, bit-{126,127,128,129,253} boundaries) through the real `use_glv=true` path; checks the 2-limb (≤128-bit) split halves recombine to k·P, catching any silent >128-bit truncation or top-window carry drop. - `DedupLargeClusterCarryAndCaps` — equal-scalar clusters of distinct points past `DEDUP_MAX_CHUNK_MEMBERS` and the staged cap, exercising the multi-chunk tree-reduce carry and the partial-consolidation fallback (vs naive and vs dedup-off). - `ExternalGlvDoubledDirect` — the `external_glv_doubled` aliasing branch directly (was only reached transitively via the batch driver), incl. N above the native GLV threshold. **Batch driver (the production commit-batch path):** - `BatchDriverSharedPathRagged` — `batch_multi_scalar_mul(handle_edge_cases=false)` is the **only** entry into the shared `pippenger_round_parallel_batched` path (GLV-group assignment, `external_glv_doubled` aliasing, single shared arena sized to the largest member). Every existing batch test uses the default `handle_edge_cases=true` and routes around it. Ragged batch with empty MSMs interspersed (the `n==0 → infinity` skips), fully-zero MSMs, a mixed GLV / non-GLV group split, total nonzero past the >4096 REBALANCE threshold, and mixed per-MSM dedup hints. **Large-N multi-threaded edge cases (Jacobian path):** - `HandleEdgeCases{PointAtInfinity,InversePairs,AllInfinity}` — infinity / P+(−P) bucket collisions at large N with forced 8 threads, hitting the multi-threaded Jacobian split + cross-thread reduction. `scalar_multiplication_safe_mode.test.cpp` covers these only at ≤60 pts (single-threaded). ## Verification Built `ecc_tests` on native (x86_64) — all new tests pass for BN254 and Grumpkin; full `*ScalarMultiplication*`+`*VariableWindow*` suite green. Then rebuilt under the `asan-fast` preset (ASAN + `_GLIBCXX_DEBUG`) and ran the entire suite (123 tests): clean after the OOB fix — the **real** Zone P/W/S allocator validated across the whole swept dispatch space, not just the test-side layout mirror. clang-format-20 clean. Large/GLV-threshold-dependent cases are `__wasm__`-skipped per the file's convention. --------- Co-authored-by: iakovenkos <sergey.s.yakovenko@gmail.com>
wei3erHase
referenced
this pull request
in defi-wonderland/aztec-packages
Jun 8, 2026
BEGIN_COMMIT_OVERRIDE refactor: Booth-slice element MSM (straus_msm + batch_mul), shared with Constantine recoder (#23691) feat: Pippenger refactor full 11 may (#23297) chore: pippenger edge-case unit tests, fuzzers, bench markers removal (#23887) END_COMMIT_OVERRIDE
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.
Ports AztecProtocol/barretenberg-claude#3643.