Sequencer failure #849
Sequencer failure #849
Conversation
📝 WalkthroughWalkthroughAdds per-message enqueue timestamps and read accessors to the L1 message queue; introduces a rollupDelayPeriod, timing checks, and a new commitBatchWithProof flow in Rollup; adds tests for stalled commit scenarios; updates Go bindings and deployed bytecode artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Proposer
participant Rollup
participant L1MessageQueue
participant Verifier
Proposer->>Rollup: commitBatchWithProof(batchData, sig, header, proof)
activate Rollup
Rollup->>L1MessageQueue: getFirstUnfinalizedMessageEnqueueTime()
activate L1MessageQueue
L1MessageQueue-->>Rollup: enqueueTimestamp
deactivate L1MessageQueue
alt timing too recent
Rollup-->>Proposer: revert (timing not met)
else timing OK
Rollup->>Rollup: validate batch data and header
Rollup->>Verifier: verifyProof(batchHeader, proof)
activate Verifier
Verifier-->>Rollup: proof result
deactivate Verifier
alt proof valid
Rollup->>Proposer: finalize/commit batch (emit events)
else proof invalid
Rollup-->>Proposer: revert
end
end
deactivate Rollup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-01-15T15:49:18.270ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @contracts/contracts/l1/rollup/Rollup.sol:
- Around line 103-104: The state variable rollupDelayPeird is misspelled, never
initialized, and lacks a setter causing it to default to 0; rename it to
rollupDelayPeriod, initialize rollupDelayPeriod to a sensible nonzero default
inside initialize(), add an onlyOwner setter function
updateRollupDelayPeriod(uint256 _newPeriod) that requires _newPeriod > 0,
updates rollupDelayPeriod, emits an UpdateRollupDelayPeriod(old, new) event
(declare that event), and ensure any timing checks reference rollupDelayPeriod
after these changes.
- Around line 103-104: There is a typo in the public state variable name
rollupDelayPeird — rename it to rollupDelayPeriod everywhere in the contract and
tests to avoid breaking the external API; update the declaration (uint256 public
rollupDelayPeriod), all internal references (e.g., uses around the delay logic
and any getter access), and any test comments/constants that reference the old
name (e.g., ROLLUP_DELAY_PERIOD_SLOT in Rollup.t.sol) so names and comments
match the corrected identifier.
- Around line 349-357: In commitBatchWithProof where you currently only zero out
committedBatches[i] when reverting ranges, extend the cleanup inside the revert
loop to mirror revertBatch: check and handle batchInChallenge(i) deposit logic,
clear batchDataStore[i], clear committedStateRoots[i], clear challenges[i], and
remove any revert request index/state for that batch; ensure you still emit
RevertBatchRange and update lastCommittedBatchIndex to _parentBatchIndex after
the loop so no stale data or challenge deposits remain for reverted batches.
- Line 373: commitBatchWithProof calls finalizeBatch immediately but
_commitBatchWithBatchData sets batchDataStore[_batchIndex].finalizeTimestamp to
block.timestamp + finalizationPeriodSeconds + proveRemainingTime, causing
finalizeBatch to revert with "batch in challenge window"; fix by changing the
commit path so proof-verified batches are considered immediately finalizable:
either (A) add and call an internal finalize function that performs the same
state changes as finalizeBatch but skips the finalizeTimestamp challenge-window
check, (B) set batchDataStore[_batchIndex].finalizeTimestamp = 0 or =
block.timestamp when committing from commitBatchWithProof so the check in
finalizeBatch passes, or (C) add a boolean flag to _commitBatchWithBatchData
(e.g., skipFinalizeWindow) and, when true, avoid advancing finalizeTimestamp
into the future; update commitBatchWithProof to use the chosen approach and
ensure symbols _commitBatchWithBatchData, commitBatchWithProof,
batchDataStore[_batchIndex].finalizeTimestamp, and finalizeBatch are adjusted
consistently.
🧹 Nitpick comments (3)
contracts/contracts/test/Rollup.t.sol (2)
20-23: Hardcoded storage slot constants may break if storage layout changes.The slot constants (
ROLLUP_DELAY_PERIOD_SLOT = 172,FINALIZATION_PERIOD_SLOT = 152) are derived from the current storage layout. If theRollupcontract's storage layout changes, these tests will silently fail to set the intended variables.Consider adding a comment to regenerate these values when the contract changes, or verifying them programmatically.
11-68: Good test coverage, but consider adding edge cases.The test suite covers many scenarios well. Consider adding tests for:
commitBatchWithProofwheninChallengeis true (should revert with "already in challenge")- Verifying behavior when proof is empty (should revert with "Invalid batch proof")
contracts/contracts/l1/rollup/Rollup.sol (1)
678-718: MissinglastBlockNumberconsistency check.The
_verifyBatchConsistencyfunction verifies version, numL1Messages, state roots, and withdrawal root, but does not verifylastBlockNumberfrombatchDataInputagainst the batch header. While the dataHash computation includes lastBlockNumber (and thus batch hash mismatch would occur), an explicit check would be more defensive.🔎 Proposed addition
function _verifyBatchConsistency( BatchDataInput calldata batchDataInput, uint256 memPtr ) private pure { + // verify lastBlockNumber (for version >= 1) + if (batchDataInput.version >= 1) { + require( + batchDataInput.lastBlockNumber == BatchHeaderCodecV1.getLastBlockNumber(memPtr), + "lastBlockNumber mismatch" + ); + } + // verify version require(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/contracts/l1/rollup/IL1MessageQueue.solcontracts/contracts/l1/rollup/IRollup.solcontracts/contracts/l1/rollup/L1MessageQueueWithGasPriceOracle.solcontracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
contracts/contracts/l1/rollup/IRollup.sol (1)
75-77: LGTM!The new
InvalidTimingerror andRevertBatchRangeevent are well-defined with appropriate NatSpec documentation. They align with the newcommitBatchWithProoffunctionality inRollup.sol.Also applies to: 92-96
contracts/contracts/l1/rollup/IL1MessageQueue.sol (1)
60-64: LGTM!The new view function is well-documented and provides necessary timing data for the permissionless batch submission flow in
Rollup.commitBatchWithProof.contracts/contracts/l1/rollup/L1MessageQueueWithGasPriceOracle.sol (3)
265-271: LGTM!The getter functions correctly return the enqueue timestamp. Returning 0 when no pending messages exist is appropriate behavior for the timing checks in
commitBatchWithProof.
376-376: LGTM!Recording
block.timestampduring message enqueue is correctly placed within_queueTransaction.
60-61: No action needed—storage layout is safe.The new
messageEnqueueTimemapping is placed in a fresh storage slot (afterwhitelistChecker) with no collision against deprecated variables. The deprecated storage (__droppedMessageBitmap,__skippedMessageBitmap) is positioned earlier in the layout, and the new mapping correctly records timestamps at line 376 during transaction queueing.contracts/contracts/test/Rollup.t.sol (2)
69-79: LGTM!The
_computeDataHashhelper correctly mirrors the data hash computation logic fromRollup._commitBatch.
394-432: LGTM!The success path test
test_commitBatchWithProof_succeeds_when_stalledproperly validates the complete flow including finalization and state updates.contracts/contracts/l1/rollup/Rollup.sol (2)
343-347: Edge case: timing check may fail whenlastCommittedBatchIndexis 0 (genesis only).When
lastCommittedBatchIndex == 0(only genesis batch imported),batchDataStore[0].originTimestampwas set duringimportGenesisBatch. However, if the genesis was imported long ago, this would correctly indicate a stall. The logic appears sound.The timing logic correctly implements the OR semantics: revert only if BOTH batch submission AND L1 message processing are recent (not stalled).
221-226: LGTM!Clean refactoring of
commitBatchto delegate to_commitBatchWithBatchData. TheonlyActiveStakermodifier correctly remains on the public function while allowingcommitBatchWithProofto be permissionless.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @contracts/contracts/l1/rollup/Rollup.sol:
- Around line 473-480: The setter and emitted event contain a typo: rename the
function updateRollupDelayPeird to updateRollupDelayPeriod and the event
RollupDelayPeirdUpdate to RollupDelayPeriodUpdate, update the emit call inside
the function to use the new event name, and search/replace any references to
updateRollupDelayPeird, RollupDelayPeirdUpdate, and rollupDelayPeird (if you
also intend to fix the state variable name) across the contract, tests,
interfaces and deployments so all identifiers remain consistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bindings/bin/rollup_deployed.hexbindings/bindings/rollup.gobindings/bindings/rollup_more.gocontracts/contracts/l1/rollup/Rollup.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
contracts/contracts/l1/rollup/Rollup.sol (8)
226-231: LGTM: Clean refactoring to enable code reuse.The
commitBatchfunction correctly delegates to the new internal_commitBatchWithBatchDatafunction, maintaining all existing access control and validation modifiers.
376-376: LGTM: Challenge window issue correctly addressed.Setting
finalizeTimestamp = block.timestampallows immediate finalization at line 381, correctly resolving the previously flagged issue where proof-verified batches would revert due to the challenge window check.
692-735: LGTM: Comprehensive batch consistency validation.The
_verifyBatchConsistencyfunction correctly validates all critical fields betweenBatchDataInputand the loaded batch header with clear error messages for each mismatch.
850-850: LGTM: Correct routing for batch header version 1.The update correctly routes version 1 batch headers to
BatchHeaderCodecV1.loadAndValidate.
103-104: Typo in state variable name persists.The typo
rollupDelayPeird(should berollupDelayPeriod) remains in the public state variable declaration. This was previously flagged and affects the external API.Likely an incorrect or invalid review comment.
188-192: Initialization added but typo persists in function and event names.The
initialize3function correctly initializes the delay period with validation, but the typorollupDelayPeird(should berollupDelayPeriod) persists in the parameter name, storage variable, and emitted event name.Likely an incorrect or invalid review comment.
354-362: Incomplete batch reversion cleanup persists.The batch reversion loop still only clears
committedBatches[i]. As previously flagged, this misses cleanup ofbatchDataStore,committedStateRoots,challenges, and challenge deposit handling that therevertBatchfunction performs (lines 393-416).Likely an incorrect or invalid review comment.
348-351: Clarify implicit behavior when the message queue is empty.The timing check calls
getFirstUnfinalizedMessageEnqueueTime(), which returnsmessageEnqueueTime[pendingQueueIndex]. When no unfinalized messages exist, this returns 0 (the default mapping value), making the second condition0 + rollupDelayPeird >= block.timestampevaluate to false. While this permits submission when the queue is empty—which appears intentional—the implicit behavior is fragile. The function has no explicit way to distinguish "no unfinalized messages" from "message enqueued at block timestamp 0," and the timing logic implicitly assumesblock.timestamp > rollupDelayPeird. Consider documenting this behavior or using a sentinel value (e.g.,type(uint256).max) to explicitly represent the "no messages" case.bindings/bin/rollup_deployed.hex (1)
1-1: Deployed bytecode updated to reflect contract changes.The deployed bytecode has been regenerated to include the new public APIs (
commitBatchWithProof,initialize3,updateRollupDelayPeird) and internal helpers. Ensure this artifact is regenerated after fixing the typo inrollupDelayPeirdto maintain consistency between source and deployed binaries.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @contracts/contracts/l1/rollup/Rollup.sol:
- Around line 354-362: The loop that reverts a range of batches only clears
committedBatches[i] but must mirror the cleanup in revertBatch: for each i from
_parentBatchIndex+1 to lastCommittedBatchIndex also clear batchDataStore[i],
committedStateRoots[i], reset any revert request index for i, handle and return
challenge deposits if batchInChallenge(i) (same logic as in revertBatch), and
clear challenges[i]; after doing these per-batch cleanups leave
lastCommittedBatchIndex set to _parentBatchIndex.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bindings/bin/rollup_deployed.hexbindings/bindings/rollup.gobindings/bindings/rollup_more.gocontracts/contracts/l1/rollup/IRollup.solcontracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/bin/rollup_deployed.hex
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
contracts/contracts/l1/rollup/IRollup.sol (1)
75-76: LGTM! Interface declarations are well-structured.The new error and events are properly declared with clear documentation and appropriate indexing for efficient filtering.
Also applies to: 92-95, 129-132
contracts/contracts/l1/rollup/Rollup.sol (5)
188-192: LGTM! Proper initialization with validation.The
initialize3function correctly validates the delay period is non-zero and emits the appropriate event.
230-231: LGTM! Clean refactoring for code reuse.Extracting the batch commit logic into
_commitBatchWithBatchDataallows the newcommitBatchWithProoffunction to reuse the same validation and commitment logic.
348-351: Verify timing check behavior when genesis batch is the last finalized.When
lastCommittedBatchIndexequals 0 (genesis batch finalized),batchDataStore[0]is deleted (line 614), makingoriginTimestampzero. The expression0 + rollupDelayPeriodyields an old timestamp, treating the batch submission path as stalled and allowing permissionless submission to proceed.Confirm this is the intended behavior for the genesis edge case.
473-480: LGTM! Proper setter with validation.The function correctly validates the new period is positive and different from the current value, with appropriate event emission.
692-735: LGTM! Comprehensive consistency validation.The function thoroughly verifies all critical fields between
BatchDataInputand the loaded batch header, with clear error messages for each check.contracts/contracts/test/Rollup.t.sol (3)
27-67: LGTM! Proper test setup with realistic fixtures.The setUp function correctly initializes the genesis batch, registers a staker, and configures the rollup delay period using storage manipulation, which is appropriate for testing.
69-142: LGTM! Well-structured test helpers.The helper functions properly encapsulate batch header creation, hash computation, and mocking of external dependencies, making tests more readable and maintainable.
145-606: LGTM! Comprehensive test coverage for the new feature.The test suite thoroughly exercises
commitBatchWithProofwith excellent coverage of:
- Pre-condition validations (parent finalized, timing, version, pause state)
- Input validations (zero roots, incorrect hashes)
- Success paths with proper state transitions
- Event emissions and external contract interactions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/contracts/l1/rollup/Rollup.sol (1)
188-193: Missing blank line before next section comment.Minor formatting nit: there's no blank line between the function closing brace and the
/************************comment block.🔧 Suggested fix
function initialize3(uint256 _rollupDelayPeriod) external reinitializer(3) { require(_rollupDelayPeriod != 0, "invalid rollup delay period"); rollupDelayPeriod = _rollupDelayPeriod; emit RollupDelayPeriodUpdate(0, _rollupDelayPeriod); } + /************************ * Restricted Functions * ************************/contracts/contracts/test/Rollup.t.sol (1)
86-111: Helper assumes parent batch has zero total L1 messages.The helper sets both
l1MessagePoppedandtotalL1MessagePoppedtonumL1Messages(lines 101-102). This is correct only when the parent batch (genesis) hastotalL1MessagePopped = 0. For tests involving multiple batches, this helper would need modification to account for the cumulative total.Consider adding a parameter for
parentTotalL1MessagePoppedor documenting this limitation:function _createMatchingBatchHeader( uint64 lastBlockNumber, uint16 numL1Messages, bytes32 prevStateRoot, bytes32 postStateRoot, - bytes32 withdrawalRoot + bytes32 withdrawalRoot, + uint64 parentTotalL1MessagePopped // Add parameter for multi-batch tests ) internal view returns (bytes memory batchHeader1) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
contracts/contracts/l1/rollup/Rollup.sol (3)
103-104: State variable declaration looks good.The typo from previous reviews (
rollupDelayPeird) has been corrected torollupDelayPeriod.
239-339: Batch commitment logic correctly extracted.The internal
_commitBatchWithBatchDatafunction properly encapsulates the batch validation and commitment logic, enabling reuse by bothcommitBatchandcommitBatchWithProof.
464-471: Update function follows established patterns.The
updateRollupDelayPeriodfunction correctly validates input, stores the old value for the event, and emitsRollupDelayPeriodUpdate. Implementation is consistent with other update functions likeupdateProofWindowandupdateFinalizePeriodSeconds.contracts/contracts/test/Rollup.t.sol (5)
491-540: Well-documented edge case test.The test thoroughly documents the timing calculation in comments (lines 493-503) and correctly sets up conditions to trigger the "l1msg delay" error. The math is verified:
rollupDelay = 1 + 3600 < 3601 = falsel1MsgQueueDelayed = 0 + 3600 < 3601 = true
343-380: Good coverage of the happy path.The test properly verifies:
- Batch is committed (
lastCommittedBatchIndex == 1)- Batch is not auto-finalized (
lastFinalizedBatchIndex == 0)- Uses correct mocks for verifier and message queue
454-489: Test validates immediate finalization capability.The test correctly verifies that after
commitBatchWithProof, the batch is no longer inside the challenge window (batchInsideChallengeWindow(1) == false), confirming thefinalizeTimestampoverride for ZKP-backed immediate finality.
11-541: Comprehensive test coverage forcommitBatchWithProof.The new
RollupCommitBatchWithProofTestcontract provides thorough coverage of thecommitBatchWithProoffunction including:
- Timing requirement validations
- Version, state root, and parent batch validations
- Pause behavior
- Event emission
- Verifier invocation
- Immediate finalization timestamp setting
20-22: Document how the storage slot was determined and verify periodically.The test hardcodes
ROLLUP_DELAY_PERIOD_SLOT = 172, which is already documented as coming fromforge inspect Rollup storageLayout. While the source is documented, this approach is fragile—the slot can become stale if the Rollup contract's storage layout changes in the future (e.g., if inherited contracts or state variables are added/removed).Consider one of the following:
- Add a helper function or assertion in the test setup that verifies the slot value matches the current contract layout
- Document the verification process to be repeated after any contract storage modifications
- Use a contract's storage offset calculation or introspection if available
Also applies to: 65
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
contracts/contracts/l1/rollup/L1MessageQueueWithGasPriceOracle.sol
Outdated
Show resolved
Hide resolved
| // check l1msg delay - sequencer must process L1 messages when delayed | ||
| if ( | ||
| IL1MessageQueue(messageQueue).getFirstUnfinalizedMessageEnqueueTime() + rollupDelayPeriod < block.timestamp | ||
| ) { | ||
| require(batchDataInput.numL1Messages > 0, "l1msg delay"); | ||
| } |
There was a problem hiding this comment.
Why not check rollupDelay?
There was a problem hiding this comment.
Because commitBatch invalidates rollupDelay, which is exactly what we expect.
| uint256 _batchIndex = BatchHeaderCodecV0.getBatchIndex(memPtr); | ||
| bytes32 parentBatchHashFromHeader = BatchHeaderCodecV0.getParentBatchHash(memPtr); | ||
| (, bytes32 parentBatchHashFromInfo) = _loadBatchHeader(batchDataInput.parentBatchHeader); | ||
| require(parentBatchHashFromHeader == parentBatchHashFromInfo, "batch info mismatch with batch header"); | ||
| require(committedBatches[_batchIndex] == _batchHash, "incorrect batch hash"); |
There was a problem hiding this comment.
| uint256 _batchIndex = BatchHeaderCodecV0.getBatchIndex(memPtr); | |
| bytes32 parentBatchHashFromHeader = BatchHeaderCodecV0.getParentBatchHash(memPtr); | |
| (, bytes32 parentBatchHashFromInfo) = _loadBatchHeader(batchDataInput.parentBatchHeader); | |
| require(parentBatchHashFromHeader == parentBatchHashFromInfo, "batch info mismatch with batch header"); | |
| require(committedBatches[_batchIndex] == _batchHash, "incorrect batch hash"); | |
| uint256 _batchIndex = BatchHeaderCodecV0.getBatchIndex(memPtr); | |
| require(lastCommittedBatchIndex == _batchIndex+1 , "incorrect batch header"); | |
| require(committedBatches[_batchIndex] == _batchHash, "incorrect batch hash"); |
There was a problem hiding this comment.
The correctness of batchHash in_batchHeader and input.parentBatchHeader has been checked and stored in committedBatches, so only check the batchIndex?
There was a problem hiding this comment.
LGTM, but lastCommittedBatchIndex should equal with _batchIndex . like: require(lastCommittedBatchIndex == _batchIndex , "incorrect batch header");
….sol Co-authored-by: vincent <vincent.ch.cn@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@contracts/contracts/l1/rollup/Rollup.sol`:
- Around line 103-104: The code allows rollupDelayPeriod == 0 to be treated as a
valid period which can make rollupDelay and l1MsgQueueDelayed evaluate true and
enable permissionless commitBatchWithProof; change the logic to treat
rollupDelayPeriod == 0 as uninitialized and fail fast by (a) adding a guard
require(rollupDelayPeriod != 0, "rollupDelayPeriod uninitialized") at the start
of commitBatchWithProof (or its internal entry point) and (b) update any boolean
computations that derive rollupDelay or l1MsgQueueDelayed (referencing the
rollupDelayPeriod variable and the functions/blocks around lines 188-192 and
341-361) to first ensure rollupDelayPeriod != 0 before using it in comparisons;
also ensure initialize3 sets rollupDelayPeriod before any public functions can
be called.
- Around line 188-192: The initialize3 function currently emits
RollupDelayPeriodUpdate(0, _rollupDelayPeriod) regardless of any prior value;
change it to capture the current rollupDelayPeriod into a local old variable
before assignment and emit RollupDelayPeriodUpdate(old, _rollupDelayPeriod) (or
alternatively add a require(rollupDelayPeriod == 0, "already initialized") to
forbid prior setting); update references in initialize3 and ensure consistency
with updateRollupDelayPeriod event semantics.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bindings/bin/l1messagequeuewithgaspriceoracle_deployed.hexbindings/bin/rollup_deployed.hexbindings/bindings/l1messagequeuewithgaspriceoracle.gobindings/bindings/l1messagequeuewithgaspriceoracle_more.gobindings/bindings/rollup.gobindings/bindings/rollup_more.gocontracts/contracts/l1/rollup/Rollup.sol
🧰 Additional context used
🧬 Code graph analysis (1)
bindings/bindings/l1messagequeuewithgaspriceoracle_more.go (1)
bindings/solc/types.go (1)
StorageLayout(47-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
bindings/bin/rollup_deployed.hex (1)
1-1: Deployed bytecode blob change needs a reproducibility/check step.
Since this is opaque, please ensure CI (or a script) deterministically rebuilds this artifact from the Solidity sources and fails if it diverges (guards against accidental/wrong-network bytecode).bindings/bin/l1messagequeuewithgaspriceoracle_deployed.hex (1)
1-1: Deployed bytecode blob change should be validated against ABI + layout.
Please confirm this bytecode is produced from the same build as the updated ABI inbindings/bindings/l1messagequeuewithgaspriceoracle.goand the layout in..._more.go.contracts/contracts/l1/rollup/Rollup.sol (4)
239-327:_commitBatchWithBatchDataconsolidation looks good; double-check permissionless callers vsgetStakerBitmap(msg.sender).
IfcommitBatchWithProofis intended to be permissionless, ensureIL1Staking(l1StakingContract).getStakerBitmap(_msgSender())(Line 322) won’t revert for non-stakers (or explicitly decide it should).
341-376: Good: binds proof to the just-committed batch and enables immediate finalization.
require(lastCommittedBatchIndex == _batchIndex, "incorrect batch header")plus overridingfinalizeTimestampaddresses the earlier “prove/finalize window” mismatch and batch-index confusion.
467-474: LGTM: owner-controlled delay period update with event emission.
The validations (>0and!= current) are sensible.
229-237: No action needed—the delay check is safe.
getFirstUnfinalizedMessageEnqueueTime()returnsblock.timestampwhen the queue is empty (not 0), which prevents the described misfire. The checkblock.timestamp + rollupDelayPeriod < block.timestampwill always evaluate to false when the queue is empty, so the require won't be triggered in that case.bindings/bindings/l1messagequeuewithgaspriceoracle.go (2)
34-35: ABI/Bin update: ensure it matches the deployed artifact + solc output for this PR.
Given multiple sources of truth (this file,..._more.go, andbindings/bin/...hex), it’s easy to drift.
422-575: LGTM: new enqueue-time getters are exposed consistently across Caller/Session types.
The added methods align with the ABI additions (getFirstUnfinalizedMessageEnqueueTime,getMessageEnqueueTimestamp,messageEnqueueTime).bindings/bindings/l1messagequeuewithgaspriceoracle_more.go (2)
12-12: Storage layout JSON change: please confirm it’s regenerated from solc for this exact contract version.
In particular, the__gapsizing swap (1014_storagevs1013_storage) should match the Solidity source’s inheritance/storage packing expectations.
16-16: Deployed bin update: keep it in lockstep withbindings/bin/l1messagequeuewithgaspriceoracle_deployed.hex.
A mismatch here will be painful to debug at runtime.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/contracts/l1/rollup/IRollup.sol (1)
188-191: AddcommitBatchWithProofto theIRollupinterface.The
commitBatchWithProoffunction exists inRollup.solas an external, payable function designed for permissionless batch submission when the system is stalled. Since the similarcommitBatchfunction is declared in this interface,commitBatchWithProofshould also be declared here for consistency and to enable external contract integrations via the interface.
🧹 Nitpick comments (1)
contracts/contracts/l1/rollup/IRollup.sol (1)
124-128: Consider renaming for consistency with other events.All other update events in this interface follow a verb-first naming convention (e.g.,
UpdateProofWindow,UpdateFinalizationPeriodSeconds), but this event uses subject-first naming (RollupDelayPeriodUpdate).✨ Suggested rename for consistency
- /// `@notice` Emitted when the rollup delay period is updated. - /// `@param` oldPeriod The old rollupDelayPeriod. - /// `@param` newPeriod The new rollupDelayPeriod. - event RollupDelayPeriodUpdate(uint256 oldPeriod, uint256 newPeriod); + /// `@notice` Emitted when the rollup delay period is updated. + /// `@param` oldPeriod The old rollupDelayPeriod. + /// `@param` newPeriod The new rollupDelayPeriod. + event UpdateRollupDelayPeriod(uint256 oldPeriod, uint256 newPeriod);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bindings/bindings/rollup.gocontracts/contracts/l1/rollup/IRollup.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
contracts/contracts/l1/rollup/IRollup.sol (1)
75-77: LGTM!The custom error
InvalidTiming()is correctly declared. Custom errors are a gas-efficient pattern in Solidity 0.8.x compared to revert strings.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.