Skip to content

fix: batch verifier review fixes#21644

Merged
ludamad merged 2 commits into
lw/batch-verifier-service-v3from
lw/batch-verifier-review-fixes
Mar 17, 2026
Merged

fix: batch verifier review fixes#21644
ludamad merged 2 commits into
lw/batch-verifier-service-v3from
lw/batch-verifier-review-fixes

Conversation

@ludamad

@ludamad ludamad commented Mar 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Review fixes for #21460 (batch chonk verifier service), addressing findings from a 4-agent code review:

  • S1 — Shell injection: Replace execSync(mkfifo ...) with execFileSync('mkfifo', [...]) in production code (batch_chonk_verifier.ts) and test code (batch_verifier.test.ts). The bench and queue test files already used the safe form.
  • S2 — Silent truncation: Add BB_ASSERT(len <= UINT32_MAX) in write_frame before the uint32_t cast to catch >4GiB payloads instead of silently truncating.
  • P1 — Bisection optimization: When bisecting a failed batch, check the left half first. If it passes, all failures must be in the right half — skip the redundant batch_check call. This halves bisection cost for the common single-bad-proof case (3 batch_checks instead of 6 for a batch of 8).
  • P2 — O(1) queue front-erase: Change queue_ from std::vector to std::deque so front-erasure in coordinator_loop is O(1) instead of O(n).
  • P4 — Single syscall write: Combine the 4-byte header and payload into a single buffer in write_frame for one write() syscall instead of two.
  • P5 — Default harmonization: Change C++ BatchVerifierConfig::batch_size default from 4 to 8 to match the TypeScript default.

Test plan

  • Existing batch verifier tests (batch_verifier_queue.test.ts, batch_verifier.test.ts) cover all bisection patterns including random patterns with multiple bad proofs — these validate the P1 bisection optimization.
  • The execFileSync change is a drop-in replacement with identical behavior for valid paths.
  • The std::deque change is API-compatible with std::vector for all operations used.

AztecBot added 2 commits March 17, 2026 01:32
…ness

- Replace execSync(`mkfifo ...`) with execFileSync('mkfifo', [...]) in production
  and test code to eliminate shell injection surface (S1)
- Add BB_ASSERT(len <= UINT32_MAX) bounds check in write_frame to prevent silent
  truncation on >4GiB payloads (S2)
- Optimize bisection to skip redundant batch_check on the passing half — when left
  half passes, all failures must be in right half, saving ~50% bisection cost for
  the common single-bad-proof case (P1)
- Change queue_ from std::vector to std::deque for O(1) front erasure instead of
  O(n) element shifting (P2)
- Consolidate write_frame into a single write() syscall by combining header and
  payload into one buffer (P4)
- Harmonize batch_size defaults: C++ default 4 → 8 to match TypeScript default (P5)
…rrency into batch_check

- Replace execFileSync/execSync with async execFileAsync (promisify(execFile))
- Replace unlinkSync/mkdirSync/writeFileSync with fs/promises (unlink, mkdir, writeFile)
- Keep unlinkSync only in signal handlers where async is not allowed
- Move set_parallel_for_concurrency(num_cores_) into batch_check so callers
  don't need to remember it — simplifies bisection and coordinator_loop code
@ludamad ludamad merged commit 5d02f51 into lw/batch-verifier-service-v3 Mar 17, 2026
6 of 9 checks passed
@ludamad ludamad deleted the lw/batch-verifier-review-fixes branch March 17, 2026 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant