Skip to content

feat!: batch prover input run#578

Open
antoniolocascio wants to merge 4 commits intodraft-0.4.0from
alocascio-batch-prover-input-run
Open

feat!: batch prover input run#578
antoniolocascio wants to merge 4 commits intodraft-0.4.0from
alocascio-batch-prover-input-run

Conversation

@antoniolocascio
Copy link
Copy Markdown
Contributor

@antoniolocascio antoniolocascio commented Mar 19, 2026

What ❔

This PR adds native prover-input generation for a whole batch in ZKsync OS.

  • Introduces a batch runner that executes all blocks natively, returns canonical batch prover input and canonical batch pubdata, and derives each next block's ProofData internally. The batch now runs against one mutable BatchState, which is updated after each block with storage writes and published preimages.
  • Adds pubdata_used to BlockOutput in zksync-os-interface and plumbs it through forward execution. This field is for sealing/accounting only; canonical pubdata bytes still come from prover-input generation.
  • Updates the multiblock tests to compare against legacy approach.

Why ❔

Is this a breaking change?

  • Yes
  • No

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted.

@antoniolocascio antoniolocascio changed the title feat1 feat!: batch prover input run Mar 19, 2026
@antoniolocascio antoniolocascio marked this pull request as draft March 19, 2026 07:21
@antoniolocascio antoniolocascio changed the base branch from main to alocascio-prover-input-run March 19, 2026 07:22
@antoniolocascio antoniolocascio force-pushed the alocascio-batch-prover-input-run branch 3 times, most recently from ca70480 to 700c360 Compare March 19, 2026 10:13
@antoniolocascio antoniolocascio marked this pull request as ready for review March 20, 2026 06:32
@antoniolocascio-bot
Copy link
Copy Markdown

Code Review Summary

Reviewed by: Claude + Codex (3 rounds each, 6 total review passes)
Findings: 0 critical, 3 major, 5 minor, 1 nit


Critical

None found

Major

1. BatchIndex saturates instead of failing fast — silent corruption risk
forward_system/src/run/batch.rs:55-64current() clamps to len - 1 and advance() silently no-ops past the end. The host loop is correctly structured today, but if the ordering ever drifts (e.g. a future refactor adds a query after the last block), the code will silently serve the last block's metadata/tx source instead of surfacing an invariant violation. For security-critical witness construction, failing fast (assert/panic) is the safer default. (found by: Codex R1, Codex R3)

2. generate_batch_proof_input panics on caller-controlled input instead of returning Err
forward_system/src/run/mod.rs:289-292 — The function returns Result<BatchRunOutput, ForwardSubsystemError> but uses assert!(!blocks.is_empty()) which aborts instead of returning a typed error. Per AGENTS.md ("Prefer returning typed validation/internal errors"), empty batches should come back as Err, not a panic. The negative test at tests/instances/multiblock_batch/src/lib.rs:232-241 locks in this behavior with catch_unwind. (found by: Codex R1, Codex R3)

3. New BatchRunOutput fields largely untested
tests/instances/multiblock_batch/src/lib.rs — The test verifies byte-for-byte prover_input equality and two timestamps, but does not verify BatchRunOutput.pubdata, BatchRunOutput.batch_public_input, or individual block_outputs (including per-block pubdata_used). Since pubdata_used threading is a new, independent change in this PR, the lack of coverage for that path is a meaningful gap. (found by: Codex R3)

Minor

4. prepare_batch_block_input() doesn't advance chain state — easy to misuse
tests/rig/src/chain.rs:614-641 — The helper derives block_number and block_hashes from the chain's current height without advancing state. Calling it repeatedly on the same Chain produces duplicate metadata. The test works around this by sourcing block 2 input from a separately advanced legacy_tester, which hides the helper's design issue. (found by: Codex R1, Codex R3)

5. BatchProverInputSystemTypes duplicates most of ForwardSystemTypes<O, true>
forward_system/src/system/system_types/mod.rs:134-175 — The two types share almost all associated types; only PostTxLoopOp, BlockDataKeeper, and TxLoopOp differ. If ForwardSystemTypes gains new associated types, BatchProverInputSystemTypes needs the same update or will silently diverge. (found by: Codex R3)

6. Legacy witness-stitching edge case (blob_advice_words == 0) not regression-tested
forward_system/src/run/mod.rs:251-267 — The stripping logic correctly falls through when blob_advice_words == 0, but there's no targeted test for this edge case (a block with BlobsZKsyncOS DA but no pubdata producing zero blob advice). (found by: Codex R3)

7. BLOCK_PUBDATA_OVERHEAD_BYTES applied in sequencing post-op too
basic_bootloader/.../post_tx_op_sequencing.rs — The 41-byte overhead (1 + 32 + 8) is added to pubdata_used in all three post-tx-op variants including the sequencer path. Verify this is intentional — the sequencer's BlockOutput.pubdata_used will include protocol framing overhead, not just raw pubdata bytes. (found by: Claude context review)

8. Negative test locks in panic behavior instead of pushing toward typed error
tests/instances/multiblock_batch/src/lib.rs:232-241 — The empty-batch test uses catch_unwind to assert a panic, codifying the abort path. If finding #2 is addressed, this test should assert Err instead. (found by: Codex R3)

Nits

9. reconnect_external_oracle() — unused dead surface area
oracle_provider/src/lib.rs:94-96, 322-324 — Added on both ZkEENonDeterminismSource and ReadWitnessSource but has no call sites in this PR. If intended for follow-up work, consider adding with the call site or documenting the intent. (found by: Codex R3)


Automated review by Claude Code

@antoniolocascio-bot
Copy link
Copy Markdown

Addendum — Additional Findings from Late-Completing Review Agents

Three additional findings surfaced after the initial review post. Two are Major.

Major

10. BatchProverInputSystemTypes uses NopTxHashesAccumulator — batch block_outputs will have zeroed tx hashes
forward_system/src/system/system_types/mod.rs:164 — The single-block prover-input system uses TransactionsRollingKeccakHasher for its BlockDataKeeper, but BatchProverInputSystemTypes uses NopTxHashesAccumulator. This means BatchRunOutput.block_outputs will have zeroed-out transactions_rolling_hash in their block headers. This is likely intentional (the batch-level accumulator in ZKBatchDataKeeper handles L1 tx hashes), but any consumer relying on accurate per-block transaction hashes from block_outputs will get incorrect data. (found by: Claude R1, Claude R2, Claude R3)

Severity upgrade: Finding #7 (BLOCK_PUBDATA_OVERHEAD_BYTES in sequencing) → Major
Multiple review agents independently flagged this as Major rather than Minor. Adding BLOCK_PUBDATA_OVERHEAD_BYTES (41 bytes) to the sequencing post-op is a protocol-level behavior change: the sequencer's BlockOutput.pubdata_used now includes encoding overhead that was previously absent (the default record_block_pubdata_used was a no-op). This could affect fee calculations, pubdata limit enforcement, and downstream consumers. Please validate this is intentional. (found by: Claude R1, Claude R2, Claude R3)

Minor

11. Missing single-block batch test
tests/instances/multiblock_batch/src/lib.rs — The tests exercise 2-block batches and empty batches, but not a single-block batch via generate_batch_proof_input. This is an important boundary case: batch_index.advance(), batch_state.apply_block_output(), and proof_data.set() are all skipped in the 1-block path. (found by: Claude R3)


Automated review addendum by Claude Code

@antoniolocascio antoniolocascio force-pushed the alocascio-batch-prover-input-run branch from d2d003e to a6808ca Compare March 23, 2026 07:48
@antoniolocascio antoniolocascio force-pushed the alocascio-prover-input-run branch from 7687669 to 088e142 Compare March 25, 2026 06:57
Base automatically changed from alocascio-prover-input-run to draft-0.4.0 March 30, 2026 12:01
@antoniolocascio antoniolocascio force-pushed the alocascio-batch-prover-input-run branch from 87ca565 to 8b98b50 Compare March 31, 2026 12:12
@antoniolocascio antoniolocascio force-pushed the alocascio-batch-prover-input-run branch from 4a9d958 to de4da02 Compare March 31, 2026 14:09
@github-actions
Copy link
Copy Markdown
Contributor

Benchmark report

Benchmark Symbol Base Eff Head Eff (%) Base Raw Head Raw (%) Base Blake Head Blake (%) Base Bigint Head Bigint (%)
block_19299001 process_block 300,182,822 300,182,822 (+0.00%) 264,851,734 264,851,734 (+0.00%) 410,630 410,630 (+0.00%) 7,190,252 7,190,252 (+0.00%)
block_22244135 process_block 185,145,000 185,145,000 (+0.00%) 164,426,516 164,426,516 (+0.00%) 172,040 172,040 (+0.00%) 4,491,461 4,491,461 (+0.00%)
precompiles bn254_ecadd 53,268 53,268 (+0.00%) 47,816 47,816 (+0.00%) 0 0 (+0.00%) 1,363 1,363 (+0.00%)
precompiles bn254_ecmul 728,781 728,781 (+0.00%) 564,593 564,593 (+0.00%) 0 0 (+0.00%) 41,047 41,047 (+0.00%)
precompiles bn254_pairing 72,336,733 72,336,733 (+0.00%) 57,808,589 57,808,589 (+0.00%) 0 0 (+0.00%) 3,632,036 3,632,036 (+0.00%)
precompiles ecrecover 385,308 383,409 (-0.49%) 258,344 257,261 (-0.42%) 0 0 (+0.00%) 31,741 31,537 (-0.64%)
precompiles id 927 927 (+0.00%) 927 927 (+0.00%) 0 0 (+0.00%) 0 0 (+0.00%)
precompiles keccak 137,579 137,579 (+0.00%) 137,579 137,579 (+0.00%) 0 0 (+0.00%) 0 0 (+0.00%)
precompiles modexp 31,267,857 31,267,857 (+0.00%) 20,610,037 20,610,037 (+0.00%) 0 0 (+0.00%) 2,664,455 2,664,455 (+0.00%)
precompiles p256_verify 748,861 748,861 (+0.00%) 470,169 470,169 (+0.00%) 0 0 (+0.00%) 69,673 69,673 (+0.00%)
precompiles process_block 147,363,433 147,362,521 (-0.00%) 117,964,969 117,964,569 (-0.00%) 5,140 5,160 (+0.39%) 7,329,056 7,328,848 (-0.00%)
precompiles process_transaction 73,393,283 73,394,757 (+0.00%) 58,732,659 58,740,545 (+0.01%) 160 160 (+0.00%) 3,664,516 3,662,913 (-0.04%)
precompiles ripemd 8,013 8,013 (+0.00%) 8,013 8,013 (+0.00%) 0 0 (+0.00%) 0 0 (+0.00%)
precompiles run_tx_loop 146,713,258 146,711,734 (-0.00%) 117,394,154 117,393,462 (-0.00%) 180 180 (+0.00%) 7,329,056 7,328,848 (-0.00%)
precompiles sha256 13,168 13,168 (+0.00%) 13,168 13,168 (+0.00%) 0 0 (+0.00%) 0 0 (+0.00%)
precompiles system_init 46,790 46,790 (+0.00%) 46,790 46,790 (+0.00%) 0 0 (+0.00%) 0 0 (+0.00%)
precompiles verify_and_apply_batch 146,744 146,744 (+0.00%) 110,424 110,424 (+0.00%) 2,270 2,270 (+0.00%) 0 0 (+0.00%)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants