feat(rollup-verifier): make withdraw root check optional #1182
Conversation
WalkthroughThe changes increment the patch version in the version constants and update the handling of withdraw root during rollup batch synchronization. Withdraw root retrieval is now optional, missing roots are tolerated, and finalized batch metadata uses the L1 event's withdraw root. No other logic or exported entity changes were made. Changes
Sequence Diagram(s)sequenceDiagram
participant RollupSyncService
participant LocalNode
participant L1Event
RollupSyncService->>LocalNode: Fetch block state for withdraw root
alt State available
LocalNode-->>RollupSyncService: Return withdraw root
RollupSyncService->>RollupSyncService: Compare local withdraw root to L1 event (if not empty)
else State unavailable
LocalNode-->>RollupSyncService: No state (withdraw root missing)
RollupSyncService->>RollupSyncService: Log trace, skip withdraw root check
end
RollupSyncService->>L1Event: Use L1 event's withdraw root for finalized batch metadata
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rollup/rollup_sync_service/rollup_sync_service.go (3)
401-410: Heavy per-blockStateAtcalls can hurt performance
StateAtreconstructs a fullStateDBfrom the trie; executing it once per block in every chunk is O(n × state-root-lookup) and can become a CPU / IO bottleneck when finalising large bundles.- state, err := s.bc.StateAt(block.Root()) + // PERF: We only need the withdraw root of the *end* block for + // validation. Consider limiting the expensive StateAt call to the + // last block of the batch or caching results across iterations. + state, err := s.bc.StateAt(block.Root())Even if you keep the current semantics, adding a TODO and a fast-path cache for identical
block.Root()values would give an easy win.
403-409: Differentiate between “state pruned” and “unexpected error”All
StateAterrors are treated the same. A genuine DB corruption would be silently ignored just like a harmless “missing trie node”, making debugging hard.Consider:
- log.Trace("State is not available, skipping withdraw trie validation", ... + if errors.Is(err, core.MissingTrieNodeError) { // pseudo‐code + log.Trace("Historical state pruned – skipping withdraw-root check", + "blockNumber", block.NumberU64(), "err", err) + } else { + log.Warn("State lookup failed – withdraw-root check skipped!", + "blockNumber", block.NumberU64(), "err", err) + }This keeps the relaxed behaviour for pruned nodes while surfacing unexpected failures.
581-584: Use a named zero hash for readabilityMinor style nit: comparing to a literal
common.Hash{}is harder to read than using a clearly named variable.- if localWithdrawRoot != (common.Hash{}) && localWithdrawRoot != event.WithdrawRoot() { + var zeroHash common.Hash + if localWithdrawRoot != zeroHash && localWithdrawRoot != event.WithdrawRoot() {This also avoids allocating a new empty struct on every comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
params/version.go(1 hunks)rollup/rollup_sync_service/rollup_sync_service.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/rollup_sync_service/rollup_sync_service.go (2)
rollup/withdrawtrie/withdraw_trie.go (1)
ReadWTRSlot(16-18)rollup/rcfg/config.go (1)
L2MessageQueueAddress(16-16)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
params/version.go (1)
27-29: LGTM – version bump onlyPatch version increment to
44looks correct and is consistent with the new feature flag. No additional action needed.rollup/rollup_sync_service/rollup_sync_service.go (1)
612-613: Storing L1 withdraw root may surprise downstream consumers
finalizedBatchMeta.WithdrawRootis now always taken from the L1 event, even when a local root was available and verified.
If any downstream logic expects the value to reflect the local chain (e.g., for auditing), this silent switch could be confusing.Please double-check all readers of
FinalizedBatchMeta.WithdrawRootto ensure they do not rely on it containing the local value.
1. Purpose or design rationale of this PR
Make withdraw trie root verification optional, so that rollup-verifier works on full nodes with pruned historical states.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Chores
Bug Fixes