Sequencer failure #849
Sequencer failure #849
Conversation
📝 WalkthroughWalkthroughAdds per-message enqueue timestamps and accessors to the L1 message queue, extends Rollup with delay/timing checks, new commitBatchWithProof flow (consistency and zk-proof verification), new tests exercising stalled commit scenarios, and updates Go bindings plus deployed bytecode artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Proposer
participant Rollup
participant L1MessageQueue
participant Verifier
participant BatchState
Proposer->>Rollup: commitBatchWithProof(batchData, sig, header, proof)
activate Rollup
Rollup->>L1MessageQueue: getFirstUnfinalizedMessageEnqueueTime()
activate L1MessageQueue
L1MessageQueue-->>Rollup: enqueueTimestamp
deactivate L1MessageQueue
rect rgb(230,240,255)
note right of Rollup: Check enqueueTimestamp + rollupDelayPeriod
Rollup->>Rollup: revert InvalidTiming() if timing not met
end
rect rgb(240,230,255)
note right of Rollup: Revert/adjust batch range and verify consistency
Rollup->>BatchState: revertRange/update headers
Rollup->>Rollup: _verifyBatchConsistency(batchData, header)
end
rect rgb(230,255,240)
note right of Rollup: Verify zk-proof via external verifier
Rollup->>Verifier: _verifyProof(publicInputs, proof)
activate Verifier
alt proof valid
Verifier-->>Rollup: valid
else proof invalid
Verifier-->>Rollup: invalid (revert)
end
deactivate Verifier
end
Rollup->>BatchState: finalizeBatch(header)
Rollup-->>Proposer: success (emit events)
deactivate Rollup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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
| require(committedBatches[_batchIndex] == _batchHash, "incorrect batch hash"); | ||
|
|
||
| // verify consistency between batchDataInput and batchHeader | ||
| _verifyBatchConsistency(batchDataInput, memPtr); |
There was a problem hiding this comment.
The data hash check contains all these checks
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.