feat: multi blob#935
Open
chengwenxi wants to merge 45 commits into
Open
Conversation
- Add isBatchV2Upgraded hook to BatchCache so V2 header is always generated once the upgrade is activated, regardless of blob count. Previously the code fell back to V1 for single-blob batches, which is incompatible with the V2 public_input_hash (keccak(hash[0]) ≠ hash[0]). - Remove the MAX_BLOB_PER_BLOCK = 6 constant from Rollup.sol and rely solely on blobhash(i) == bytes32(0) to terminate the blob-count loop. Per spec §9 design decision, blob count limits should be controlled by tx-submitter MaxBlobCount config, not a hardcoded contract constant, so Ethereum protocol upgrades (e.g. EIP-7691) require no contract change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… challenge handler - Change ExecutorInput.blob_info to blob_infos (Vec<BlobInfo>) with batch_version field - Add BlobVerifier::verify_blobs for multi-blob KZG verification - Add BatchInfo::public_input_hash_v2 using keccak256(hash[0]||...||hash[N-1]) - Add multi-blob encoding (encode_multi_blob, encode_blob_from_bytes) in host blob.rs - Route verify() on batch_version: V2 uses aggregated blob hashes, V0/V1 unchanged - Update shadow_rollup calc_batch_pi to parse V2 header with blob_count at offset 257 - Add blob_count and extra_blob_hashes to challenge handler BatchInfo and encode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… entrypoints - Add batch_version to ProveRequest and thread through gen_client_input/execute_batch - Use verify_blobs (all blobs) instead of verify (first blob only) in server queue - Compute blobHashesHash for V2 in server batch_header_ex; pass individual hashes for fill_ext - fill_ext parses V2 blob_count + per-blob hashes from extended batch_header_ex - Add batch_version param to try_execute_batch; callers extract version from batch_header[0] - Add --batch-version CLI arg to host binary - Add blob_count param to execute_batch for correct PI hash routing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…V0/V1 compat fields Replace blob_versioned_hash + blob_count + extra_blob_hashes with a single blob_hashes: Vec<[u8; 32]>. fill_ext parses all hashes from batch_header_ex, encode writes blob_hashes[0] at offset 57 and appends count + remaining hashes for V2. No backward-compatibility shims needed since prover components upgrade together. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds BatchHeader V2 and multi-blob support across blob utilities, batch cache packing/sealing, node/prover flows, Rollup contract, tx-submitter wiring, gas-oracle sizing, tests, and infra updates. ChangesBatch V2 header, interfaces, and shared blob/encoding utilities
Batch cache V2 enablement and tests
Solidity Rollup V2 and tests
Node DA verification and batch parsing
Prover/executor multi-blob inputs and hashing
Gas-oracle multi-blob changes
Tx-submitter refactor, flags, config, infra
Sequence Diagram(s)sequenceDiagram
participant Cache
participant L2Gov
participant BlobUtils
participant TxSubmitter
participant Rollup
participant Beacon
participant Prover
Cache->>L2Gov: BatchBlockInterval/BatchTimeout
Cache->>BlobUtils: CompressBatchBytes -> MakeBlobTxSidecar
Cache->>TxSubmitter: Emit V2 header (aggregated blob hashes)
TxSubmitter->>Rollup: commitBatch(V<=2, sidecar)
Rollup-->>TxSubmitter: committed
TxSubmitter->>Beacon: fetch sidecars by tx
Beacon-->>TxSubmitter: blobs+commitments
Prover->>Prover: verify_blobs -> versioned_hashes
Prover->>Prover: public_input_hash_v2(aggregated hash)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…mpress once Split get_origin_batch into unpack_blob (field-element unpack) and decompress_batch (zstd decompress). verify_blobs now KZG-verifies each blob independently, unpacks all compressed chunks, concatenates them, then calls decompress_batch once. Previously each blob was independently decompressed which fails for N>1 since the zstd frame spans all chunks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fset 57 V2 headers now use the same 257-byte format as V1 with the aggregated blob hash (keccak256 of all blob hashes) at offset 57. This eliminates BatchHeaderCodecV2, simplifies contracts/prover/submitter, and fixes the multi-blob decompression bug in blob_verifier. - Delete BatchHeaderCodecV2.sol; V2 commitBatch computes aggregated hash inline - Unify _verifyProof and _loadBatchHeader for all versions - Remove BatchHeaderV2 struct in Go; V2 uses V1 format + version override - Simplify Rust challenge handler, queue, shadow_rollup (uniform 96-byte batch_header_ex) - Fix verify_blobs: decode BLS scalars per blob, concatenate, decompress once Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
V2 should store the aggregated blob hash (keccak256 of all blob hashes) in batchBlobVersionedHashes, consistent with the header offset 57 value, instead of blobhash(0). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…struction Move blobVersionedHash computation out of _commitBatchWithBatchData into callers via a new _computeBlobVersionedHash(version) helper: - V0/V1: blobhash(0) or ZERO_VERSIONED_HASH - V2: keccak256(blobhash(0)||...||blobhash(N-1)), requires >=1 blob _commitBatchWithBatchData now has a single unified header construction path for all versions — no more V2/V0V1 branch split. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the V2 restriction in commitState — with the simplified V2 header format (aggregated hash at offset 57), the stored batchBlobVersionedHashes value is sufficient to recommit without a blob, same as V0/V1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rsionedHash - Move `require(_blobCount > 0)` before the keccak256 assembly block to avoid computing keccak256 on empty input when no blobs are attached - Add multi-blob aggregated hash unit tests to RollupCommitBatchV2Test: single/two/three blob correctness, order-sensitivity, and _verifyProof publicInputHash alignment with V2 aggregated hash semantics - Add Rust unit tests for public_input_hash_v2 in prover-executor-client: single/two/three blob correctness, order-sensitivity, and structural difference from V1 (V2 uses keccak(h0||...||hN-1), not raw hash) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: corey <corey.zhang@bitget.com>
Co-authored-by: corey <corey.zhang@bitget.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve conflicts after PR #939 (remove sequencer batch generation): - node/types/batch_test.go: keep HEAD (multi_batch) version; it carries TestBatchHeaderV2 / TestBlobHashesHashUnavailableForLegacy which still exercise BatchHeaderV0/V1/V2 helpers retained on this branch. - go.work.sum: union both sides (toolchain entries from x/exp, x/mod, x/tools); identical go directive on both branches. - go-ethereum submodule: take main's 045be0fd (v2.2.2, includes the matching "Remove sequencer batch write paths" change for #939) over multi_batch's older 62952ec7. - common/go.mod: bump tendermint replace from v0.3.4 -> v0.3.7 to align with node/tx-submitter/contracts/oracle/ops modules; common itself has no tendermint imports, the directive was stale and was the cause of "conflicting replacements for github.com/tendermint/tendermint" in the workspace build. Co-authored-by: Cursor <cursoragent@cursor.com>
…/batch (#945) Co-authored-by: corey <corey.zhang@bitget.com> Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ops/docker/docker-compose-4nodes.yml (1)
485-497:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse one batch-V2 cutover config for every submitter.
This compose file now enables V2 batching only on
tx-submitter-0: it hardcodesTX_SUBMITTER_BATCH_V2_UPGRADE_TIMEandTX_SUBMITTER_SEAL_BATCH, while themulti-submitterinstances can be started without either flag. In the same file the nodes read${BATCH_UPGRADE_TIME}, so enabling that profile can put submitters and nodes on different batch/header versions.Suggested config alignment
tx-submitter-0: environment: - - TX_SUBMITTER_BATCH_V2_UPGRADE_TIME=1777533291 + - TX_SUBMITTER_BATCH_V2_UPGRADE_TIME=${BATCH_UPGRADE_TIME} - TX_SUBMITTER_SEAL_BATCH=true tx-submitter-1: environment: + - TX_SUBMITTER_SEAL_BATCH=true + - TX_SUBMITTER_BATCH_V2_UPGRADE_TIME=${BATCH_UPGRADE_TIME} tx-submitter-2: environment: + - TX_SUBMITTER_SEAL_BATCH=true + - TX_SUBMITTER_BATCH_V2_UPGRADE_TIME=${BATCH_UPGRADE_TIME} tx-submitter-3: environment: + - TX_SUBMITTER_SEAL_BATCH=true + - TX_SUBMITTER_BATCH_V2_UPGRADE_TIME=${BATCH_UPGRADE_TIME}Also applies to: 499-541, 542-584, 585-626
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ops/docker/docker-compose-4nodes.yml` around lines 485 - 497, The compose file enables batch-V2 only for tx-submitter-0 by hardcoding TX_SUBMITTER_BATCH_V2_UPGRADE_TIME and TX_SUBMITTER_SEAL_BATCH while other submitters (multi-submitter) lack them; change the submitter service environment blocks so all submitter instances use the same batch cutover vars (reference TX_SUBMITTER_BATCH_V2_UPGRADE_TIME and TX_SUBMITTER_SEAL_BATCH) and/or point them to the shared ${BATCH_UPGRADE_TIME} variable used by the nodes; update the tx-submitter-0 and multi-submitter service env lists so they consistently include TX_SUBMITTER_BATCH_V2_UPGRADE_TIME (or ${BATCH_UPGRADE_TIME}) and TX_SUBMITTER_SEAL_BATCH to ensure all submitters flip to batch-V2 together.common/batch/batch_cache_test.go (1)
28-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't block a test on manual
SIGINT.
TestBatchCacheInitServernever returns unless someone sendsos.Interrupt, sogo testcan hang indefinitely in CI and local runs. Please gate this as a manual/integration test or give it a bounded timeout and stop the signal handler on exit.Suggested fix
func TestBatchCacheInitServer(t *testing.T) { + if os.Getenv("RUN_BATCH_CACHE_INIT_SERVER_TEST") == "" { + t.Skip("manual integration test; set RUN_BATCH_CACHE_INIT_SERVER_TEST=1 to run") + } + testDB := openTestKV(t) cache := NewBatchCache(nil, nil, 2, l1Client, &SingleL2Client{C: l2Client}, rollupContract, l2Gov, testDB) @@ interrupt := make(chan os.Signal, 1) signal.Notify(interrupt, os.Interrupt) - <-interrupt + defer signal.Stop(interrupt) + + select { + case <-interrupt: + case <-time.After(30 * time.Second): + t.Fatal("timed out waiting for TestBatchCacheInitServer to finish") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/batch/batch_cache_test.go` around lines 28 - 59, TestBatchCacheInitServer currently blocks waiting for os.Interrupt and can hang CI; change it to be bounded or skipped: either (A) skip in normal runs by adding at the top if testing.Short() { t.Skip("manual/integration test") } or (B) enforce a timeout by replacing the blocking interrupt wait with a context.WithTimeout(ctx, <duration>) and select on ctx.Done() instead of <-interrupt, and ensure you stop the signal handler by calling signal.Stop(interrupt) and closing the channel when the test exits. Update references to TestBatchCacheInitServer, interrupt, InitAndSyncFromDatabase, and AssembleCurrentBatchHeader accordingly so the goroutines exit when the test context cancels.node/derivation/batch_info.go (1)
127-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate block-number ordering before computing
blockCount.Both subtraction paths can underflow if a malformed batch reports a
LastBlockNumbersmaller than the decoded start/parent block. That turnsblockCountinto a huge value and later blows up allocation/slicing in this parser. Reject the batch before subtracting when the numbers go backwards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/derivation/batch_info.go` around lines 127 - 145, Before computing blockCount, validate that batch.LastBlockNumber is not less than the reference block to avoid unsigned underflow: in the parentVersion == 0 branch, after decoding startBlock (BlockContext.Decode on batchBytes[:60]) check that batch.LastBlockNumber >= startBlock.Number and return a clear error if not; in the else branch, after obtaining parentBatchBlock from parentBatchHeader.LastBlockNumber(), check that batch.LastBlockNumber >= parentBatchBlock and return an error if not. Keep the existing blockCount formulas (batch.LastBlockNumber - startBlock.Number + 1 and batch.LastBlockNumber - parentBatchBlock) but only perform them after these validations.tx-submitter/services/rollup.go (1)
1260-1261:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential truncation when converting gas caps to uint256.
tip.Int64()andgasFeeCap.Int64()truncate values exceedingmath.MaxInt64(~9.2 ETH in wei). While unlikely under normal conditions, extreme gas spikes could cause silent data loss. Pass the*big.Intdirectly touint256.MustFromBig.Proposed fix
return ethtypes.NewTx(ðtypes.BlobTx{ ChainID: uint256.MustFromBig(r.chainId), Nonce: nonce, - GasTipCap: uint256.MustFromBig(big.NewInt(tip.Int64())), - GasFeeCap: uint256.MustFromBig(big.NewInt(gasFeeCap.Int64())), + GasTipCap: uint256.MustFromBig(tip), + GasFeeCap: uint256.MustFromBig(gasFeeCap), Gas: gas, To: r.rollupAddr, Data: calldata,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tx-submitter/services/rollup.go` around lines 1260 - 1261, Replace the truncating Int64 conversions for gas caps by passing the original *big.Int values into uint256.MustFromBig: locate the struct/assignment where GasTipCap and GasFeeCap are set (the lines using GasTipCap, GasFeeCap, uint256.MustFromBig, and the tip and gasFeeCap variables) and change the arguments from big.NewInt(tip.Int64()) and big.NewInt(gasFeeCap.Int64()) to pass tip and gasFeeCap directly to uint256.MustFromBig so no data is silently truncated.prover/crates/executor/client/src/verifier/blob_verifier.rs (1)
48-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
unwrap()calls with proper error handling in lines 48-50.The function returns
Result<B256, anyhow::Error>, and the immediately following operation (line 54) correctly uses.map_err(...)?for error propagation. Thefrom_slice()calls on lines 48-50 should follow the same pattern. Usingunwrap()on potentially invalid input will panic instead of returning a recoverable error, which is problematic in a ZK proof verification context.Proposed fix
- let blob = KzgRsBlob::from_slice(&blob_info.blob_data).unwrap(); - let commitment = Bytes48::from_slice(&blob_info.commitment).unwrap(); - let proof = Bytes48::from_slice(&blob_info.proof).unwrap(); + let blob = KzgRsBlob::from_slice(&blob_info.blob_data) + .map_err(|e| anyhow!("invalid blob data: {e:?}"))?; + let commitment = Bytes48::from_slice(&blob_info.commitment) + .map_err(|e| anyhow!("invalid commitment: {e:?}"))?; + let proof = Bytes48::from_slice(&blob_info.proof) + .map_err(|e| anyhow!("invalid proof: {e:?}"))?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prover/crates/executor/client/src/verifier/blob_verifier.rs` around lines 48 - 50, Replace the three unwrap() calls so malformed input yields a propagated error instead of panicking: call KzgRsBlob::from_slice(&blob_info.blob_data).map_err(|e| anyhow::anyhow!("invalid blob_data: {}", e))? and do the same pattern for Bytes48::from_slice(&blob_info.commitment) and Bytes48::from_slice(&blob_info.proof), using descriptive messages (e.g. "invalid commitment" / "invalid proof") and the ? operator to return Result<B256, anyhow::Error>, consistent with the existing .map_err(...)? style used later in this function.
🧹 Nitpick comments (1)
gas-oracle/app/src/da_scalar/blob.rs (1)
202-206: ⚡ Quick winAdd at least one true multi-blob decode test.
These assertions still exercise
num_blobs = 1, so they won't catch boundary mistakes in concatenation/trimming across blob joins—the core behavior added by this PR. Please add a fixture or generated case whose compressed payload spans at least two blobs.Also applies to: 250-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gas-oracle/app/src/da_scalar/blob.rs` around lines 202 - 206, Add a true multi-blob decode unit test that constructs at least two Blob instances whose concatenated payload contains a single compressed batch split across the blob boundary (so the compressed bytes start in blob A and continue in blob B), then call Blob::get_payload_bytes on each blob and run Blob::detect_zstd_compressed and the downstream decompression path (decompress_batch or whatever consumer is used) to assert the full batch decodes correctly; target the same test module that currently uses get_payload_bytes and Blob::detect_zstd_compressed, create fixtures or generate a zstd-compressed payload long enough to span blobs, split it across two Blob objects, and assert the decoded records match the original uncompressed data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/batch/batch_cache_genesis_header_test.go`:
- Around line 127-141: The test blocks on waiting for an OS interrupt and leaves
the background go testLoop running; make it self-terminating by replacing the
manual interrupt with a cancellable/timeout approach: either call
cache.AssembleCurrentBatchHeader() synchronously once (removing go testLoop and
the interrupt channel), or run go testLoop with a context.WithTimeout/WithCancel
and call cancel after the timeout (or use time.After) so the loop exits and the
test continues; update references to batchCacheSyncMu, go testLoop and
AssembleCurrentBatchHeader to ensure the lock is held around the single call or
is used inside the timed loop and remove the blocking <-interrupt.
In `@common/batch/commit_test.go`:
- Around line 26-29: The test currently defines pk = "" which causes a panic
when code slices pk[2:] later; update the test to either supply a valid
non-empty test key or guard the test with an explicit skip when the key is
unset: check the package-level variable pk before any slicing (the code that
uses pk[2:] or functions that derive from pk) and call t.Skipf with a clear
message if pk == "" so the test won't panic, or replace the empty pk with a real
test key value sourced from a test config/env variable used by the test helpers.
In `@common/blob/fee.go`:
- Around line 241-248: The loop in BlobHashes drives iteration by blobs but the
output slice and inputs are commitments, causing panics or zeroed tails; update
BlobHashes to iterate over commitments (e.g., for i := range commitments) and
compute h[i] = kzg4844.CalcBlobHashV1(hasher, &commitments[i]) so the loop index
aligns with the sized result slice and the commitment used to compute each hash.
- Around line 251-261: MakeBlobProof currently indexes commitment[i] without
validating that commitment and blobs have matching lengths, which can panic when
commitment is shorter; add a preflight check at the start of MakeBlobProof that
validates len(commitment) == len(blobs) (or at least len(commitment) >=
len(blobs)) and return a clear error if not, before entering the loop that calls
kzg4844.ComputeBlobProof; this prevents panics and surfaces a normal error to
the caller.
In `@common/blob/payload.go`:
- Around line 43-51: In RetrieveBlobBytes, the validation error prints
data[i*32] (the zeroed output buffer) so it always reports 0x00; change the
logged byte to the source blob byte (use blob[i*32]) in the fmt.Errorf call so
the error shows the actual offending byte value; keep the rest of the loop and
the copy from blob to data intact and ensure you reference the same i and
indexing used in the copy when building the error message.
In `@contracts/contracts/l1/rollup/Rollup.sol`:
- Around line 921-942: In _computeBlobVersionedHash, for the legacy branch (when
_version < 2) add a guard to reject extraneous blobs by requiring blobhash(1) ==
bytes32(0) before accepting the batch; update the else branch that computes
_blobVersionedHash (which currently uses blobhash(0) and ZERO_VERSIONED_HASH) to
first require blobhash(1) == bytes32(0) so any non-zero extra blob causes a
revert, referencing the existing blobhash and ZERO_VERSIONED_HASH symbols.
In `@gas-oracle/app/src/da_scalar/l1_scalar.rs`:
- Around line 301-303: The function currently returns Ok((0,0,0)) when
indexed_hashes.is_empty(), which later hits the l2_data_len == 0 =>
MAX_BLOB_SCALAR branch in calculate_from_rollup and forces a max blob scalar;
instead return a sentinel that means “no blobs, skip blob-scalar update” (e.g.
change the return type to Result<Option<(u64,u64,u64)>, _> or return a distinct
enum variant) from the blob-processing function where indexed_hashes and tx_hash
are available, and update calculate_from_rollup to detect that sentinel and skip
updating the blob scalar rather than treating it as zero-length calldata; ensure
callers of the blob function (names: indexed_hashes, calculate_from_rollup) are
updated to handle the Option/enum and preserve existing behavior for real
zero-length calldata vs blobless batches.
In `@node/derivation/batch_info.go`:
- Around line 150-155: The code reads the 2-byte block count prefix without
checking length, causing a panic when batch.BlockContexts is non-nil but shorter
than 2; before calling binary.BigEndian.Uint16(batch.BlockContexts[:2]) (and
before computing blockCount), add a guard that len(batch.BlockContexts) >= 2 and
return a clear error if not; keep the subsequent existing length check
(len(batch.BlockContexts) < 2+60*blockCount) intact and reference
batch.BlockContexts and blockCount when updating the error messages.
In `@prover/crates/executor/client/src/lib.rs`:
- Line 13: BlobVerifier::verify_blobs currently returns versioned_hashes and
batch_data_from_blob but the caller blindly indexes versioned_hashes[0] for
V0/V1 and treats any >=2 as V2; update the caller to explicitly validate the
blob version and the exact number of hashes before computing the public-input
(PI) hash: check the version value in versioned_hashes (only accept the explicit
supported variants, e.g., 0,1,2), assert that the hash cardinality matches the
expected count for that version (e.g., V0/V1 => exactly 1 hash, V2 => expected 2
hashes), and return an Err when the version is unknown or the count mismatches
instead of proceeding to compute the PI hash (affects the code that uses
versioned_hashes and batch_data_from_blob, including the logic around lines
37-41).
In `@prover/crates/executor/client/src/types/blob.rs`:
- Around line 17-29: The function decode_blob_scalars assumes blob_data has
BLOB_WIDTH*32 bytes and will panic on short inputs; add an explicit length check
at the start of decode_blob_scalars to validate blob_data.len() is at least (or
exactly) BLOB_WIDTH * 32 and return an Err (anyhow::Error) if not, so you avoid
indexing panics and clarify behavior for longer inputs (either reject lengths !=
BLOB_WIDTH*32 or document/handle > by only processing the first BLOB_WIDTH*32
bytes); reference the function name decode_blob_scalars and the constant
BLOB_WIDTH to locate where to add this guard.
In `@prover/crates/executor/client/src/types/input.rs`:
- Around line 72-74: The batch_version field currently accepts any u8; add
validation at the input boundary to reject unsupported values (allow only 0, 1,
2) by returning an error on deserialization or conversion. Implement this by
either adding a custom Deserialize for the input struct in types/input.rs or
implementing TryFrom<RawInput> (or a validate() method invoked immediately after
deserialization) that checks the batch_version field and returns an Err for any
value not in {0,1,2}; ensure the error propagates back to the caller so
malformed inputs are rejected before any execution or hash-version branching
occurs.
---
Outside diff comments:
In `@common/batch/batch_cache_test.go`:
- Around line 28-59: TestBatchCacheInitServer currently blocks waiting for
os.Interrupt and can hang CI; change it to be bounded or skipped: either (A)
skip in normal runs by adding at the top if testing.Short() {
t.Skip("manual/integration test") } or (B) enforce a timeout by replacing the
blocking interrupt wait with a context.WithTimeout(ctx, <duration>) and select
on ctx.Done() instead of <-interrupt, and ensure you stop the signal handler by
calling signal.Stop(interrupt) and closing the channel when the test exits.
Update references to TestBatchCacheInitServer, interrupt,
InitAndSyncFromDatabase, and AssembleCurrentBatchHeader accordingly so the
goroutines exit when the test context cancels.
In `@node/derivation/batch_info.go`:
- Around line 127-145: Before computing blockCount, validate that
batch.LastBlockNumber is not less than the reference block to avoid unsigned
underflow: in the parentVersion == 0 branch, after decoding startBlock
(BlockContext.Decode on batchBytes[:60]) check that batch.LastBlockNumber >=
startBlock.Number and return a clear error if not; in the else branch, after
obtaining parentBatchBlock from parentBatchHeader.LastBlockNumber(), check that
batch.LastBlockNumber >= parentBatchBlock and return an error if not. Keep the
existing blockCount formulas (batch.LastBlockNumber - startBlock.Number + 1 and
batch.LastBlockNumber - parentBatchBlock) but only perform them after these
validations.
In `@ops/docker/docker-compose-4nodes.yml`:
- Around line 485-497: The compose file enables batch-V2 only for tx-submitter-0
by hardcoding TX_SUBMITTER_BATCH_V2_UPGRADE_TIME and TX_SUBMITTER_SEAL_BATCH
while other submitters (multi-submitter) lack them; change the submitter service
environment blocks so all submitter instances use the same batch cutover vars
(reference TX_SUBMITTER_BATCH_V2_UPGRADE_TIME and TX_SUBMITTER_SEAL_BATCH)
and/or point them to the shared ${BATCH_UPGRADE_TIME} variable used by the
nodes; update the tx-submitter-0 and multi-submitter service env lists so they
consistently include TX_SUBMITTER_BATCH_V2_UPGRADE_TIME (or
${BATCH_UPGRADE_TIME}) and TX_SUBMITTER_SEAL_BATCH to ensure all submitters flip
to batch-V2 together.
In `@prover/crates/executor/client/src/verifier/blob_verifier.rs`:
- Around line 48-50: Replace the three unwrap() calls so malformed input yields
a propagated error instead of panicking: call
KzgRsBlob::from_slice(&blob_info.blob_data).map_err(|e| anyhow::anyhow!("invalid
blob_data: {}", e))? and do the same pattern for
Bytes48::from_slice(&blob_info.commitment) and
Bytes48::from_slice(&blob_info.proof), using descriptive messages (e.g. "invalid
commitment" / "invalid proof") and the ? operator to return Result<B256,
anyhow::Error>, consistent with the existing .map_err(...)? style used later in
this function.
In `@tx-submitter/services/rollup.go`:
- Around line 1260-1261: Replace the truncating Int64 conversions for gas caps
by passing the original *big.Int values into uint256.MustFromBig: locate the
struct/assignment where GasTipCap and GasFeeCap are set (the lines using
GasTipCap, GasFeeCap, uint256.MustFromBig, and the tip and gasFeeCap variables)
and change the arguments from big.NewInt(tip.Int64()) and
big.NewInt(gasFeeCap.Int64()) to pass tip and gasFeeCap directly to
uint256.MustFromBig so no data is silently truncated.
---
Nitpick comments:
In `@gas-oracle/app/src/da_scalar/blob.rs`:
- Around line 202-206: Add a true multi-blob decode unit test that constructs at
least two Blob instances whose concatenated payload contains a single compressed
batch split across the blob boundary (so the compressed bytes start in blob A
and continue in blob B), then call Blob::get_payload_bytes on each blob and run
Blob::detect_zstd_compressed and the downstream decompression path
(decompress_batch or whatever consumer is used) to assert the full batch decodes
correctly; target the same test module that currently uses get_payload_bytes and
Blob::detect_zstd_compressed, create fixtures or generate a zstd-compressed
payload long enough to span blobs, split it across two Blob objects, and assert
the decoded records match the original uncompressed data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4edcdb64-4766-4507-8797-0cbdffcef1ad
⛔ Files ignored due to path filters (4)
common/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (58)
common/batch/batch_cache.gocommon/batch/batch_cache_genesis_header_test.gocommon/batch/batch_cache_test.gocommon/batch/batch_data.gocommon/batch/batch_header.gocommon/batch/batch_header_test.gocommon/batch/batch_query.gocommon/batch/batch_restart_test.gocommon/batch/batch_storage.gocommon/batch/blob.gocommon/batch/commit_test.gocommon/batch/encoding.gocommon/batch/helpers_test.gocommon/batch/interfaces.gocommon/batch/l2_gov.gocommon/blob/fee.gocommon/blob/payload.gocommon/go.modcontracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.solgas-oracle/app/src/da_scalar/blob.rsgas-oracle/app/src/da_scalar/calculate.rsgas-oracle/app/src/da_scalar/l1_scalar.rsgo-ethereumnode/derivation/batch_info.gonode/derivation/batch_info_test.gonode/derivation/beacon.gonode/derivation/derivation.gonode/types/batch_header.goops/docker/docker-compose-4nodes.ymlops/docker/layer1/configs/values.env.templateops/l2-genesis/deploy-config/devnet-deploy-config.jsonoracle/oracle/batch.goprover/bin/challenge/src/handler.rsprover/bin/host/src/execute.rsprover/bin/host/src/main.rsprover/bin/server/src/queue.rsprover/bin/shadow-prove/src/execute.rsprover/bin/shadow-prove/src/main.rsprover/bin/shadow-prove/src/shadow_rollup.rsprover/crates/executor/client/src/lib.rsprover/crates/executor/client/src/types/batch.rsprover/crates/executor/client/src/types/blob.rsprover/crates/executor/client/src/types/input.rsprover/crates/executor/client/src/verifier/blob_verifier.rsprover/crates/executor/host/src/blob.rstx-submitter/batch/batch_storage_test.gotx-submitter/batch/blob.gotx-submitter/flags/flags.gotx-submitter/go.modtx-submitter/iface/client.gotx-submitter/services/rollup.gotx-submitter/types/blob.gotx-submitter/types/blob_compat.gotx-submitter/types/blob_params.gotx-submitter/types/converter.gotx-submitter/types/l2Caller.gotx-submitter/utils/config.go
💤 Files with no reviewable changes (5)
- tx-submitter/batch/batch_storage_test.go
- tx-submitter/go.mod
- tx-submitter/types/blob_params.go
- tx-submitter/types/blob.go
- tx-submitter/batch/blob.go
Comment on lines
301
to
+303
| if indexed_hashes.is_empty() { | ||
| log::info!("no blob in this batch, batch_tx_hash: {:#?}", tx_hash); | ||
| return Ok((0, 0)); | ||
| return Ok((0, 0, 0)); |
Contributor
There was a problem hiding this comment.
Skip blob-scalar recalculation for blobless batches.
Returning (0, 0, 0) here falls through to the existing l2_data_len == 0 => MAX_BLOB_SCALAR path in calculate_from_rollup, so a calldata-only / blobless commit batch can push the oracle to the max blob scalar on the next update. Please propagate a sentinel for “no blobs” and skip the blob-scalar update in that case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gas-oracle/app/src/da_scalar/l1_scalar.rs` around lines 301 - 303, The
function currently returns Ok((0,0,0)) when indexed_hashes.is_empty(), which
later hits the l2_data_len == 0 => MAX_BLOB_SCALAR branch in
calculate_from_rollup and forces a max blob scalar; instead return a sentinel
that means “no blobs, skip blob-scalar update” (e.g. change the return type to
Result<Option<(u64,u64,u64)>, _> or return a distinct enum variant) from the
blob-processing function where indexed_hashes and tx_hash are available, and
update calculate_from_rollup to detect that sentinel and skip updating the blob
scalar rather than treating it as zero-length calldata; ensure callers of the
blob function (names: indexed_hashes, calculate_from_rollup) are updated to
handle the Option/enum and preserve existing behavior for real zero-length
calldata vs blobless batches.
| pub fn verify(input: ExecutorInput) -> Result<B256, anyhow::Error> { | ||
| // Verify DA | ||
| let (versioned_hash, batch_data_from_blob) = BlobVerifier::verify(&input.blob_info)?; | ||
| let (versioned_hashes, batch_data_from_blob) = BlobVerifier::verify_blobs(&input.blob_infos)?; |
Contributor
There was a problem hiding this comment.
Validate batch version and blob-hash count before computing the PI hash.
The current dispatch fails open: V0/V1 blindly index versioned_hashes[0] and ignore any additional hashes, while >= 2 treats unknown future versions as V2. That can panic on malformed input or compute a public-input hash that does not match the DA commitment. Return an error unless the version is explicitly supported and the blob-hash cardinality matches it.
Suggested fix
- let public_input_hash = if input.batch_version >= 2 {
- batch_info.public_input_hash_v2(&versioned_hashes)
- } else {
- batch_info.public_input_hash(&versioned_hashes[0])
- };
+ let public_input_hash = match (input.batch_version, versioned_hashes.as_slice()) {
+ (0 | 1, [hash]) => batch_info.public_input_hash(hash),
+ (2, hashes) if !hashes.is_empty() => batch_info.public_input_hash_v2(hashes),
+ (0 | 1, hashes) => {
+ return Err(anyhow::anyhow!(
+ "batch version {} requires exactly 1 blob hash, got {}",
+ input.batch_version,
+ hashes.len()
+ ));
+ }
+ (2, _) => return Err(anyhow::anyhow!("batch version 2 requires at least 1 blob hash")),
+ (v, _) => return Err(anyhow::anyhow!("unsupported batch version: {v}")),
+ };Also applies to: 37-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@prover/crates/executor/client/src/lib.rs` at line 13,
BlobVerifier::verify_blobs currently returns versioned_hashes and
batch_data_from_blob but the caller blindly indexes versioned_hashes[0] for
V0/V1 and treats any >=2 as V2; update the caller to explicitly validate the
blob version and the exact number of hashes before computing the public-input
(PI) hash: check the version value in versioned_hashes (only accept the explicit
supported variants, e.g., 0,1,2), assert that the hash cardinality matches the
expected count for that version (e.g., V0/V1 => exactly 1 hash, V2 => expected 2
hashes), and return an Err when the version is unknown or the count mismatches
instead of proceeding to compute the PI hash (affects the code that uses
versioned_hashes and batch_data_from_blob, including the logic around lines
37-41).
Comment on lines
+17
to
+29
| pub fn decode_blob_scalars(blob_data: &[u8]) -> Result<Vec<u8>, anyhow::Error> { | ||
| let mut chunk = vec![0u8; BLOB_WIDTH * 31]; | ||
| for i in 0..BLOB_WIDTH { | ||
| if blob_data[i * 32] != 0 { | ||
| return Err(anyhow!(format!( | ||
| return Err(anyhow!( | ||
| "Invalid blob, found non-zero high order byte {:x} of field element {}", | ||
| blob_data[i * 32], | ||
| i | ||
| ))); | ||
| )); | ||
| } | ||
| batch_data[i * 31..i * 31 + 31].copy_from_slice(&blob_data[i * 32 + 1..i * 32 + 32]); | ||
| chunk[i * 31..i * 31 + 31].copy_from_slice(&blob_data[i * 32 + 1..i * 32 + 32]); | ||
| } | ||
| decompress_batch(&batch_data) | ||
| Ok(chunk) |
Contributor
There was a problem hiding this comment.
Validate the blob length before indexing into it.
Line 17 assumes blob_data is exactly BLOB_WIDTH * 32 bytes. Short inputs will panic, and longer inputs are silently ignored past the first blob-width window.
🛡️ Suggested guard
pub fn decode_blob_scalars(blob_data: &[u8]) -> Result<Vec<u8>, anyhow::Error> {
+ let expected_len = BLOB_WIDTH * 32;
+ if blob_data.len() != expected_len {
+ return Err(anyhow!(
+ "invalid blob length: got {}, want {}",
+ blob_data.len(),
+ expected_len
+ ));
+ }
+
let mut chunk = vec![0u8; BLOB_WIDTH * 31];
for i in 0..BLOB_WIDTH {
if blob_data[i * 32] != 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn decode_blob_scalars(blob_data: &[u8]) -> Result<Vec<u8>, anyhow::Error> { | |
| let mut chunk = vec![0u8; BLOB_WIDTH * 31]; | |
| for i in 0..BLOB_WIDTH { | |
| if blob_data[i * 32] != 0 { | |
| return Err(anyhow!(format!( | |
| return Err(anyhow!( | |
| "Invalid blob, found non-zero high order byte {:x} of field element {}", | |
| blob_data[i * 32], | |
| i | |
| ))); | |
| )); | |
| } | |
| batch_data[i * 31..i * 31 + 31].copy_from_slice(&blob_data[i * 32 + 1..i * 32 + 32]); | |
| chunk[i * 31..i * 31 + 31].copy_from_slice(&blob_data[i * 32 + 1..i * 32 + 32]); | |
| } | |
| decompress_batch(&batch_data) | |
| Ok(chunk) | |
| pub fn decode_blob_scalars(blob_data: &[u8]) -> Result<Vec<u8>, anyhow::Error> { | |
| let expected_len = BLOB_WIDTH * 32; | |
| if blob_data.len() != expected_len { | |
| return Err(anyhow!( | |
| "invalid blob length: got {}, want {}", | |
| blob_data.len(), | |
| expected_len | |
| )); | |
| } | |
| let mut chunk = vec![0u8; BLOB_WIDTH * 31]; | |
| for i in 0..BLOB_WIDTH { | |
| if blob_data[i * 32] != 0 { | |
| return Err(anyhow!( | |
| "Invalid blob, found non-zero high order byte {:x} of field element {}", | |
| blob_data[i * 32], | |
| i | |
| )); | |
| } | |
| chunk[i * 31..i * 31 + 31].copy_from_slice(&blob_data[i * 32 + 1..i * 32 + 32]); | |
| } | |
| Ok(chunk) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@prover/crates/executor/client/src/types/blob.rs` around lines 17 - 29, The
function decode_blob_scalars assumes blob_data has BLOB_WIDTH*32 bytes and will
panic on short inputs; add an explicit length check at the start of
decode_blob_scalars to validate blob_data.len() is at least (or exactly)
BLOB_WIDTH * 32 and return an Err (anyhow::Error) if not, so you avoid indexing
panics and clarify behavior for longer inputs (either reject lengths !=
BLOB_WIDTH*32 or document/handle > by only processing the first BLOB_WIDTH*32
bytes); reference the function name decode_blob_scalars and the constant
BLOB_WIDTH to locate where to add this guard.
Comment on lines
+72
to
+74
| pub blob_infos: Vec<BlobInfo>, | ||
| #[serde(default)] | ||
| pub batch_version: u8, |
Contributor
There was a problem hiding this comment.
Reject unsupported batch_version values at input boundary.
Line 74 accepts any u8; malformed values can pass deserialization and drive invalid execution/hash-version branching. Please validate allowed versions (e.g., 0/1/2) before processing.
Suggested fix pattern
pub struct ExecutorInput {
pub block_inputs: Vec<BlockInput>,
pub blob_infos: Vec<BlobInfo>,
#[serde(default)]
pub batch_version: u8,
}
+
+impl ExecutorInput {
+ pub fn validate(&self) -> Result<(), ClientError> {
+ match self.batch_version {
+ 0 | 1 | 2 => Ok(()),
+ v => Err(ClientError::InvalidBatchVersion(v)),
+ }
+ }
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@prover/crates/executor/client/src/types/input.rs` around lines 72 - 74, The
batch_version field currently accepts any u8; add validation at the input
boundary to reject unsupported values (allow only 0, 1, 2) by returning an error
on deserialization or conversion. Implement this by either adding a custom
Deserialize for the input struct in types/input.rs or implementing
TryFrom<RawInput> (or a validate() method invoked immediately after
deserialization) that checks the batch_version field and returns an Err for any
value not in {0,1,2}; ensure the error propagates back to the caller so
malformed inputs are rejected before any execution or hash-version branching
occurs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t prefix ABI-decoded `bytes` can be a non-nil zero/short slice; reading batch.BlockContexts[:2] without a length check would panic on crafted/empty calldata. Add an explicit len < 2 guard before the BigEndian.Uint16 read so derivation returns an error instead of panicking. Co-authored-by: Cursor <cursoragent@cursor.com>
batch.LastBlockNumber and the reference block (decoded startBlock for the V0->V1+ transition or parent header's LastBlockNumber for V1+) are not constrained by the L1 Rollup contract, so a malformed batch with LastBlockNumber below the reference would underflow blockCount and lead to a makeslice panic downstream in the blob-only path. Reject such batches with a clear error before subtracting. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/derivation/batch_info.go`:
- Around line 167-172: The calldata-derived blockCount is overwriting the
header-derived count for batch.Version > 0; update the logic in the block that
reads batch.BlockContexts so that you first compute the calldataPrefixCount
(from binary.BigEndian.Uint16(batch.BlockContexts[:2])), then if
bi.lastBlockNumber / batch logic already set an expectedCount compare
expectedCount to calldataPrefixCount and return an error if they differ,
otherwise only assign blockCount = calldataPrefixCount when no prior expected
count exists; keep the existing length check against calldataPrefixCount and
ensure bi.lastBlockNumber (or LastBlockNumber) continues to come from the
batch/header-derived value unless the prefix matches.
- Around line 127-143: The V0→V1 transition block currently reads a BlockContext
from batchBytes (using BlockContext.Decode(batchBytes[:60])) but in calldata
mode batchBytes holds tx payload and is not a block context; instead detect
calldata mode (e.g. when batch.BlockContexts or rawBlockContexts will be used)
and do not read batchBytes there—either defer the V0→V1 block-count computation
until after rawBlockContexts is selected or, when calldata is present, derive
the start block from batch.BlockContexts[0].Number (or the selected
rawBlockContexts[0]) and then compute blockCount = batch.LastBlockNumber -
startBlock.Number + 1; update the logic around parentVersion,
BlockContext.Decode, and uses of batchBytes to avoid decoding payload as a
BlockContext.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0fb1911-4e04-4b34-af6d-ae3eeba9e254
📒 Files selected for processing (1)
node/derivation/batch_info.go
Comment on lines
127
to
143
| if parentVersion == 0 { | ||
| blobData, err := types.RetrieveBlobBytes(&batch.Sidecar.Blobs[0]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| batchBytes, err := zstd.DecompressBatchBytes(blobData) | ||
| if err != nil { | ||
| return fmt.Errorf("decompress batch bytes error:%v", err) | ||
| // V0 -> V1+ transition: parent header carries no LastBlockNumber, | ||
| // so derive blockCount from the first block context embedded at | ||
| // the start of the decompressed batch. | ||
| if len(batchBytes) < 60 { | ||
| return fmt.Errorf("decompressed batch too short for start block context: have %d, need 60", len(batchBytes)) | ||
| } | ||
| var startBlock BlockContext | ||
| if err := startBlock.Decode(batchBytes[:60]); err != nil { | ||
| return fmt.Errorf("decode chunk block context error:%v", err) | ||
| } | ||
| // Guard against uint64 underflow for malformed batches whose | ||
| // LastBlockNumber is below the decoded start block. | ||
| if batch.LastBlockNumber < startBlock.Number { | ||
| return fmt.Errorf("invalid batch: lastBlockNumber %d < start block %d", batch.LastBlockNumber, startBlock.Number) | ||
| } | ||
| blockCount = batch.LastBlockNumber - startBlock.Number + 1 |
Contributor
There was a problem hiding this comment.
Don't read the V0→V1 transition block context from batchBytes when calldata is present.
Lines 160-172 redefine batchBytes as tx payload in calldata mode, so batchBytes[:60] here is no longer a block context. A valid first V1/V2 batch with empty or short tx payload will fail before the calldata block-count prefix is even consulted. Derive the transition count from batch.BlockContexts in this mode, or defer the V0→V1 calculation until after rawBlockContexts has been selected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/derivation/batch_info.go` around lines 127 - 143, The V0→V1 transition
block currently reads a BlockContext from batchBytes (using
BlockContext.Decode(batchBytes[:60])) but in calldata mode batchBytes holds tx
payload and is not a block context; instead detect calldata mode (e.g. when
batch.BlockContexts or rawBlockContexts will be used) and do not read batchBytes
there—either defer the V0→V1 block-count computation until after
rawBlockContexts is selected or, when calldata is present, derive the start
block from batch.BlockContexts[0].Number (or the selected rawBlockContexts[0])
and then compute blockCount = batch.LastBlockNumber - startBlock.Number + 1;
update the logic around parentVersion, BlockContext.Decode, and uses of
batchBytes to avoid decoding payload as a BlockContext.
Contributor
There was a problem hiding this comment.
Analyzed; not fixing in this PR. Reasoning below.
After reverting aae2213 (the speculative underflow guards), the residual behavior on this path is:
- In calldata mode,
BlockContext.Decode(batchBytes[:60])does run on tx-payload bytes and produces a garbagestartBlock.Number. The resultingblockCount = batch.LastBlockNumber - startBlock.Number + 1is also garbage but is unconditionally overwritten by the calldata-prefix value a few lines below, so it has no downstream effect. - The only observable artifact is the pre-existing
len(batchBytes) < 60guard: it could reject a V0->V1+ transition batch submitted via the legacy calldata ABI whose decompressed tx payload happens to be shorter than 60 bytes. That intersection requires (a) the historical V0->V1 transition batch having been committed through the legacy ABI, and (b) that single batch containing only L1 messages with no L2 tx bytes -- a one-shot historical event, and it returns a clean error rather than panicking.
Restructuring the V0->V1+ start-block decode to be deferred until after calldata/blob selection would address it cleanly, but is purely defensive against an edge case that has not been observed in production. Per a "must have demonstrable benefit" policy, leaving as-is; happy to revisit if a concrete failure surfaces.
Contributor
There was a problem hiding this comment.
@curryxbo, understood — the analysis is sound. The critical point is that blockCount is unconditionally overwritten in calldata mode, so the garbage decoded value never reaches downstream logic. The only residual risk — the len(batchBytes) < 60 guard rejecting a V0→V1+ calldata batch whose entire decompressed payload is shorter than 60 bytes — is bounded to a single historical transition event and produces a clean error rather than silent data corruption. Deferring the fix until a concrete failure surfaces is reasonable under that policy.
(°ヘ°) …noted
✏️ Learnings added
Learnt from: curryxbo
Repo: morph-l2/morph PR: 935
File: node/derivation/batch_info.go:127-138
Timestamp: 2026-05-11T08:28:23.195Z
Learning: In `node/derivation/batch_info.go` (`ParseBatch`), during the V0→V1+ transition path (`parentVersion == 0`), `BlockContext.Decode(batchBytes[:60])` is called even in calldata mode where `batchBytes` holds tx-payload bytes, producing a garbage `startBlock.Number`. However, the resulting `blockCount` is unconditionally overwritten by the calldata block-count prefix a few lines below, so there is no downstream effect. The only real risk is the `len(batchBytes) < 60` guard falsely rejecting a historical V0→V1+ transition batch submitted via calldata ABI whose entire decompressed tx payload is shorter than 60 bytes — a one-shot historical edge case. Per the project's "must have demonstrable benefit" policy, this was intentionally left unfixed in PR `#935`; revisit only if a concrete production failure surfaces.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
167
to
+172
| blockCount = uint64(binary.BigEndian.Uint16(batch.BlockContexts[:2])) | ||
| if uint64(len(batch.BlockContexts)) < 2+60*blockCount { | ||
| return fmt.Errorf("calldata block contexts too short: have %d, need %d", len(batch.BlockContexts), 2+60*blockCount) | ||
| } | ||
| rawBlockContexts = batch.BlockContexts[2 : 60*blockCount+2] | ||
| txsData = batchBytes |
Contributor
There was a problem hiding this comment.
Keep the calldata block-count prefix consistent with the header-derived range.
For batch.Version > 0, blockCount was already inferred from batch.LastBlockNumber and the parent header above, but this branch overwrites it unconditionally. That means an inconsistent header/context pair can parse successfully and bi.lastBlockNumber ends up coming from calldata instead of the batch metadata. Please compare the calldata prefix against the previously derived count before assigning it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/derivation/batch_info.go` around lines 167 - 172, The calldata-derived
blockCount is overwriting the header-derived count for batch.Version > 0; update
the logic in the block that reads batch.BlockContexts so that you first compute
the calldataPrefixCount (from binary.BigEndian.Uint16(batch.BlockContexts[:2])),
then if bi.lastBlockNumber / batch logic already set an expectedCount compare
expectedCount to calldataPrefixCount and return an error if they differ,
otherwise only assign blockCount = calldataPrefixCount when no prior expected
count exists; keep the existing length check against calldataPrefixCount and
ensure bi.lastBlockNumber (or LastBlockNumber) continues to come from the
batch/header-derived value unless the prefix matches.
Contributor
There was a problem hiding this comment.
Analyzed; not fixing in this PR. Reasoning below.
The observation is correct: for batch.Version > 0 in calldata mode, blockCount is first derived from batch.LastBlockNumber - parentBatchBlock (or the V0->V1+ variant) and then unconditionally overwritten by BigEndian.Uint16(batch.BlockContexts[:2]), with no consistency check. Reaching a divergent state requires the submitter producing a batch whose header metadata and calldata block-context count disagree; the L1 Rollup contract does not enforce this invariant.
This is the same threat model as the speculative LastBlockNumber <= parentBatchBlock guard we just reverted on a "no demonstrable benefit" criterion (trusted-sequencer model, no observed occurrence). To stay consistent with that policy, leaving this check out for now. If derivation later gets a coordinated pass to harden against malicious-sequencer input as a class, this consistency check is a reasonable addition alongside the underflow guards.
Contributor
There was a problem hiding this comment.
@curryxbo, understood — the reasoning is consistent and well-documented. Trusting the sequencer to produce coherent header/calldata pairs is a reasonable stance for the current security model, and keeping derivation hardening guards together in a coordinated pass makes the trade-off explicit rather than scattering ad-hoc checks across PRs.
Would you like me to open a follow-up GitHub issue to track the calldata block-count consistency check (and the LastBlockNumber <= parentBatchBlock underflow guard) for a future derivation hardening pass?
✏️ Learnings added
Learnt from: curryxbo
Repo: morph-l2/morph PR: 935
File: node/derivation/batch_info.go:157-162
Timestamp: 2026-05-11T08:28:19.645Z
Learning: In `node/derivation/batch_info.go`, the project follows a trusted-sequencer model for derivation. Speculative defensive guards (e.g. calldata block-count prefix vs header-derived blockCount consistency check, LastBlockNumber underflow guards) are intentionally deferred to a future coordinated derivation-hardening pass rather than added ad-hoc. Do not raise these as blocking issues in the current multi-blob / feat/multi_batch PR context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…med batches" This reverts commit aae2213.
Point the go-ethereum submodule at 045be0fdc7ca (morph-v2.2.2) to match origin/main and the locked go.mod version. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a protocol-level multi-blob (Batch V2) upgrade spanning 73 files across the Rollup contract, derivation, prover public-input hashing, gas-oracle DA scalar, and tx-submitter sealing/commit paths — it should get a human review.
Extended reasoning...
Overview
This PR introduces Batch Header V2 and multi-blob support end-to-end:
- L1 contract (
contracts/contracts/l1/rollup/Rollup.sol): acceptsversion <= 2, adds_computeBlobVersionedHashwhich aggregatesblobhash(0..N-1)viakeccak256for V2, and routes V2 headers through the V1 257-byte loader. The 32-byte field at offset 57 now carries an aggregated hash for V2 (versioned hash for V0/V1), and_verifyProofconsumes that field uniformly. - Prover (
prover/crates/executor/client,prover/bin/{server,shadow-prove,challenge}):ExecutorInputgainsblob_infos: Vec<BlobInfo>andbatch_version: u8;BlobVerifier::verify_blobsconcatenates raw blob scalars and decompresses once;BatchInfo::public_input_hash_v2hasheskeccak256(h0||...||hN-1)into the public input. The proverServer's blob input fed intobatch_header.dataflips between aggregated and single hash based onbatch_version. - Node derivation (
node/derivation):ParseBatchnow concatenates all blob bodies before a single zstd decompress (per-blob decompression would fail mid-frame), enforces L1-declared blob order via a versioned-hash map, and replacesVerifyBlobProofwith a commitment round-trip (verifyBlob) since beacon/blob_sidecarsno longer guarantees a legacy single-blob KZG proof post-Osaka/PeerDAS. - Common module (
common/batch,common/blob): new Go module hostingBatchCache,BatchStorage, V2 header codec (BlobHashesHashvsBlobVersionedHash), and multi-blob sidecar/compression helpers; tx-submitter and node now depend on it viacommon/blobcompat shims. - Tx-submitter: new flags
MAX_BLOB_COUNTandBATCH_V2_UPGRADE_TIMEgate V2 activation;EstimateGasnow passesBlobHashes/BlobGasFeeCapsoblobhash(i)simulates correctly duringeth_estimateGas. - Gas-oracle: blob extraction switched to multi-blob concat-then-decompress;
get_data_from_blobreturns(batch_size, num_blobs, txn_count)and blob-scalar capacity scales withnum_blobs. - Configs/infra:
programVkeyupdated across all networks (holesky/hoodi/l1/qanetl1/sepolia/testnetl1) — verifier-side change that must match the new SP1 ELF.
Security risks
- L1 contract surface changes:
Rollup._computeBlobVersionedHashand_loadBatchHeaderaccept a new version, and the proof public-input now depends on an aggregated blob hash for V2. A mismatch between the contract's aggregation (loop overblobhash(i)until zero) and the prover's aggregation (Vec from beacon) would silently invalidate proofs or accept malformed batches. CodeRabbit flagged that the V0/V1 branch does not assertblobhash(1) == 0, allowing legacy batches to carry uncommitted extra blobs; the author elected not to add the guard. - Verifier key rotation:
programVkeychanges across all deploy configs — any deploy-script discrepancy between contract upgrade and prover ELF would brick finalization. - Derivation hardening deferred: CodeRabbit raised a calldata-prefix length check (since fixed in 2843031), a stale V0→V1
BlockContext.Decodeon tx-payload bytes, and a header-vs-calldatablockCountmismatch. The author explicitly deferred the latter two under a 'trusted sequencer' policy — defensible but worth a human sign-off on the threat model. - Removed KZG proof check:
VerifyBlobProofwas replaced with a commitment→versioned-hash round-trip. The reasoning (PeerDAS makes beacon-supplied proofs unreliable) is sound, but the security argument deserves a human review given that this is the only on-the-fly DA check at derivation time. - Blob length validation:
copy(blob[:], b)is now guarded by an exact length check after CodeRabbit raised it — good — but several other multi-blob index sites (Rustdecode_blob_scalars, GoMakeBlobProof) were flagged without explicit fixes landing.
Level of scrutiny
This is the highest-scrutiny tier: a coordinated protocol upgrade that changes consensus-relevant public-input hashing, L1 contract code, and verifier keys, with several deferred hardening items and an explicit invocation of the trusted-sequencer model in derivation. Outside the bot's auto-approval criteria — needs a human reviewer who can validate the V2 activation/coordination plan, confirm prover/contract aggregation parity, and accept the deferred derivation guards.
Other factors
- 60+ minute estimated review effort per CodeRabbit's own walkthrough.
- Multiple unresolved CodeRabbit majors (legacy-version extra-blob guard, blobless-batch scalar fallback, batch_version validation, calldata-vs-header blockCount consistency) where the author chose to defer rather than fix; a human should weigh those tradeoffs.
- Tests added are non-trivial (V2 header parsing, multi-blob ParseBatch round-trip, contract V2 aggregation tests) but do not cover the cross-boundary parity between contract aggregation and prover aggregation end-to-end.
- The bug hunting system found no bugs this run, but absence of findings here is not a substitute for human review on a change of this scope.
Contributor
|
@claude review |
Comment on lines
+27
to
33
| #[serde(default = "default_batch_version")] | ||
| pub batch_version: u8, | ||
| } | ||
|
|
||
| fn default_batch_version() -> u8 { | ||
| 2 | ||
| } |
There was a problem hiding this comment.
🔴 The server defaults batch_version to 2 in prover/bin/server/src/queue.rs:27-33, but the challenge handler's ProveRequest at prover/bin/challenge/src/handler.rs:21-27 has no batch_version field at all and POSTs JSON without it (handler.rs:184-189). For any V0/V1 batch the challenge handler asks the prover to prove, the server applies the V2 default and dispatches public_input_hash_v2 at lib.rs:37-41 plus writes keccak256(h0||...) to batch_header.data at queue.rs:167-175, producing a public-input hash that does not match the on-chain Rollup contract's V0/V1 blobhash(0)-derived PI — so L1 challenge proof verification fails. Fix by updating challenge handler ProveRequest to include batch_version (read from the batch header it already loads, mirroring shadow_prove.rs:127), or change default_batch_version() to 0 to match host/src/main.rs:36 and ExecutorInput's #[serde(default)].
Extended reasoning...
What the bug is
prover/bin/server/src/queue.rs:21-33 defines the prover server's HTTP API request schema with a #[serde(default = "default_batch_version")] on batch_version, and default_batch_version() returns 2 (the V2 multi-blob variant). Any caller that POSTs to /prove_batch without including the field will have its proof generated under V2 PI semantics.
prover/bin/challenge/src/handler.rs:21-27 defines its own ProveRequest struct that literally has no batch_version field:
#[derive(Serialize)]
pub struct ProveRequest {
pub batch_index: u64,
pub start_block: u64,
pub end_block: u64,
pub rpc: String,
}It is serialized to JSON and POSTed to /prove_batch at handler.rs:184-190. The server deserializes that JSON, sees no batch_version field, and applies the default of 2.
Why existing code does not prevent it
This PR is the one introducing batch_version. It updates shadow_prove.rs:127-134 to read batch_version from the on-chain batch header and pass it explicitly, but it does not update challenge/src/handler.rs. Three other entry points in this PR all default to 0:
prover/bin/host/src/main.rs:36—#[clap(long = "batch-version", default_value_t = 0)]prover/crates/executor/client/src/types/input.rs:73-74—#[serde(default)]onpub batch_version: u8(u8 default = 0)prover/bin/shadow-prove/src/shadow_prove.rs:24—#[serde(default)](u8 default = 0)
Only the server queue defaults to 2. So the challenge handler — the one in-tree caller that doesn't set the field — silently triggers V2 PI math on every batch it asks for proof of.
Step-by-step proof
- A challenge is filed against a V0 or V1 batch on L1 (e.g. any historical batch from before
BATCH_V2_UPGRADE_TIME). challenge-handlerdetects the event, inspects the batch (handler.rs:154-161), and constructsProveRequest { batch_index, start_block, end_block, rpc }— nobatch_versionfield at all (handler.rs:184-189).util::call_prover(serde_json::to_string(&request).unwrap(), "/prove_batch")(handler.rs:190) sends a JSON body that lacksbatch_version.- The server's
ProveRequestdeserializer inqueue.rs:21-29appliesdefault_batch_version()= 2. prove_for_queuecallsgen_client_input(..., batch_version=2)(queue.rs:90-96). Inside,execute_batchis invoked withbatch_version = 2(queue.rs:145-153), so the resultingExecutorInput.batch_version = 2.gen_client_inputthen computesblob_inputfor the savedbatch_header.data(queue.rs:163-181):For the V0/V1 batch we getlet blob_input = if batch_version >= 2 { let mut blob_hasher = Keccak256::new(); for h in &versioned_hashes { blob_hasher.update(h.as_slice()); } blob_hasher.finalize() } else { versioned_hashes[0] };
keccak256(versioned_hashes[0])instead ofversioned_hashes[0].- Inside the zkVM,
prover/crates/executor/client/src/lib.rs:37-41dispatches oninput.batch_version:So the proof commits to a public input that useslet public_input_hash = if input.batch_version >= 2 { batch_info.public_input_hash_v2(&versioned_hashes) } else { batch_info.public_input_hash(&versioned_hashes[0]) };
keccak256(versioned_hashes[0])as the blob input field. - On L1, the Rollup contract's
_verifyProof(Rollup.sol:758-772) reads offset 57 of the V0/V1 batch header, which was set at commit time by_computeBlobVersionedHash(0_or_1)(Rollup.sol:921-942) toblobhash(0)directly, notkeccak256(blobhash(0)). The contract then computes its expected public input hash usingblobhash(0)as the blob field. - Off-chain PI (
keccak256(h0)) != on-chain PI (h0) → SP1 verifier rejects the proof → challenge resolution fails on L1.
Impact
Challenge resolution is the safety-critical defense path: when an honest party challenges an invalid batch, the prover must produce a valid SP1 proof to defend the batch on L1. With this bug, the prover silently produces proofs against the wrong public input for any V0/V1 batch challenged via challenge-handler, so verification fails and the rollup loses the challenge by default. This is bounded to the V0/V1 → V2 transition window (any historical V0/V1 batches still inside the proof window when the V2 upgrade goes live), but during that window the challenge handler is broken.
How to fix
Three options, in order of robustness:
- Best: Update
challenge/src/handler.rs:21-27ProveRequestto includebatch_version: u8, and athandler.rs:184-189populate it from the parent batch header thatbatch_inspectalready loads (mirroringshadow_prove.rs:124-134). The challenge handler already parses the batch header athandler.rs:486-500, so the version byte is trivially available. - Change
default_batch_version()inqueue.rs:31-33to return0instead of2, matchinghost/src/main.rs:36andtypes/input.rs:73-74. This eliminates the silent regression but still leavesbatch_versionimplicit — a future V2-only caller might forget to set it. - Remove
#[serde(default = "default_batch_version")]entirely and make the field mandatory in the request payload. This forces every caller to be explicit, which is the cleanest contract but requires touching all callers.
Option (1) addresses the current production-breaking path; option (2) or (3) is a defense-in-depth follow-up worth landing in the same PR.
curryxbo
pushed a commit
that referenced
this pull request
May 13, 2026
Brings in 6 commits from base feat/multi_batch since the previous merge (09a2c72) so PR #951 stops reporting CONFLICTING: - 4be9cda update challenge handler dep - c6f7cc2 add v0/v1 blob check - 7ec11cf Merge branch 'feat/multi_batch' of github.com:morph-l2/morph - fa9f4e8 fix submitter replay batch with config max blob count - 1fe0d74 add rustc version desc - fbd1bee chore: align go-ethereum submodule with origin/main Single content conflict in common/batch/batch_cache.go: base added a new sealEffectiveBlobCount function in the same hunk where SPEC-005 exported BuildBlockContext (PR #935-derived rename). Resolved by keeping both: sealEffectiveBlobCount stays where base placed it; the exported BuildBlockContext (with SPEC-005 docstring) follows. base's lowercase "buildBlockContext" comment was dropped (now stale -- we renamed it). All other touched files auto-merged: contracts/Rollup.sol, BlobVersionedHashLegacy.t.sol (added by base), prover/Cargo.{toml,lock}, prover/{,bin/challenge/}rust-toolchain (deleted by base), tx-submitter/mock/rollup.go, common/batch/batch_restart_test.go. go build ./common/batch/ ./common/blob/ ./node/... ./tx-submitter/... ./oracle/... -- clean. go test ./node/derivation/... -- 22 tests PASS (incl. 9 added in b9ad14e). common/batch tests hang on a pre-existing txSenderCacher goroutine leak unrelated to this merge (reproducible on origin/feat/multi_batch in isolation); not introduced here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Configuration
Improvements
Tests