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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new commitState entrypoint and per-batch Changes
Sequence Diagram(s)sequenceDiagram
actor Staker
participant Rollup
participant Storage as batchBlobVersionedHashes
Note over Staker,Rollup: Initial commit with blob
Staker->>Rollup: commitBatch(batchData, sig, blob)
Rollup->>Rollup: _commitBatchWithBatchData(..., requireBlobWhenNoStoredHash=true)
Rollup->>Rollup: extract blob versioned hash
Rollup->>Storage: store batchBlobVersionedHashes[batchIndex] = hash
Rollup-->>Staker: batch committed
Note over Staker,Rollup: Re-commit after revert using stored hash
Staker->>Rollup: commitState(batchData, sig)
Rollup->>Storage: read batchBlobVersionedHashes[batchIndex]
Storage-->>Rollup: return stored hash
Rollup->>Rollup: use stored hash (no blob required)
Rollup-->>Staker: state committed
Note over Staker,Rollup: Finalize cleans up
Staker->>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 You can get early access to new features in CodeRabbit.Enable the |
![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
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