refactor(bb): give cycle_group its own offset generator store

[AztecBot]

The coupling

cycle_group took its MSM offset generators from crypto::generator_data — the pedersen generator store — via a GeneratorContext parameter threaded down batch_mul / fixed_batch_mul.

That store is shared, mutable, and grows in place. Its own header documents it as not thread-safe:

This is not thread-safe. Each process that uses a generator_data object may extend generator_data if more generators are required ... either each process must use an independent generator_data object or the author must KNOW that generator_data will not be extended by any process.

It also returns a std::span into a std::vector that a later call for the same domain reallocates, so a view handed to one caller can be freed underneath it.

Both halves of that contract are violated in practice. cycle_group::batch_mul holds one of those spans across an entire MSM build, and bb aztec_process derives verification keys for several circuits on concurrent std::threads against the shared singleton. The result is a rare, unreproducible cycle_group: Point is not on curve abort during noir-contracts bootstrap — an example run, the failing contract.

The GeneratorContext parameter carrying this dependency was never once supplied. Every call site in stdlib/ and dsl/ defaulted it, and ipa.hpp passed a literal {} purely to reach the table_bits argument sitting behind it.

What this does

Adds stdlib/primitives/group/cycle_group_offset_generators.hpp, a store that owns exactly this one concern:

template <typename Curve> class cycle_group_offset_generators {
  public:
    static constexpr std::string_view DOMAIN_SEPARATOR = "cycle_group_offset_generator";

    static const cycle_group_offset_generators& default_generators();
    std::span<const AffineElement> get(size_t num_generators) const;
};

The design aims at removing the brittleness rather than guarding it:

• Prefix-only. No offset or slice parameter. Generator i depends on nothing but the domain separator and i, so no request can change what another request sees. That is all cycle_group ever needed from the general store.
• Append-only blocks. Growth allocates a new block and retains the superseded one, so a span already handed out stays valid however many times other threads call get afterwards. Growth doubles, so the retained set stays at a handful of entries.
• An instance, not a hidden global. Circuit construction shares default_generators(), but the store is an ordinary object. This is what lets the tests below be non-vacuous — see the note on that.
• Locked. A plain std::mutex; the critical section is a size check plus a rare derivation.

GeneratorContext disappears from cycle_group's API, fixed_batch_mul(points, scalars, {}, 8) in ipa.hpp becomes fixed_batch_mul(points, scalars, 8), and the now-unused crypto/pedersen_commitment/pedersen.hpp include comes out of both cycle_group files.

crypto::generator_data and the pedersen path are untouched.

Why memoize

Measured here: derive_generators costs roughly 65us per generator — 550us for 8, 4.3ms for 65, 84ms for 1024. Deriving per batch_mul would be a real regression, so the store memoizes, and doubling blocks keep a circuit made of many differently sized MSMs to a handful of derivations rather than one per MSM.

Verification keys are unaffected

Offset generator i is the same point as before: same domain separator, same derivation, same prefix semantics — the previous path also requested offset 0. MatchesDerivation and PrefixIsStableAcrossRequestSizes pin exactly that, and the circuit tests below carry fixed expected values.

Tests

stdlib/primitives/group/cycle_group_offset_generators.test.cpp:

Test	What it pins
MatchesDerivation	Values equal derive_generators for the domain
PrefixIsStableAcrossRequestSizes	Generator i does not depend on the request size
GetKeepsPreviouslyReturnedGeneratorsValid	A span stays valid after a larger request grows the store
ConcurrentGetReturnsOnCurvePoints	Concurrent get never yields an off-curve point

These were checked against a deliberately reintroduced bug — replacing the block on growth instead of retaining it. PrefixIsStableAcrossRequestSizes and GetKeepsPreviouslyReturnedGeneratorsValid go red deterministically, and green again once the retain is restored.

Two details are worth knowing before editing these tests, because both produced a silently passing suite first time round:

• The store has to be an instance. With function-local statics the cache is process-global, the growth path is live only for the first few assertions, and everything afterwards passes trivially — the first draft passed even with the bug present.
• The deterministic test sprays the freed size range before re-reading. Without that, a stale span reads its own old contents back and passes while pointing at released memory.

ConcurrentGetReturnsOnCurvePoints is the weakest of the four and is not the primary regression net: with the map race gone the residual failure is a dangling read, and freed-but-unreused memory usually still reads back correct. It is kept as the symptom-level reproduction and as an ASan/TSan target, with the two deterministic tests doing the real work.

Run

Full cmake --build build clean, including bb, bb-avm and bb-avm-sim.

stdlib_primitives_tests            1349 passed, 0 failed   (128 cycle_group)
dsl_tests                           551 passed, 0 failed
commitment_schemes_tests (IPA)       30 passed, 0 failed
crypto_pedersen_commitment_tests      2 passed, 0 failed
crypto_pedersen_hash_tests            6 passed, 0 failed

Note on scope

This removes cycle_group's dependence on the shared store; it does not fix the store itself. generator_data::get still hands out spans it can later invalidate, which remains a hazard for the pedersen path and for any future caller, and is worth addressing separately.

---

Created by claudebox · group: slackbot · requested by Sergei · Slack thread

[AztecBot]

⏳ Claude Review — starting review session...

[AztecBot]

✅ Adversarial verdict: PASS

✅ Claude Review — completed
live status
22m

VERDICT: PASS

The new head 9d59a8e is a merge of next into the PR branch; the PR's own delta is untouched — git diff 115d643..9d59a8e over stdlib/ and commitment_schemes/ is empty, and every merged-in file comes from already-merged next work (#24799, #25073, #25139, etc.: ipc, spartan, acir components check). No conflict resolutions touched cycle_group, ipa.hpp, or the new offset-generator store, so the prior review's correctness analysis (index-addressable derive_generators ⇒ identical points and VKs; append-only retained blocks ⇒ span validity; mutex-guarded growth) carries over verbatim.

Fresh verification at the exact head (clean reconfigure after workspace reset, CRS re-downloaded):

• Build of stdlib_primitives_tests + dsl_tests at 9d59a8e: clean.
• CycleGroupOffsetGenerators.* + CycleGroupTest/*: 120/120 pass.
• Full stdlib_primitives_tests: 1349 passed, 0 failed.
• Full dsl_tests (exercises VK generation through the new store, incl. the merged acir_format changes): 551 passed, 0 failed.

No new findings; the two cosmetic nits from the first review remain non-blocking.

[AztecBot]

Claude Review: Starting automated code review... workflow run

[AztecBot]

Claude Review: Starting automated code review... workflow run