Skip to content
This repository was archived by the owner on Aug 7, 2026. It is now read-only.

ci(bench): WASM + native perf-regression tracking with sticky PR comments - #1002

Open
WiktorStarczewski wants to merge 11 commits into
nextfrom
wiktor/bench-tracking
Open

ci(bench): WASM + native perf-regression tracking with sticky PR comments#1002
WiktorStarczewski wants to merge 11 commits into
nextfrom
wiktor/bench-tracking

Conversation

@WiktorStarczewski

@WiktorStarczewski WiktorStarczewski commented May 10, 2026

Copy link
Copy Markdown
Collaborator

"The perf looks really nice, and we should definitely start tracking WASM performance in the E2E benchmarking infrastructure to make sure it stays that way!"

@adr1anh on Slack, reviewing #998

This PR is a direct response to that. Adrian's right — measuring a one-time win is less valuable than catching the regression that erodes it back. So this PR sets up automated perf-regression tracking for 0xMiden/crypto, both in WASM (the runtime that ships to wallet/dApp users via V8) and natively (Linux x86_64 GHA runners), with sticky PR comments showing the diff vs the latest next baseline on every single PR going forward.


What this PR adds

A new CI workflow (.github/workflows/bench.yml) with two jobs that run on every PR + every push to next, both feeding benchmark-action/github-action-benchmark:

  1. bench-wasm — headless-Chromium harness in miden-bench-wasm/. Builds the bench WASM via wasm-pack, drives Chromium via Playwright, runs the benches, posts the medians.
  2. bench-nativecargo bench --bench {hash,word,transpose}. Same primitives at native speed for comparison + native-side regression detection.

The crypto repo's first piece of automated perf tracking. No bench had previously been wired into CI — the existing benches/README.md is hand-curated and doesn't catch regressions between runs.


Why a sticky PR comment, specifically

When a PR opens, every reviewer asks the same question: "is this PR going to make things slower?" Today that question gets answered the way it always has: someone runs benches by hand, eyeballs the numbers, hopes nothing slipped. That doesn't scale and it's not reliable.

A sticky perf comment puts the answer to that question right in the PR review thread, updated automatically on every push:

| Bench                           | Before  | After   | Δ      |
| ------------------------------- | ------- | ------- | ------ |
| rpo256_merge                    | 4.83 µs | 4.91 µs | +1.7%  |
| rpo256_packed_permute           | 4.85 µs | 2.42 µs | -50.0% ⚡
| lifted_stark_prove_blake3       | 4.63 s  | 3.21 s  | -30.7% ⚡

Six properties make this a load-bearing pattern, not just a nice-to-have:

  1. Single source of truth per PR. The comment updates in place on every push, so the review thread doesn't fill with stale snapshots.
  2. Diff vs baseline at a glance. Reviewers don't need to know what numbers used to look like — the action computes the delta against the latest next median.
  3. Native to the GitHub review flow. No external dashboard to remember, no auth, no "go check Grafana" tax. The signal is where the conversation already is.
  4. Symmetric on wins and losses. Same comment that flags a regression also celebrates the optimisation — it's how PR feat(field,crypto,stark): wasm32+simd128 PackedFelt + trait-generic packed Permutation + DEEP/FRI hot-path fix #998 will show its win once both PRs land.
  5. Zero maintainer overhead per PR. No manual perf review step. The bot does it.
  6. Cheap to ignore false positives. GHA Linux runners share CPUs — single-digit-percent variance is real. We threshold alerts at 10% on both wasm and native, don't fail the workflow, and let humans decide whether the diff is real or noise. False-positive comments are cheap; false-negative regressions are expensive. (See "On the alert threshold itself" below for why 10% and not 15%-20%.)

Industry pattern

This pattern (run benches in CI → store medians on a side branch → post a sticky comment with diff vs baseline) is the standard perf-regression-detection setup in 2026 for projects that take perf seriously but don't want a full perf-engineering team. A non-exhaustive list of users of this exact action or its close peers:

  • Yarn — uses github-action-benchmark for cross-version perf tracking on package-install workloads.
  • realm/SwiftLint — sticky comments on Swift codebase analysis perf.
  • hadolint/hadolint — Dockerfile linter, regression alerts on parser benchmarks.
  • GoogleChrome/web-vitals — same pattern, sticky comment with median + IQR per metric.
  • Pydantic — uses CodSpeed (a SaaS variant of the same pattern) for similar coverage.
  • Turborepo — CodSpeed-based, sticky comments on every PR.
  • DOMPurify — uses Bencher.dev, another variant of the pattern.

The common insight across all of these: catching a regression at the PR that introduces it is dramatically cheaper than catching it three months later in a release-readiness audit. Every tool that implements this pattern has the same UX shape: bench output → side-branch storage → sticky PR comment → optional alert threshold. We're picking the most-used Rust-friendly variant of it.


Three layers of tracking

The bench set was chosen carefully — different layers catch different classes of regression. None of them on its own is sufficient; together they cover the relevant failure modes.

Layer 1 — Public hash-primitive throughput

bench_rpo256_merge / bench_rpo256_sequential_felt_100
bench_rpx256_merge / bench_rpx256_sequential_felt_100
bench_poseidon2_merge / bench_poseidon2_sequential_felt_100
bench_blake3_256_merge / bench_blake3_256_sequential_felt_100
bench_keccak256_merge / bench_keccak256_sequential_felt_100

Tests the public Hash API (merge, hash_elements) end-to-end. Catches general regressions in the hash primitives — anyone who slows down the Rescue round function, the Poseidon2 MDS layer, or the Blake3 chunk processor will show up here.

These benches go through the scalar (WIDTH=1) fast path of the trait-generic Permutation impl. By construction this is byte-identical between next and PR #998 — the const-folded WIDTH=1 path in #998 is explicitly preserved as a zero-cost no-op. So these metrics will NOT show the simd128 win. They're orthogonal to it. They catch a different class of regression.

Layer 2 — Packed-permutation throughput

bench_rpo256_packed_permute
bench_rpx256_packed_permute
bench_poseidon2_packed_permute

These call permute_mut(&mut [<Felt as Field>::Packing; 12]). <Felt as Field>::Packing resolves to:

The dashboard step-down on these three metrics, the moment #998 lands, is the simd128 win. Automatic. Unambiguous. No bench-source changes required at the PR-merge boundary.

Layer 3 — End-to-end synthetic prove

bench_lifted_stark_prove_blake3

Drives miden-lifted-stark::prove_multi through the complete pipeline: LDE → constraint folding → DEEP composition → FRI → LMCS Merkle commits. Same arithmetic shape any production prove hits, on a synthetic Blake3 AIR with a 4096-row trace.

This is the headline regression-tracking metric. Every PR going forward gets a one-number answer to "did this change slow down the actual prove stack?" — without anyone having to run the wallet's full proving-bench harness manually.

PCS params are calibrated for fast bench runs, NOT production security:

  • log_blowup = 1 (vs miden-vm's 3) — smaller LDE, faster bench
  • folding_pow_bits = 0 — no PoW, no anti-grinding cost
  • num_queries = 27

This is a perf-tracking bench, not a security parameter; only the relative number matters. The arithmetic shape (NTT, ext-field constraint folding, DEEP, FRI, Merkle) is the same as a production prove — that's what makes the metric representative.

The simd128 PR's gain on packed ext-field math should drop this number ~30-50%, since AIR evaluation + NTT (the chunks simd128 actually accelerates) dominate prove time. That gives a third indicator of #998's impact, complementing Layer 2's direct measurement.

Why all three?

Failure mode Layer 1 catches Layer 2 catches Layer 3 catches
Someone slows down RPO round constants
Someone breaks the WIDTH=1 const-fold (regresses scalar to packed-shape codegen) ❌ (also slows down)
Someone breaks packed permutation (e.g. introduces bounds checks)
Someone slows down LDE / FRI / DEEP / Merkle
Someone speeds up the packed permutation lane (e.g. PR #998) ✅ (50%) ✅ (30-50% on prove)

The matrix shows why each layer is load-bearing. Layer 1 alone misses simd128. Layer 2 alone misses non-permutation regressions. Layer 3 alone is too coarse for diagnostics. Together they triangulate.


Architecture

miden-bench-wasm/ — new workspace member

miden-bench-wasm/
├── Cargo.toml          # cdylib + rlib, miden-crypto + miden-lifted-stark deps
├── src/lib.rs          # 14 #[wasm_bindgen] bench functions
├── static/bench.html   # ESM loader page (loads pkg/ from wasm-pack)
├── driver.mjs          # Playwright headless-Chromium driver
├── package.json        # Playwright + serve dev deps
├── package-lock.json   # pinned for CI determinism
└── README.md           # local-run instructions, design rationale, tuning

Each bench function takes either (num_batches, batch_size, warmup) (microbenches) or (num_runs, log_n) (synthetic prove). The driver picks per-bench tuning from BENCH_CONFIG so we can re-balance without rebuilding the WASM.

Why headless Chromium and not wasmtime: V8 (what the wallet ships) and Cranelift (wasmtime's codegen) produce meaningfully different machine code from the same wasm32+simd128 input. SIMD codegen quality can differ 1.5–2×. A wasmtime number could miss a real regression that only shows up in V8. For perf signals we'd actually act on, we need to be measuring what users see.

Why no COOP/COEP: every bench in scope is single-threaded pure compute. No SharedArrayBuffer, no Workers, no rayon, no wasm-bindgen-rayon. Skipping COOP/COEP cuts ~3 s of CI time and removes a known browser SW-reload flake.

.github/workflows/bench.yml — new workflow

Two jobs, separately gated, separately commented:

jobs:
  bench-wasm:    # Headless Chromium / V8
  bench-native:  # cargo bench on Linux x86_64

Both run on pull_request + push to next + workflow_dispatch. Each:

  1. Runs the relevant bench
  2. Uploads raw results as an artifact (full per-batch sample distribution, ~100 KB) — kept 30 days for post-hoc analysis when an alert fires
  3. Calls benchmark-action/github-action-benchmark to:
    • Push the median onto the gh-pages/bench/{wasm,native}/ subdirectory (separate from the existing gh-pages/docs/ deploy used by docs.yml — no collision)
    • Post a sticky PR comment with diff vs the latest next baseline
    • Alert (via comment header + cc @maintainer) on regression > 10% (both wasm and native — see below for the philosophy)

fail-on-alert: false deliberately — a regression alert is a signal for human review, not an automatic block. False-positive comments are cheap to dismiss; false-positive-blocked PRs are expensive.

On the alert threshold itself: 10% for both wasm and native. The instinct on shared GHA Linux runners is to widen the threshold to absorb CPU-contention noise — we explicitly don't do that here. A 15-20% regression IS a regression worth investigating; setting the alarm there normalises noise as acceptable and defeats the point. The right move is to drive variance below the threshold by tuning the bench (longer batches, more samples), not to widen the threshold to fit measured variance.

What's already in place to keep variance under 10%:

  • 20+ ms per batch (from batch_size × per-op cost) so performance.now()'s ~100 µs precision contributes < 1 % timer noise per batch.
  • 50-sample median instead of mean — robust to single-batch outliers from CPU contention.
  • Per-bench tuning so each metric's own profile dictates iteration count.

Tightening further is a roadmap (see miden-bench-wasm/README.md "Noise reduction"):

  1. iai-callgrind for native — instruction-count benchmarking, fully deterministic, drops native variance to ~0%.
  2. Larger / dedicated GHA runnersubuntu-latest-4-cores or buildjet. Cuts shared-neighbour interference, paid by the minute.
  3. CodSpeed for unified WASM + native — SaaS, free for OSS, valgrind-based instruction counting on both sides.
  4. Best-of-N per PR — run each bench 3 times, pick the best median. Doubles CI time but drops variance ~30-50 % on the noisiest benches.

auto-push: ${{ github.event_name == 'push' }} — only push events to next write to gh-pages; PR runs read-only compare against the existing baseline.

.gitignore — wasm-pack output, node_modules, results

miden-bench-wasm/static/pkg/, node_modules/, and results.json are all build artifacts. Repo holds the source.

Cargo.toml (workspace) — miden-bench-wasm added to members

One-line addition. The bench crate uses miden-crypto + miden-lifted-stark workspace deps so its versions stay in lockstep with everything else.


How to read the comment

Every PR will get two sticky comments (one per job):

  • WASM perf (Chromium / V8) — 14 metrics: 10 hash primitive (Layer 1), 3 packed perm (Layer 2), 1 synthetic prove (Layer 3).
  • Native perf (Linux x86_64)hash + word + transpose benchmarks via criterion's --output-format bencher.

For each metric, the comment shows:

  • Current run's median
  • Baseline median (latest next)
  • Percentage delta
  • ⚠️ flag if delta exceeds the regression threshold

To investigate a flagged regression:

  1. Click the workflow artifact link in the comment to download the raw JSON.
  2. The raw JSON has the full per-batch sample distribution — look at IQR / variance to distinguish "real regression" from "noisy CI run".
  3. If real, look at the PR's diff for the obvious culprit. Common ones: cache-line breaks, accidental allocation in a hot loop, wrong feature flag.

To tune a bench's iteration count when variance is too high:

  1. Edit BENCH_CONFIG in miden-bench-wasm/driver.mjs — bump batch_size (more amortisation) or num_batches (more samples for the median).
  2. The 10% threshold is the floor — any need to widen it indicates the bench is too coarse, not that the regression is acceptable.

Local validation

Pre-PR sanity check on Apple M5 (results from node driver.mjs):

Bench                                Median (ns/iter)
─────────────────────────────────────────────────────
blake3_256_merge                                57
blake3_256_sequential_felt_100                 660
keccak256_merge                                160
keccak256_sequential_felt_100                  900
poseidon2_merge                              1 700
poseidon2_sequential_felt_100               21 500
rpo256_merge                                10 900
rpo256_sequential_felt_100                 134 250
rpx256_merge                                 5 750
rpx256_sequential_felt_100                  77 250
rpo256_packed_permute                       11 200   ← matches rpo256_merge ✓ (WIDTH=1 on next)
rpx256_packed_permute                        5 800   ← matches rpx256_merge ✓
poseidon2_packed_permute                     1 675   ← matches poseidon2_merge ✓
lifted_stark_prove_blake3              4 631 200 000   (4.63 s — full prove, log_n=12 trace)

Sanity checks pass:

  • Layer 1 numbers match the README's CPU comparison table (closest column: Apple M4 Max).
  • Layer 2 packed-perm = Layer 1 merge (within IQR) — confirms WIDTH=1 const-fold is intact on next.
  • Layer 3 prove fits inside CI budget (~5 s × 5 runs = 25 s for the bench, ~5 min total job runtime).

Caveats / known limits

  1. First run on next will have an empty baseline. Until the first push to next after merge, PR comparisons will just show "no baseline" for each metric. Not a blocker — fills in automatically on the first next push.
  2. GHA Linux runner variance is real. Single-digit-percent swings between runs on identical code are normal. The 10% threshold is the floor — see the dedicated section above for why we drive variance below the threshold (longer batches, 50-sample medians, per-bench tuning) rather than widening it. If 10% turns out to false-positive in practice, the fix is one of the four roadmap items in miden-bench-wasm/README.md (iai-callgrind, larger runners, CodSpeed, best-of-N), not relaxing the threshold.
  3. No browser matrix yet. Chromium-only on Linux. Firefox / Safari / WebKit produce different SIMD codegen and would catch engine-specific regressions, but the wallet's user base is dominated by Chromium-based browsers, so this is the right initial scope.
  4. Per-phase prove timing not yet exposed. The synthetic prove tracks total wall-clock. Splitting that into LDE / constraint-eval / DEEP / FRI / Merkle phases requires a custom tracing subscriber to capture span timings — ~100 LoC, deliberately deferred. Useful follow-up if a regression alert fires and we want to know which phase moved.

Out-of-scope (explicit)

  • PackedFelt mul/add micro-benches — would directly measure the simd128 lane. Adding them now would build on PR feat(field,crypto,stark): wasm32+simd128 PackedFelt + trait-generic packed Permutation + DEEP/FRI hot-path fix #998's source, coupling this PR to that one. The Layer 2 packed-perm benches already capture the same signal at one level higher.
  • SMT / Merkle Tree / large_smt benches in CI — multi-minute runtimes don't fit per-PR CI. Ideal for a separate nightly bench job (follow-up).
  • Browser engine matrix (Firefox, Safari) — useful but expands CI cost; deferred.

Files changed

File Change Purpose
Cargo.toml +1 line Add miden-bench-wasm to workspace members
Cargo.lock auto Lockfile updates for new deps
.gitignore +5 lines wasm-pack output, node_modules, results.json
.github/workflows/bench.yml +195 lines (new) Two-job CI workflow
miden-bench-wasm/Cargo.toml +66 lines (new) Crate manifest
miden-bench-wasm/src/lib.rs +320 lines (new) 14 bench functions
miden-bench-wasm/static/bench.html +36 lines (new) ESM loader page
miden-bench-wasm/driver.mjs +152 lines (new) Playwright driver
miden-bench-wasm/package.json +14 lines (new) npm dev deps
miden-bench-wasm/package-lock.json +1105 lines (new) npm lockfile
miden-bench-wasm/README.md +160 lines (new) Design + local-run docs

Net: 2 commits, ~2k lines added (most of which is the npm lockfile + the bench harness — actual logic + config is ~600 lines).


cc @adr1anh — this is the response to your Slack comment. Want your eyes on it before merge.

Adds the first automated bench-tracking infrastructure for `miden-crypto`.
Two CI jobs run on every PR + every push to `next`, both feeding
`benchmark-action/github-action-benchmark`:

  - bench-wasm:   Headless-Chromium harness in `miden-bench-wasm/`. Uses
                  `wasm-pack build --target web` + a tiny Playwright
                  driver. Tracks RPO/RPX/Poseidon2/Blake3/Keccak `merge`
                  + `hash_elements(100)` as compiled to wasm32+simd128
                  and run in V8 — the runtime real users ship in.
  - bench-native: `cargo bench --bench {hash,word,transpose}`. Same
                  primitives at native speed, for comparison + native-
                  side regression detection.

Both jobs:
  - Store medians on the `gh-pages` branch under `bench/{wasm,native}/`,
    coexisting with `docs.yml`'s existing `docs/` deploy (no collision —
    docs.yml uses `destination_dir: docs` which only wipes that subdir).
  - Post a sticky PR comment with the diff vs the latest `next` baseline
    (one comment header per job, so wasm and native each get their own).
  - Alert on regression > 15 % (wasm) / 20 % (native). Don't fail the
    workflow — GHA Linux runners share CPUs, false-positive alerts are
    cheap to ignore but a failed PR check would be friction.
  - Upload raw results (per-batch sample distribution) as a workflow
    artifact for post-hoc analysis when an alert fires.

Why headless Chromium and not wasmtime: V8 and Cranelift produce
meaningfully different machine code from the same wasm32+simd128 input;
SIMD codegen quality can differ 1.5–2x. A wasmtime number could miss
real regressions that only show up in V8. For perf signals we'd
actually act on, we want to be measuring what users see.

Why no COOP/COEP: the bench set is single-threaded pure compute. No
SharedArrayBuffer, Workers, rayon, or wasm-bindgen-rayon. Skipping
COOP/COEP cuts ~3 s of CI time and removes the SW-reload flake that
the wallet's prove harness routinely hits.

Native bench triage:
  - In CI: hash, word, transpose (fast, sub-second per group)
  - Excluded for now (revisit): smt, merkle, partial_mt, store
    (moderate runtime, defer until first PR's setup is stable)
  - Permanently excluded: large_smt, large_smt_forest, sparse_path
    (multi-minute; ideal for a separate nightly job)

Local validation:
  - `cargo check -p miden-bench-wasm --target wasm32-unknown-unknown`: clean
  - `wasm-pack build --release --target web`: 780 KB wasm
  - `node driver.mjs`: produced sane medians end-to-end
    (Blake3 merge ~57 ns, Keccak ~163 ns, Poseidon2 ~1.7 µs,
     RPX ~5.7 µs, RPO ~10.7 µs — matches the README's CPU table)
  - `cargo bench --bench hash` with `--output-format bencher`: produces
    the libtest format that `tool: cargo` parses

Addresses review feedback on #998 from @SantiagoPittella:
"we should definitely start tracking WASM performance in the E2E
benchmarking infrastructure to make sure it stays that way!"
…rove

Three-layer tracking, addressing the gap that the original bench set
covered only `merge` / `hash_elements` — both of which go through the
scalar (WIDTH=1) fast path and would show 0% delta on the simd128 PR.

Adds:

  Layer 2 — packed-permutation throughput:
    bench_rpo256_packed_permute
    bench_rpx256_packed_permute
    bench_poseidon2_packed_permute
    Runs permute_mut on `[<Felt as Field>::Packing; 12]`. Resolves to
    `Felt` (WIDTH=1, scalar) on `next` — matches the merge benches
    and confirms the WIDTH=1 const-folded fast path is intact. After
    PR #998 lands, same source resolves to `PackedFelt` (WIDTH=2),
    ns/iter halves. The dashboard step-down on these metrics IS the
    simd128 win, automatic and unambiguous.

  Layer 3 — end-to-end synthetic prove:
    bench_lifted_stark_prove_blake3
    Drives miden-lifted-stark::prove_multi through the full pipeline
    (LDE → constraint folding → DEEP → FRI → LMCS Merkle) on a 4096-
    row Blake3 AIR trace. Headline regression metric: "did this PR
    slow down the actual prove stack?" Same arithmetic shape a real
    prove hits, in a CI-friendly ~5 s budget per run. PCS params
    calibrated for bench speed (log_blowup=1, no PoW) — perf metric,
    not a security parameter.

Local validation:
  - `cargo check -p miden-bench-wasm --target wasm32-unknown-unknown`: clean
  - `wasm-pack build --release --target web`: 1.3 MB wasm (up from
    780 KB; the prove path adds LDE/FRI/Merkle code)
  - `node driver.mjs` end-to-end: all 14 benches produce sane medians
    (Blake3 ~57 ns, RPO ~10.7 µs, packed-perm matching scalar on `next`,
    synthetic prove ~4.6 s/iter)

Notes:
  - Total bench wall-clock ~30 s (10 s microbenches + ~23 s prove).
    Within the 15-min job timeout.
  - Per-phase breakdown of the synthetic prove (LDE / eval_constraints /
    DEEP / FRI separately) is left as a follow-up — would need a custom
    tracing-subscriber to capture span timings, ~100 LoC vs the value
    of total-prove tracking on first cut.
@WiktorStarczewski WiktorStarczewski added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label May 10, 2026
Adrian's reviewer-side concern was right: setting alert thresholds at
15 % wasm / 20 % native effectively normalises CI noise as acceptable
and defeats the purpose. A 15 % regression IS a regression worth
investigating — the right move is to drive variance below the threshold,
not widen the threshold to fit measured variance.

Threshold: 110 % for both wasm and native. Anything above 10 % indicates
either a real regression or a tooling problem; either way, a maintainer
should look.

Variance reduction:
  - num_batches: 30 → 50. More samples → tighter median, robust to
    occasional outliers from shared GHA-runner CPU contention.
  - batch_size: bumped per-bench so each batch is ≥ 20 ms (was 5–10 ms).
    At 20 ms, performance.now()'s ~100 µs precision contributes ~0.5 %
    timer noise per batch — well under the 10 % threshold.
  - synthetic prove: log_n=12 + n=5 → log_n=10 + n=15. More samples in
    similar wall clock (~16 s vs ~23 s). The trace is still big enough
    that LDE / FRI / DEEP / Merkle / constraint folding all run.

Documents the tighter threshold + the tooling roadmap if 10 % proves
hard to hold (iai-callgrind for native instruction-count benches,
larger GHA runners, CodSpeed integration, best-of-N).

Local validation: same medians as the prior run within 1-2 %, n=50 IQR
visibly tighter. Total bench wall clock ~40 s (microbenches: ~24 s,
prove: ~16 s) — still well under the 15-min job timeout.
  - rustfmt: nightly fmt run on the new sources (lifted into the repo's
    rustfmt.toml-driven layout — imports_granularity=Crate, etc.)
  - taplo: TOML formatter ran across miden-bench-wasm/Cargo.toml
  - workspace-lints: added `[lints] workspace = true` so the crate
    inherits the workspace-wide clippy denies (required by
    `make workspace-check`)
  - clippy `unused_qualifications`: replaced two `alloc::vec::Vec`
    references with bare `Vec` (the prelude already imports it). The
    `extern crate alloc` directive is no longer needed and was removed.
  - cargo-shear: added `[package.metadata.cargo-shear] ignored =
    ["getrandom"]`. `getrandom` is a direct dep purely to enable the
    `wasm_js` Cargo feature on the transitive 0.4 version (cargo doesn't
    unify features across majors); no symbol from getrandom is named in
    our source, so cargo-shear correctly notices that, but removing the
    dep would silently break the wasm32 link.
  - silenced two cdylib `cargo build --tests` warnings via `test = false
    + doctest = false` on the lib target — wasm-bindgen exports have no
    Rust-side test surface; they're exercised end-to-end via driver.mjs.
The wasm bench job on GHA timed out at the 15-min cap even though every
bench finished and produced a median in ~2 min. Cause: after `main()`
returns, Node's event loop stayed alive (open file descriptors from the
`npx serve` child's piped stdio + Playwright's lingering handles), so
the script never exited until GHA killed it.

Fix: explicit `process.exit(0)` after main() resolves. SIGKILL on the
serve child alone doesn't reliably clear the parent's piped fds; just
exit when work is done.

Also bumped the wasm job's `timeout-minutes` 15 → 20 to give comfortable
headroom: GHA ubuntu-latest is ~2.7× slower than Apple M5 on these
benches (measured), bringing total step time to ~6-7 min in the typical
case.

Verified locally: driver now exits cleanly in 42s (was hanging
indefinitely). All 14 medians produced as before.

@huitseeker huitseeker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep workflow permissions minimal, pin actions, and avoid mixing untrusted code execution with privileged automation. The neighboring miden-vm non-regression workflows (1 2) are a good local pattern: the benchmark job is read-only, checkout uses persist-credentials: false, and comment/write permissions live in a separate push-only follow-up job.

Comment thread .github/workflows/bench.yml Outdated
Comment thread .github/workflows/bench.yml Outdated
Comment thread Cargo.toml
… gate

Three review-flagged issues:

1. Privilege escalation. The bench jobs run PR-controlled Rust / npm /
   Node while holding contents:write (for gh-pages push) and
   pull-requests:write (for sticky comments). actions/checkout persists
   the GHA token in .git/config by default, giving PR-controlled build
   scripts (build.rs, npm postinstall) access to it before the
   benchmark step runs.

   Split into two layers, mirroring the miden-vm non-regression
   workflows: bench-{wasm,native} run with contents:read and
   persist-credentials:false, upload a results artifact, and never see
   a write token; publish-{wasm,native} check out the default branch
   (trusted ref), download the artifact, and run
   benchmark-action/github-action-benchmark with the privileged token.
   The PR's tree never reaches a step that has write scope.

2. Unpinned actions. Floating @v4 / @v0.4.0 / @v1 tags can be moved by
   the action author. Pinned to commit SHAs with readable
   pin@<version> comments, matching the rest of the repo's workflows:
   - actions/checkout                @34e114876b... pin@v4
   - actions/upload-artifact         @ea165f8d65... pin@v4
   - actions/download-artifact       @d3f86a106a... pin@v4
   - jetli/wasm-pack-action          @0d096b08b4... pin@v0.4.0
   - benchmark-action/github-action-benchmark @52576c92bc... pin@v1.22.1

3. No-std gate corruption. miden-bench-wasm hard-depends on
   miden-crypto/std (browser benches need std for the once_cell real-
   sync path). `make build-no-std` runs
   `cargo build --no-default-features --target wasm32-unknown-unknown`
   without --workspace; with miden-bench-wasm as a default workspace
   member, Cargo feature unification silently enables std on
   miden-crypto and the no-std check passes without actually validating
   the no-std config.

   Added `default-members` to the workspace manifest, excluding
   miden-bench-wasm. `cargo bench` / `wasm-pack` invoke the bench
   crate by explicit package path, so the bench workflow is unaffected.
   Verified locally: build-no-std now finishes without compiling
   miden-bench-wasm.
- actionlint: shellcheck SC2002 'useless cat'. Replaced
  `cat results.json | wc -l` with `wc -l < results.json`.
- zizmor: `artipacked` warned that the publish-* jobs' checkouts left
  the token persisted in .git/config. The benchmark action reads its
  token from the `github-token` input rather than the persisted
  remote credential, so persist-credentials: false is safe here and
  closes the warning.

doc and doc-build are pre-existing failures on next (broken intra-doc
links in merkle/smt/large_forest/backend/persistent/snapshot.rs from
PR #989) — unrelated, not addressed here.

@huitseeker huitseeker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. You might want to update the description/title.

Comment thread .github/workflows/bench.yml Outdated
alert-threshold: "110%"
fail-on-alert: false # warn via comment, don't fail the PR
summary-always: true # keep the diff comment fresh on each run
comment-always: ${{ github.event_name == 'pull_request' }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be gated to same-repo PRs? Fork PRs get a read-only GITHUB_TOKEN, so comment-always: true can make the publish step fail with Resource not accessible by integration when the action tries to write the sticky comment.

# - encryption / dsa: defer pending review of which sub-benches
# are stable.
run: |
cargo bench --bench hash --bench word --bench transpose \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add set -o pipefail here before the pipeline? Right now a failed cargo bench compile or run can be hidden because tee exits successfully, and then the job can upload/publish a bad bench-native.txt.

Comment thread .github/workflows/bench.yml Outdated

publish-native:
name: Publish native bench
needs: bench-native

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these two publish jobs be serialized before they write gh-pages? On a push run both publishers can fetch the same branch, make separate commits, and then race on push. Making publish-native depend on publish-wasm, or merging the publishers, would avoid non-fast-forward failures and lost benchmark updates.

…ass)

Three follow-ups from @huitseeker's second review pass:

1. Fork PRs no longer crash the publish step. GitHub silently strips
   `pull-requests: write` from `GITHUB_TOKEN` for fork PRs even when
   the workflow declares it, so `comment-always: true` would fail
   with 'Resource not accessible by integration'. Gated to same-repo
   PRs (and push events) — fork PRs still get the artifact + workflow
   summary, just no sticky comment.

2. `set -euo pipefail` on both bench shell blocks. Without it,
   `cargo bench ... | tee bench-native.txt` silently turns a compile
   or runtime failure into 'exit 0, bad file uploaded' because tee's
   exit code is the one observed. Same hazard on the wasm side where
   `node driver.mjs > results.json` leaves a partial file behind on
   a panicking driver. Both blocks now fail fast.

3. Publish jobs serialized on gh-pages. On a push-to-next event both
   publishers fetch gh-pages, append a commit, and push concurrently;
   the loser hits non-fast-forward and the metric is lost. Added
   `needs: publish-wasm` to `publish-native` so they run in series.
   Cost: ~20 s on push events only (PR runs don't push). Keeps
   per-job permission scope narrow vs the alternative of merging the
   publishers into one job.
Replaces the fork-PR gate from the previous commit with the right fix:
keep posting the sticky comment on fork PRs by running under
`pull_request_target` instead of `pull_request`.

Under `pull_request`, GitHub silently strips `pull-requests: write`
and `contents: write` from `GITHUB_TOKEN` for fork PRs regardless of
declared `permissions:`. The previous commit worked around this by
skipping the comment on fork PRs entirely — which is the wrong answer:
fork contributors deserve the same perf-tracking signal as same-repo
PRs. `pull_request_target` keeps the token alive on fork PRs because
the workflow runs in the base repo's context.

SAFETY note (and why `pull_request_target` is OK despite the
'pwn-request' risk): the bench jobs (the only steps that execute
PR-supplied code) declare per-job `permissions: contents: read`,
check out the PR head with `persist-credentials: false`, and
reference no `secrets.*`. So even with the PR's tree on disk, no
privileged token is reachable from build.rs / npm postinstall / Node.
The publish jobs check out the BASE ref (which is `pull_request_target`'s
default), so PR-supplied code never touches a step with the write
token. The workflow file itself comes from base under
`pull_request_target`, so a malicious PR can't change the workflow
logic against itself.

Same pattern is used in web-sdk's check-linked-client-pr.yml for the
same reason.
Comment thread .github/workflows/bench.yml Fixed
Comment thread .github/workflows/bench.yml Fixed
GHAS's `actions/cache-poisoning/poisonable-step` rule fired on the
previous `pull_request_target` version of bench.yml: any step under
`pull_request_target` that executes untrusted code holds an implicit
`ACTIONS_RUNTIME_TOKEN` with Actions-cache write, regardless of
declared `permissions:`. The fix is the workflow_run split that
GitHub Security Lab documents
(https://securitylab.github.com/research/github-actions-preventing-pwn-requests/):
untrusted bench in one workflow with no privileged token, trusted
publish in a second workflow that fires via workflow_run.

bench.yml (UNTRUSTED):
- back to `pull_request` trigger (no privilege downgrade concern
  because we no longer try to comment from here)
- workflow- and job-scope `permissions: contents: read`
- bench-wasm + bench-native upload results + a metadata.json
  containing the PR number, head SHA, and event name (fork PRs aren't
  reachable via workflow_run.pull_requests, so we have to stash the
  PR identity ourselves)
- no publish jobs at all

bench-publish.yml (TRUSTED, new):
- triggered by `workflow_run: workflows: [bench]: types: [completed]`
- runs in the BASE repo's context, full write token even for fork PRs
- doesn't execute PR code: only consumes the bench-results JSON
  artifact via cross-workflow download with run-id
- validates the PR-author-controlled metadata.json against tight
  formats (pr_number ^[0-9]+$, pr_head_sha ^[0-9a-f]{40}$, event
  whitelist) before trusting any value from it
- benchmark-action with comment-always disabled (it can't find the
  PR under a workflow_run event anyway); we post our own sticky
  comment via actions/github-script reading the local
  benchmark-data-repository clone for the diff against baseline
- publish-native serialized after publish-wasm to keep gh-pages
  pushes ordered

Side effects:
- The first PR after this lands will have no publish coverage (the
  workflow_run trigger reads bench-publish.yml from `next`, which
  this PR puts there). After merge, future PRs are fully covered.
- Fork PRs now get a sticky comment again — same as before the
  GHAS-driven revert, but via a path the cache-poisoning rule
  accepts.
// populated. The file is a JS assignment, not pure JSON —
// strip the `window.BENCHMARK_DATA = ` prefix and the
// trailing semicolon.
const raw = fs.readFileSync(process.env.DATA_PATH, 'utf8');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native publisher handles the first run by checking whether DATA_PATH exists before reading it, but the WASM publisher reads it unconditionally here. On a fresh setup with no gh-pages benchmark history yet, this throws before the intended "skip comment" path; mirroring the native guard keeps the first PR/push from failing.

gh-pages-branch: gh-pages
# Push baseline updates ONLY on push to next (PR runs compare
# against the existing baseline, they don't update it).
auto-push: ${{ steps.meta.outputs.event == 'push' }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This decision is coming from metadata.json, which was produced in the untrusted benchmark workflow. A fork PR can change the workflow or runner output to make that artifact say event: "push", and then this trusted workflow would publish PR-controlled benchmark data with the base repo token. It would be safer to derive push/comment eligibility from github.event.workflow_run and GitHub API data, then only use artifact metadata after checking it matches the triggering run and PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants