-
Notifications
You must be signed in to change notification settings - Fork 22
feat: consider RLE blocks in compatibility check starting from V9 #67
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
* fix compability * fmt
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
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.
Pull Request Overview
This PR enables support for RLE (Run-Length Encoding) blocks in the compressed data compatibility check for codec version 9. The change introduces a new compatibility check function checkCompressedDataCompatibilityV9 that handles RLE block types, which were previously unsupported.
Key changes:
- Added
checkCompressedDataCompatibilityV9function that properly handles RLE blocks (type 1) in addition to compressed blocks - Created
DACodecV9struct that overrides all methods transitively calling the compatibility check to use the V9 version - The V9 codec now accepts both RLE blocks and compressed blocks (types 1 and 2), rejecting only reserved block type 3
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| encoding/da.go | Adds new checkCompressedDataCompatibilityV9 function with RLE block support |
| encoding/codecv9.go | Implements DACodecV9 struct with overridden methods to use V9 compatibility check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jonastheis
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.
Looks good to me (checked with a diff to CodecV8).
Just to clarify:
Once Galileo activates we start creating batches with CodecV9 and potentially with RLE blocks. This is fine as l2geth will then (after Galileo) recreate the same batches with RLE blocks. Compression is unmodified and should work in either case.
Co-authored-by: Copilot <[email protected]>
* add galileo CodecV9 * add missing switch cases and tests * feat: consider RLE blocks in compatibility check starting from V9 (#67) * fix: consider RLE blocks in zstd compatibility check (#64) * fix compability * fmt * override behavior * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Ho <[email protected]> Co-authored-by: georgehao <[email protected]> Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Péter Garamvölgyi <[email protected]> Co-authored-by: Ho <[email protected]> Co-authored-by: Copilot <[email protected]>
Purpose or design rationale of this PR
Enable #64 starting from codecV9.
See also discussion.
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:
Breaking change label
Does this PR have the
breaking-changelabel?