Skip to content

perf(consensus): flush crypto batches at block boundary#350

Open
ValarDragon wants to merge 2 commits into
ironwood-mainfrom
codex/block-boundary-batch-flush
Open

perf(consensus): flush crypto batches at block boundary#350
ValarDragon wants to merge 2 commits into
ironwood-mainfrom
codex/block-boundary-batch-flush

Conversation

@ValarDragon

Copy link
Copy Markdown

Motivation

Semantic block verification near the chain tip can queue proof/signature batch work and then wait for the global MAX_BATCH_LATENCY before the underlying crypto batch starts. Lowering that latency globally would also affect mempool behavior, so this PR adds a targeted block-verification flush instead.

Solution

  • Add Batch::flush() to queue an explicit flush command on the existing batch worker.
  • Add Fallback::primary() so consensus code can flush the primary batched service without changing fallback verification behavior.
  • Register each semantic block verification batch in zebra-consensus::primitives, keyed by the existing per-block shared known_outpoint_hashes Arc.
  • Mark each block transaction once its async checks have had an initial poll, then flush initialized Ed25519, Sapling, and Halo2 batch services when all transactions in the block have reached that boundary.
  • Keep block safety unchanged: the block verifier still awaits all transaction verifier futures before calculating block totals and committing the block.

Tests

  • cargo fmt --all -- --check
  • cargo test -p tower-batch-control --test ed25519 batch_flushes_on_explicit_flush
  • cargo test -p zebra-consensus primitives::tests
  • cargo test -p tower-batch-control --test ed25519
  • cargo check -p zebra-consensus -p tower-batch-control -p tower-fallback --all-targets
  • cargo clippy -p zebra-consensus -p tower-batch-control -p tower-fallback --all-targets -- -D warnings

Specifications & References

None.

Follow-up Work

  • Monitor whether future transaction verifier changes start using additional shared batch services, such as RedJubjub or RedPallas, and add them to the targeted flush helper if needed.

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: OpenAI Codex for implementation, tests, validation, and PR description.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

@v12-auditor

v12-auditor Bot commented Jul 1, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found two issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-94544 🔵 Low
Empty-batch flush skip can strand zero-weight items

The new early-return in the batch worker's flush_service() treats pending_items_weight == 0 as proof that the batch is empty and returns without ever issuing BatchControl::Flush to the inner service, also clearing the latency timer. However, RequestWeight::request_weight() is a public trait method returning an unconstrained usize, and process_req() still calls the inner service and hands the caller a response future for any item, even one contributing zero weight. For a flush-driven inner service (the ed25519/sapling/halo2 verifiers only compute and broadcast results on BatchControl::Flush), a zero-weight item is accepted into a batch whose 100ms timer now no-ops, so its response future never resolves. This is a behavioral regression introduced by this diff: previously flush_service() unconditionally forwarded BatchControl::Flush, so such an item would be verified. In the in-repo Zebra verifiers this is not currently reachable, because the only non-default weight impl is halo2 Item returning bundle.actions().len() and Orchard bundles are AtLeastOne<AuthorizedAction> (≥1 action), while every other verifier item uses the default weight of 1.

F-94545 🔵 Low
Explicit batch flush bypasses queue backpressure

The new public Batch::flush() method sends Message::Flush directly on the worker's mpsc::UnboundedSender and returns as soon as the message is queued, without calling poll_ready() or acquiring a semaphore permit. Item submission via call() is bounded by a semaphore sized max_items_weight_in_batch * max_batches_in_queue, but flush messages are not counted by that bound. The worker only receives from the channel when can_spawn_new_batches() is true, so while concurrent_batches.len() >= max_concurrent_batches the receive branch is disabled and queued flush messages are not drained. A holder of a Batch handle that calls flush() repeatedly while all batch slots are busy can therefore grow the unbounded channel without limit. In Zebra's consensus path this is not attacker-amplifiable: flush_block_verifier_batches() is invoked at most once per block (guarded by flush_queued) and emits at most five flushes, mempool verification never triggers a flush, and concurrent block verification is bounded, so flush volume stays small.

Analyzed seven files, diff 831c87a...3cd0079.

@ValarDragon
ValarDragon marked this pull request as ready for review July 5, 2026 07:27
@ValarDragon

Copy link
Copy Markdown
Author

Fable comments:

Verdict: safe — no race in this PR can cause a block to skip or corrupt verification

I traced every concurrency path in PR #350 (explicit crypto-batch flush at the block boundary) and ran its concurrency tests. The design has one property that makes it robust: the flush is purely advisory. It only starts pending batch work earlier; it never decides whether something gets verified. Every race I found degrades to extra latency (falling back to the pre-existing 100ms timer), never to skipped verification.

Why verification can't be skipped, regardless of flush timing

  1. The block verifier still awaits every transaction future. zebra-consensus/src/block.rs:310 loops async_checks.next().await over all transaction verifier futures and fails the whole block on any error, before summing fees/sigops and committing. The flush machinery doesn't touch this.
  2. Each batch item has its own oneshot result channel. Whichever batch an item lands in (flushed early, flushed by another block, or flushed by the timer), its result is delivered to its caller. The worker's flush_service never drops items — the new empty-batch early return only skips flushing when there's literally nothing pending.
  3. If the flush never fires or fires early, the old triggers still exist. Max batch size and the 100ms MAX_BATCH_LATENCY timer are unchanged, so worst case is exactly today's behavior.

Races I traced and their outcomes

  • Arc-address key reuse (ABA): The registry key is Arc::as_ptr(known_outpoint_hashes). A stale increment hitting a different block's entry would require the address to be reused while a key-holding future from the old block is still alive. That can't happen: the tx verifier futures are polled inside the block verifier's FuturesUnordered (the tower Buffer only constructs the lazy future and hands it back — nothing is spawned detached, confirmed in router.rs:362-385), so they're dropped together with the block future; and the guard is declared after the Arc in block.rs:277-282, so on drop the registry entry is removed while the Arc still pins the address. Even if this reasoning ever broke, a stale increment would only trigger a premature flush — latency, not correctness.
  • Flush firing before a transaction's batch items are queued: the yield_now heuristic in AsyncChecks::check isn't a hard guarantee (an item can stall on the batch semaphore, though the bound is max_items × max_batches, so it's rare). If it happens, that item just waits for the timer.
  • Lazy::get returning None while another thread is initializing a verifier: items land in the initializing batch and get flushed by the timer. Latency only.
  • Cancellation/error paths: if a tx fails a quick check or check() errors, its increment never happens and the block flush never fires — but the block fails anyway, and the guard removes the registry entry on drop (no leak; verified by the guard test). Mempool requests pass None and the disabled select branch means the completed-future-repoll panic can't happen — the needs_block_batch_flush flag correctly gates the &mut block_batch_flush poll.
  • Deadlock: the registry mutex is never held across an await — the lock lives inside the sync block_verifier_batch_flush_ready, and flush() is a non-blocking unbounded-channel send.
  • Ordering: flush and items share one FIFO mpsc channel, so a flush sent after the last increment flushes everything queued before it.

Coverage is also complete: every batched service used in block transaction verification (ed25519, sapling, the three per-NU halo2 verifiers) is flushed; groth16 joinsplits are an unbatched ServiceFn, and redjubjub/redpallas aren't in the tx path on this

  • Ordering: flush and items share one FIFO mpsc channel, so a flush sent afs everything queued before it.

Coverage is also complete: every batched service used in block transaction g, the three per-NU halo2 verifiers) is flushed; groth16 joinsplits are an unbatched ServiceFn, and redjubjub/redpallas aren't in the tx path on this
Test evidence In a temp worktree at the PR head: tower-batch-control --test ed25519 (4 teit-flush test) and zebra-consensus primitives::tests (2 registry tests) all pass. Two non-safety notes worth raising in review 1. Throughput under concurrent block verification. During near-tip catch-upt once, each block's flush cuts short batches that other blocks are filling, shrinking average batch size and losing batch-verification amortization. Itghput tradeoff, but worth a benchmark or a mention in the PR.
2. Undocumented drop-order dependency. The safety of the registry cleanup reing declared after known_outpoint_hashes (reverse drop order). A comment
there would keep a future refactor from silently reordering them — though emode is latency, not correctness.

…ase tests

- Document that the block batch flush guard must be declared after
  known_outpoint_hashes, so the registry entry is removed while the Arc
  address is still pinned and cannot be reused by another block's key.
- Test that an explicit flush on an empty batch is a no-op and later
  batches still verify.
- Test that re-registering a reused flush key starts a fresh transaction
  count instead of inheriting a stale one.
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