Open
Conversation
- Add _setStoredBlobHash helper and BATCH_BLOB_VERSIONED_HASHES_SLOT in L1MessageBaseTest - Preset batchBlobVersionedHashes in tests so commitBatch succeeds without blob tx - test_commitAndFinalizeWithL1Messages_succeeds: set stored blob hash for batch 1 and 2 - test_commitBatches_succeeds: set stored blob hash for batch 1 - test_revertBatch_succeeds: set stored blob hash for batch 1 and 2 - Remove duplicate ZERO_VERSIONED_HASH from RollupCommitBatchWithProofTest Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughAdds a new external commitState entrypoint and per-batch Changes
Sequence Diagram(s)sequenceDiagram
actor Staker
participant Rollup
participant Storage as batchBlobVersionedHashes
participant Verifier
Note over Staker,Rollup: Initial commit (no stored hash)
Staker->>Rollup: commitBatch(batchData, sig, blob)
Rollup->>Verifier: verify(blob/proof)
Verifier-->>Rollup: ok
Rollup->>Rollup: _commitBatchWithBatchData(..., computedBlobHash)
Rollup->>Storage: store batchBlobVersionedHashes[batchIndex] = computedBlobHash
Rollup-->>Staker: batch committed
Note over Staker,Rollup: Re-commit state using stored hash
Staker->>Rollup: commitState(batchData, sig)
Rollup->>Storage: read batchBlobVersionedHashes[nextBatchIndex]
Storage-->>Rollup: storedBlobHash
Rollup->>Verifier: verify(signatures)
Verifier-->>Rollup: ok
Rollup->>Rollup: _commitBatchWithBatchData(..., storedBlobHash)
Rollup-->>Staker: state committed
Note over Rollup,Storage: Finalization cleanup
Rollup->>Rollup: finalizeBatch()
Rollup->>Storage: delete batchBlobVersionedHashes[batchIndex - 1]
Rollup-->>Staker: finalized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![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: 1
🧹 Nitpick comments (3)
contracts/contracts/test/Rollup.t.sol (1)
763-764: Add one end-to-end test for the new stored-hash lifecycle.These fixtures manually seed
batchBlobVersionedHashes, but they never verify the production flow: initial commit populates it,revertBatch()preserves it,commitState()reuses it, andcommitBatch()still reverts when neither a stored hash nor blob is present. The new path can regress without any of these tests noticing.Also applies to: 835-836, 994-995, 1032-1033, 1058-1059
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/test/Rollup.t.sol` around lines 763 - 764, Add a single end-to-end test in Rollup.t.sol that exercises the stored-hash lifecycle: call _setStoredBlobHash(1) (the initial commit path) and assert batchBlobVersionedHashes is populated, call revertBatch() and assert the stored hash remains unchanged, call commitState() and assert it reuses the stored hash for state commitment, and finally ensure commitBatch() still reverts when neither a stored hash nor an on-chain blob is present; apply the same pattern to the other similar fixtures referenced around the _setStoredBlobHash usages so the production flow (populate → preserve on revert → reuse on commitState → revert if absent) is fully covered.contracts/contracts/l1/rollup/Rollup.sol (1)
224-225: Confirm the rollout drains or backfills in-flight batches.These are the only new write sites for
batchBlobVersionedHashes. If a live proxy is upgraded while batches are already committed but not finalized, those historical indices stay zeroed andcommitState()cannot reuse their blob hash after a revert. Please either roll this out with no pending batches or add a reinitializer/backfill for the current unfinalized range.Also applies to: 347-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/l1/rollup/Rollup.sol` around lines 224 - 225, The new write site for batchBlobVersionedHashes (via BatchHeaderCodecV0.getBlobVersionedHash) can leave historical indices zero after an upgrade if there are in-flight/unfinalized batches; add a reinitializer/backfill to populate batchBlobVersionedHashes for the current unfinalized index range so commitState() can reuse blob hashes. Implement a function (e.g., backfillBatchBlobVersionedHashes(uint64 startIndex, uint64 endIndex)) that iterates the range, recomputes each blob hash from the stored batch header using the same BatchHeaderCodecV0.getBlobVersionedHash logic, writes into batchBlobVersionedHashes, protects the call with an appropriate admin modifier (onlyOwner/onlyProxyAdmin) and guard so it can only run during upgrade/deployment (or require no pending batches), and ensure your upgrade script calls it for the unfinalized range when rolling out; alternatively, document that the rollout must occur only when there are no pending batches.contracts/contracts/test/base/L1MessageBase.t.sol (1)
49-57: Avoid a magic storage slot in the test helper.
173is coupled to the current linearized storage layout ofRollupand its OpenZeppelin bases. The next storage change will invalidate this helper, so please prefer a harness setter or a storage helper keyed offbatchBlobVersionedHashes(uint256)instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/test/base/L1MessageBase.t.sol` around lines 49 - 57, Replace the hard-coded storage slot approach in _setStoredBlobHash (which uses BATCH_BLOB_VERSIONED_HASHES_SLOT = 173) with a deterministic, resilient setter: add a test-only setter on the Rollup harness (e.g., function setBatchBlobVersionedHash(uint256 batchIndex, bytes32 hash) external onlyTest or in a RollupTestHarness) and call that from _setStoredBlobHash to write ZERO_VERSIONED_HASH, or alternatively compute the mapping slot dynamically by deriving the mapping's storage slot identifier from the Rollup contract (but preferred is adding the harness setter). Update references to BATCH_BLOB_VERSIONED_HASHES_SLOT and the hevm.store call to use the new harness setter (target symbols: _setStoredBlobHash and ZERO_VERSIONED_HASH and rollup.setBatchBlobVersionedHash).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/contracts/l1/rollup/Rollup.sol`:
- Around line 107-109: Stored blobVersionedHash in mapping
batchBlobVersionedHashes prevents replacing hashes for batches that were
reverted; modify commit paths to allow overwriting that entry when a batch has
been reverted (or otherwise not active/finalized). In practice, update
commitBatch() and commitBatchWithProof() to check batch status (use whatever
batch metadata/flag tracked by revertBatch(), e.g., a "reverted" or "finalized"
marker) and permit writing batchBlobVersionedHashes[batchIndex] = newHash when
the batch is marked reverted/not finalized (but keep existing protection when
the batch is active/finalized); also ensure revertBatch() sets the status flag
used to allow this replacement.
---
Nitpick comments:
In `@contracts/contracts/l1/rollup/Rollup.sol`:
- Around line 224-225: The new write site for batchBlobVersionedHashes (via
BatchHeaderCodecV0.getBlobVersionedHash) can leave historical indices zero after
an upgrade if there are in-flight/unfinalized batches; add a
reinitializer/backfill to populate batchBlobVersionedHashes for the current
unfinalized index range so commitState() can reuse blob hashes. Implement a
function (e.g., backfillBatchBlobVersionedHashes(uint64 startIndex, uint64
endIndex)) that iterates the range, recomputes each blob hash from the stored
batch header using the same BatchHeaderCodecV0.getBlobVersionedHash logic,
writes into batchBlobVersionedHashes, protects the call with an appropriate
admin modifier (onlyOwner/onlyProxyAdmin) and guard so it can only run during
upgrade/deployment (or require no pending batches), and ensure your upgrade
script calls it for the unfinalized range when rolling out; alternatively,
document that the rollout must occur only when there are no pending batches.
In `@contracts/contracts/test/base/L1MessageBase.t.sol`:
- Around line 49-57: Replace the hard-coded storage slot approach in
_setStoredBlobHash (which uses BATCH_BLOB_VERSIONED_HASHES_SLOT = 173) with a
deterministic, resilient setter: add a test-only setter on the Rollup harness
(e.g., function setBatchBlobVersionedHash(uint256 batchIndex, bytes32 hash)
external onlyTest or in a RollupTestHarness) and call that from
_setStoredBlobHash to write ZERO_VERSIONED_HASH, or alternatively compute the
mapping slot dynamically by deriving the mapping's storage slot identifier from
the Rollup contract (but preferred is adding the harness setter). Update
references to BATCH_BLOB_VERSIONED_HASHES_SLOT and the hevm.store call to use
the new harness setter (target symbols: _setStoredBlobHash and
ZERO_VERSIONED_HASH and rollup.setBatchBlobVersionedHash).
In `@contracts/contracts/test/Rollup.t.sol`:
- Around line 763-764: Add a single end-to-end test in Rollup.t.sol that
exercises the stored-hash lifecycle: call _setStoredBlobHash(1) (the initial
commit path) and assert batchBlobVersionedHashes is populated, call
revertBatch() and assert the stored hash remains unchanged, call commitState()
and assert it reuses the stored hash for state commitment, and finally ensure
commitBatch() still reverts when neither a stored hash nor an on-chain blob is
present; apply the same pattern to the other similar fixtures referenced around
the _setStoredBlobHash usages so the production flow (populate → preserve on
revert → reuse on commitState → revert if absent) is fully covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4b81342-9dfa-407e-87a9-6a13d34757ee
📒 Files selected for processing (4)
contracts/contracts/l1/rollup/IRollup.solcontracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.solcontracts/contracts/test/base/L1MessageBase.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/contracts/test/Rollup.t.sol (1)
1005-1010: Clarify the test comment.The comment says "batch is already committed, revert" but the actual revert reason is
"commitBatch requires no stored blob hash". The test is valid - it verifies that after committing viacommitBatchWithProof, the blob hash is stored andcommitBatchcorrectly rejects duplicate commits. Consider updating the comment to reflect the new behavior.📝 Suggested comment clarification
- // batch is already committed, revert (commitBatch also reverts with "commitBatch requires no stored blob hash" when slot is set) + // batch 1 already committed via commitBatchWithProof; commitBatch rejects because stored blob hash exists🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/test/Rollup.t.sol` around lines 1005 - 1010, The test comment is misleading: the revert comes from commitBatch rejecting a stored blob hash set earlier by commitBatchWithProof, not simply "batch already committed"; update the comment above the test invoking rollup.commitBatch(batchDataInput, batchSignatureInput) to state that commitBatch reverts with "commitBatch requires no stored blob hash" because commitBatchWithProof stored a blob hash for that batch (reference commitBatchWithProof, commitBatch, IRollup.BatchDataInput, batchDataInput, batchSignatureInput, getTreeRoot), so the comment clearly describes that the duplicate commit is rejected due to an existing stored blob hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/contracts/test/base/L1MessageBase.t.sol`:
- Around line 50-53: Add explicit documentation and an automated validation:
update the test to include a README-style comment showing the exact command to
regenerate storage slots (e.g., "forge inspect Rollup storage-layout") and add a
short test function (e.g., testValidateStorageSlots) that deploys or references
the Rollup instance and uses vm.load with the constants
BATCH_BLOB_VERSIONED_HASHES_SLOT and ROLLUP_DELAY_PERIOD_SLOT to read those
storage slots and assert they match the on-chain getters/expected values (for
rollupDelayPeriod compare to Rollup.rollupDelayPeriod(), for
batchBlobVersionedHashes check length/hash presence or known value), while
keeping ZERO_VERSIONED_HASH unchanged; this provides both reproduction
instructions and a runtime check to catch storage-layout drift.
---
Nitpick comments:
In `@contracts/contracts/test/Rollup.t.sol`:
- Around line 1005-1010: The test comment is misleading: the revert comes from
commitBatch rejecting a stored blob hash set earlier by commitBatchWithProof,
not simply "batch already committed"; update the comment above the test invoking
rollup.commitBatch(batchDataInput, batchSignatureInput) to state that
commitBatch reverts with "commitBatch requires no stored blob hash" because
commitBatchWithProof stored a blob hash for that batch (reference
commitBatchWithProof, commitBatch, IRollup.BatchDataInput, batchDataInput,
batchSignatureInput, getTreeRoot), so the comment clearly describes that the
duplicate commit is rejected due to an existing stored blob hash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 078bf2a7-7863-48b2-adbe-02c02da182fa
📒 Files selected for processing (6)
bindings/bin/rollup_deployed.hexbindings/bindings/rollup.gobindings/bindings/rollup_more.gocontracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.solcontracts/contracts/test/base/L1MessageBase.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/bin/rollup_deployed.hex
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/contracts/l1/rollup/Rollup.sol`:
- Around line 108-110: Pre-upgrade committed but unfinalized batches have
batchBlobVersionedHashes == bytes32(0) so they cannot use commitState() after
revert; add a controlled backfill to populate those entries. Implement an
owner-only function (e.g., backfillBatchBlobVersionedHashes) that iterates from
lastFinalizedBatchIndex+1 up to lastCommittedBatchIndex, derives the same
versioned hash used on genesis/future commits (use the same logic as in the
genesis import and commit paths), writes batchBlobVersionedHashes[index] for
each committed batch if unset, is idempotent (skips already-populated entries),
validates there are no missing batch data needed to compute the hash (revert on
gap), and emits an event per-filled entry; protect it with the same access
control as upgrades so operators can run this once after migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3c75450-4ceb-43b7-b056-ec859fee5b7c
📒 Files selected for processing (1)
contracts/contracts/l1/rollup/Rollup.sol
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
1 participant
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
Updates
Tests
Chores