[Provers L02] Check for non-zero target commitments#92
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR standardizes error handling across all blockchain prover implementations by introducing a unified Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/provers/TAIKO.md (1)
14-18:⚠️ Potential issue | 🟡 MinorCheckpoint storage layout is inaccurate.
The storage layout notes imply
blockNumberis stored in the struct, but in SignalService the mapping key is the block number and the stored struct only containsblockHash(slot+0) andstateRoot(slot+1). Please update the “Checkpoint Structure” diagram and the “Storage Slot Calculation” section to avoid incorrect slot derivation guidance.✏️ Suggested doc fix
- struct Checkpoint { - uint48 blockNumber; // Block number this checkpoint refers to - bytes32 blockHash; // Block hash of that block - bytes32 stateRoot; // State root (may not always be populated) - } + struct Checkpoint { + bytes32 blockHash; // Block hash of that block + bytes32 stateRoot; // State root (may not always be populated) + }-// slot + 0: blockNumber (packed with other small values) -// slot + 0: blockHash (first bytes32 of struct) -// slot + 1: stateRoot +// slot + 0: blockHash (first bytes32 of struct) +// slot + 1: stateRoot +// Note: blockNumber is the mapping key, not stored in the structBased on learnings: “In Taiko's SignalService contract, the mapping
mapping(uint48 blockNumber => CheckpointRecord checkpoint) private _checkpointsstores CheckpointRecord structs with only two fields:bytes32 blockHash(at slot+0) andbytes32 stateRoot(at slot+1). The blockNumber is the mapping key, not stored in the struct. The interface Checkpoint struct includes blockNumber only for ABI/function returns, where it's reconstructed from the function parameter.”Also applies to: 327-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/provers/TAIKO.md` around lines 14 - 18, Update the "Checkpoint Structure" diagram and "Storage Slot Calculation" text to reflect that in SignalService the mapping mapping(uint48 blockNumber => CheckpointRecord checkpoint) private _checkpoints stores only bytes32 blockHash (slot+0) and bytes32 stateRoot (slot+1); blockNumber is the mapping key and is not stored inside the CheckpointRecord struct. Replace any diagram or slot derivation that shows uint48 blockNumber as a struct member, and add a short note that the public/interface Checkpoint struct (used in returns) may include blockNumber for ABI convenience but it is reconstructed from the mapping key, not stored in storage. Ensure references to Checkpoint, CheckpointRecord, and _checkpoints are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/provers/arbitrum/ParentToChildProver.sol`:
- Around line 57-61: The check for targetStateCommitment being zero is
duplicated: remove the redundant require(targetStateCommitment != bytes32(0),
InvalidTargetStateCommitment()) so only the explicit if (...) { revert
InvalidTargetStateCommitment(); } remains; update ParentToChildProver.sol to
delete the require that references targetStateCommitment to avoid the duplicate
zero-check while preserving the single revert error path.
In `@src/contracts/provers/scroll/ParentToChildProver.sol`:
- Around line 33-35: The second require that throws InvalidTargetStateCommitment
is unreachable because StateRootNotFound() already reverts on a zero state root;
consolidate to a single standardized error by either removing the earlier
StateRootNotFound() revert and using InvalidTargetStateCommitment() in its
place, or drop the later require and keep StateRootNotFound(), ensuring only one
of the error symbols (StateRootNotFound() or InvalidTargetStateCommitment()) is
used for the zero-check in ParentToChildProver.sol (apply the same change for
the similar check around the 72-76 region).
---
Outside diff comments:
In `@docs/provers/TAIKO.md`:
- Around line 14-18: Update the "Checkpoint Structure" diagram and "Storage Slot
Calculation" text to reflect that in SignalService the mapping mapping(uint48
blockNumber => CheckpointRecord checkpoint) private _checkpoints stores only
bytes32 blockHash (slot+0) and bytes32 stateRoot (slot+1); blockNumber is the
mapping key and is not stored inside the CheckpointRecord struct. Replace any
diagram or slot derivation that shows uint48 blockNumber as a struct member, and
add a short note that the public/interface Checkpoint struct (used in returns)
may include blockNumber for ABI convenience but it is reconstructed from the
mapping key, not stored in storage. Ensure references to Checkpoint,
CheckpointRecord, and _checkpoints are updated accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/provers/ARBITRUM.mddocs/provers/TAIKO.mdsrc/contracts/provers/arbitrum/ChildToParentProver.solsrc/contracts/provers/arbitrum/ParentToChildProver.solsrc/contracts/provers/linea/ChildToParentProver.solsrc/contracts/provers/linea/ParentToChildProver.solsrc/contracts/provers/optimism/ChildToParentProver.solsrc/contracts/provers/optimism/ParentToChildProver.solsrc/contracts/provers/scroll/ChildToParentProver.solsrc/contracts/provers/scroll/ParentToChildProver.solsrc/contracts/provers/taiko/ChildToParentProver.solsrc/contracts/provers/taiko/ParentToChildProver.solsrc/contracts/provers/zksync/ChildToParentProver.solsrc/contracts/provers/zksync/ParentToChildProver.sol
2 participants
Across multiple prover implementations, verifyTargetStateCommitment may succeed and return a zero targetStateCommitment when the expected on-chain commitment is missing or has not yet been written.
In the ZKsync ParentToChildProver contract, the function is expected to return the l2LogsRootHash stored in the L1 Gateway contract for a given batch. The verification reads the l2LogsRootHashes mapping using ProverUtils.getSlotFromBlockHeader. However, this helper does not revert when the requested storage slot does not exist or contains the default zero value (e.g., when the batch has not yet been executed on L1). Instead, it returns 0, allowing verifyTargetStateCommitment to return a zero commitment without reverting.
A similar issue exists in ChildToParentProver implementations that rely on buffer contracts to store L1 block hashes on L2 (e.g., Taiko, Linea, Scroll, and ZKsync). In these cases, verifyTargetStateCommitment is intended to return an L1 block hash read from a buffer storage slot. If the block hash has not yet been written, getSlotFromBlockHeader again returns 0, and the function may succeed despite the absence of a valid commitment.
In both scenarios, the behavior deviates from the intended specification. The verifyTargetStateCommitment function is expected to succeed only when a valid, initialized commitment exists. Silently returning zero creates ambiguity between a legitimate value and an uninitialized or missing commitment.
This PR updates the provers to revert if the
targetStateCommitmentis zero on theverifyTargetStateCommitmentfunction.Summary by CodeRabbit
Bug Fixes
Documentation