-
Notifications
You must be signed in to change notification settings - Fork 284
feat(L1 follower and rollup verifier): support CodecV7 in L1 follower mode and rollup verifier #1105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…retrieve data from L1
…s submitted in one transaction
…single transactions
WalkthroughThis pull request introduces versioning and enhanced error handling for committed batch metadata within the rollup event processing system. It adds new fields and types (e.g., Version, ChunkBlockRanges, LastL1MessageQueueHash) to distinguish between legacy (V0) and new (V7) metadata. The changes update encoding/decoding routines and test cases accordingly and modify commit batch functions (V1 and V7) to better handle blob hashes and block timestamps. Additionally, dependency versions have been updated, and rollup synchronization logic now validates batches with additional parent metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RCM as ReadCommittedBatchMeta
participant DecV7 as Decoder (V7)
participant DecV0 as Decoder (V0)
Caller->>RCM: Request batch metadata read
RCM->>DecV7: Attempt V7 decode
alt V7 decode successful
DecV7-->>RCM: Return V7 metadata
else V7 decode fails
RCM->>DecV0: Attempt V0 decode
DecV0-->>RCM: Return V0 metadata or error
end
RCM-->>Caller: Return metadata (or error)
sequenceDiagram
participant ES as Rollup Event Stream
participant CBS as CalldataBlobSource
participant Helper as getAndAppendCommitBatchDA
ES->>CBS: New rollup event arrives
CBS->>CBS: Check commit transaction hash
alt Transaction changed
CBS->>Helper: Process and append previous commit events
Helper-->>CBS: Return aggregated commit data
else Same transaction
CBS->>CBS: Append commit event to current batch
end
CBS-->>ES: Return processed batch data
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
…-use-code-from-l1-follower
…1-follower-mode-update-da-codec
…atch within a set of batches
colinlyguo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
…erifier-use-code-from-l1-follower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/rawdb/accessors_rollup_event.go (2)
196-207: Enhance version validation.While the version check exists, consider adding an upper bound check to ensure the version is not higher than the maximum supported version.
if encoding.CodecVersion(cbm7.Version) < encoding.CodecV7 { return nil, fmt.Errorf("unexpected committed batch metadata version: batch index %d, version %d", batchIndex, cbm7.Version) } +if encoding.CodecVersion(cbm7.Version) > encoding.MaxCodecVersion { + return nil, fmt.Errorf("unsupported committed batch metadata version: batch index %d, version %d", batchIndex, cbm7.Version) +}
209-219: Consider adding version validation for V0.Similar to V7 validation, consider adding version validation for V0 to ensure data integrity.
if err = rlp.Decode(bytes.NewReader(data), cbm0); err != nil { return nil, fmt.Errorf("failed to decode committed batch metadata: batch index %d, err: %w", batchIndex, err) } +if encoding.CodecVersion(cbm0.Version) >= encoding.CodecV7 { + return nil, fmt.Errorf("unexpected version for V0 format: batch index %d, version %d", batchIndex, cbm0.Version) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/rawdb/accessors_rollup_event.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
core/rawdb/accessors_rollup_event.go (4)
23-29: LGTM! Well-structured metadata versioning.The
CommittedBatchMetastruct is well-designed with clear versioning support and backward compatibility considerations. The comment for the new field clearly indicates its version requirement.
31-36: LGTM! Good backward compatibility.The
committedBatchMetaV0struct maintains backward compatibility while clearly documenting the unused field with a helpful comment.
162-184: LGTM! Clean version-based encoding.The function elegantly handles different versions of metadata while maintaining backward compatibility. The error handling is robust and the version check is properly implemented.
187-194: LGTM! Improved error handling.Good improvement in error handling by returning errors instead of using
log.Crit. This allows better error recovery in the calling code.
6d5af23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/da_syncer/da/calldata_blob_source.go (1)
234-259: Consider usingdefaultfor CodecV7 and above.Based on a previous review comment, using
defaultinstead of an explicit case for V7 would make it easier to handle future ZK bug fixes that might require a new verifier version without changing the batch format.Apply this diff to improve future maintainability:
- default: // CodecVersion 7 and above + case 7:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/da_syncer/da/calldata_blob_source.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (5)
rollup/da_syncer/da/calldata_blob_source.go (5)
11-11: LGTM!The addition of the
commonpackage import is necessary for using theHashtype in the new implementation.
99-121: LGTM! Well-structured implementation for batch processing.The implementation effectively tracks and processes commit events from the same transaction together, with clear comments explaining the logic. The helper function
getAndAppendCommitBatchDAprovides a clean way to process batched events.
134-144: LGTM! Robust handling of commit events.The code correctly identifies and processes commit events from different transactions while maintaining the order of events. The implementation aligns with the comment from the previous review about handling multiple commit batch events belonging to the same transaction.
145-186: LGTM! Consistent cleanup of pending commit events.The code ensures that any pending commit events are processed before handling revert or finalize events, maintaining data consistency.
191-233: LGTM! Thorough validation of commit events.The implementation includes comprehensive validation of commit events, ensuring:
- Non-empty commit events list
- Matching transaction hashes
- Matching block numbers and hashes
- Sequential batch indices
ca408bc
1. Purpose or design rationale of this PR
This PR introduces
CodecV7for theEuclidV2hardfork to the L1 follower mode and rollup verifier. Both components reuse the same underlyingCalldataBlobSource. Additionally, the rollup verifier is modified to support the new batch structure when verifying batches locally vs received from L1.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
New Features
Refactor
Tests
Chore