feat(CodecV7): add CodecV7 to support upgrade 5.2 Euclid phase2 #44
Conversation
…n CodecFromConfig
…1 message hash from a given set of blocks
Conflicts: encoding/da.go encoding/interfaces.go go.mod go.sum
* fix: clear 32 bits instead of 36 * fix: test expectations for rolling hash * fix: tests * fix tests --------- Co-authored-by: jonastheis <[email protected]>
Co-authored-by: colin <[email protected]>
…MessageQueueHash -> postL1MessageQueueHash
WalkthroughThe changes introduce a new codec version, CodecV7, along with comprehensive implementations for encoding, decoding, and batch processing. New methods were added to both the existing DACodecV0 and the new DACodecV7. Enhancements include blob decoding, batch creation, and extensive L1 message queue handling with rolling hash computation. Additionally, corresponding unit tests and updated interfaces were provided, and a dependency version was updated in go.mod. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as CodecFromVersion
participant V7 as DACodecV7
C->>F: Request codec instance (e.g., CodecV7)
F-->>C: Returns DACodecV7 instance
C->>V7: Call DecodeBlob(blob) / NewDABatchFromParams(...)
V7->>V7: Process blob/batch creation internally
V7-->>C: Return DABlobPayload/DABatch and error
sequenceDiagram
participant B as Block
participant MQ as MessageQueue Functions
B->>MQ: Call NumL1MessagesNoSkipping()
MQ-->>B: Return count and indices
B->>MQ: For each L1 message, call MessageQueueV2ApplyL1Message
MQ->>MQ: Update rolling hash per message
MQ->>MQ: Finalize hash with messageQueueV2EncodeRollingHash
MQ-->>B: Return updated L1 message queue hash
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
encoding/codecv7.go (3)
41-71: Check for chunk concept edge cases.
NewDAChunkis implemented despite “no notion of chunks” in V7. Ensure future refactors or merges do not introduce conflicts or confusion in chunk-based code paths.
334-346: Address the gas calculation TODO.
The comment indicates gas cost is overestimated. Revisit and refine once the contract’s final gas usage is known, to ensure accurate L1 commit gas predictions.Would you like me to open an issue to track completion of this TODO?
348-361: Ensure consistent JSON encoding for large batches.
JSONFromBytescan produce large JSON payloads for bigger batches. Consider adding or confirming streaming/partial rendering is not needed for extremely large data sets.encoding/codecv7_types.go (2)
141-165: Validate potential double hashing logic.
challengeDigestuses nested keccak calls, which is correct for some commitments but can introduce confusion. Document or assert this usage to ensure future maintainers understand the intended cryptographic flow.
481-500: Add defensive checks on appended magic bytes.
decompressV7BytesprependszstdMagicNumberto incoming data before decompression. Ensure the incoming data does not already contain these bytes or conflict with other compression wrappers.encoding/da.go (2)
112-114: Document new fields for chunk-based L1 message queue hashes.
PrevL1MessageQueueHashandPostL1MessageQueueHashare newly introduced. Consider adding doc comments or references showing how they integrate with the rest of the system for clarity.
124-127: Confirm block vs. chunk usage in Codec V7.
TheBatchstruct now has bothChunksandBlocks. In Codec V7, blocks are processed directly, but references toChunksremain. Ensure no confusion or partial usage arises down the line.encoding/codecv7_test.go (2)
199-363: Good validation of blob encoding and blob-versioned hash correctness.
The approach systematically covers edge cases involving empty batches and single/multiple blocks. Consider verifying that large block sets don’t degrade performance excessively in production environments.
898-966: Helper functions appear consistent and clear.
However,generateRandomDatareuses a fixed seed, resulting in deterministic output each time. If needed, consider randomizing the seed or allowing an override for truly random tests.encoding/interfaces.go (1)
60-64: New methods in the Codec interface.
NewDABatchFromParamsandDecodeBlobimprove extensibility for new batch creation and blob decoding. Ensure thorough usage in existing code paths to detect potential integration gaps.encoding/da_test.go (1)
154-224: LGTM! Consider adding edge cases.The test is well-structured using table-driven testing and covers essential scenarios. The test cases effectively verify that the last 4 bytes are zeroed out in the rolling hash computation.
Consider adding edge cases to improve test coverage:
testCases := []struct { name string input common.Hash expectedOutput common.Hash }{ + { + "single bit set in last 4 bytes", + common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000001"), + common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000000"), + }, + { + "single bit set in first 4 bytes", + common.HexToHash("0x0100000000000000000000000000000000000000000000000000000000000000"), + common.HexToHash("0x0100000000000000000000000000000000000000000000000000000000000000"), + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
encoding/codecv0.go(2 hunks)encoding/codecv7.go(1 hunks)encoding/codecv7_test.go(1 hunks)encoding/codecv7_types.go(1 hunks)encoding/da.go(10 hunks)encoding/da_test.go(3 hunks)encoding/interfaces.go(5 hunks)encoding/interfaces_test.go(2 hunks)go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (4)
encoding/interfaces.go (1)
Learnt from: colinlyguo
PR: scroll-tech/da-codec#25
File: encoding/interfaces.go:95-108
Timestamp: 2024-11-12T12:17:31.140Z
Learning: In the `CodecFromConfig` function in the Go `encoding/interfaces.go` file, if none of the chain configuration conditions match, it's acceptable to default to returning `&DACodecV0{}` because, in the current logic, we can only deduce the codec version as the function implements, and the logic is complete.
encoding/codecv7.go (1)
Learnt from: colinlyguo
PR: scroll-tech/da-codec#25
File: encoding/codecv1_types.go:105-116
Timestamp: 2024-11-12T12:17:31.140Z
Learning: The code in `encoding/codecv1_types.go`, specifically the `Encode` method in `daBatchV1`, has been updated. Previous comments regarding hardcoded byte offsets may be outdated.
encoding/codecv0.go (2)
Learnt from: colinlyguo
PR: scroll-tech/da-codec#25
File: encoding/codecv0.go:387-401
Timestamp: 2024-11-12T12:17:31.140Z
Learning: In `DACodecV0`, methods like `EstimateChunkL1CommitBatchSizeAndBlobSize`, `EstimateBatchL1CommitBatchSizeAndBlobSize`, and `JSONFromBytes` are intentionally left as no-ops (returning zero or nil) to maintain a consistent interface across codecs and prevent the caller from needing conditional logic.
Learnt from: colinlyguo
PR: scroll-tech/da-codec#25
File: encoding/codecv1_types.go:152-154
Timestamp: 2024-11-12T12:17:31.140Z
Learning: In the `daBatchV1` struct, the `BlobBytes()` method is intentionally returning `nil`.
encoding/codecv7_types.go (1)
Learnt from: colinlyguo
PR: scroll-tech/da-codec#25
File: encoding/codecv1_types.go:105-116
Timestamp: 2024-11-12T12:17:31.140Z
Learning: The code in `encoding/codecv1_types.go`, specifically the `Encode` method in `daBatchV1`, has been updated. Previous comments regarding hardcoded byte offsets may be outdated.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (20)
encoding/codecv7.go (2)
27-29: Consider imposing a practical upper limit on batch chunks.
Returningmath.MaxIntas the maximum number of chunks may pose a risk of resource exhaustion in some scenarios. Evaluate whether a smaller limit or validation logic is necessary to avoid creating impractically large batches.
73-94: Validate that all blocks are processed correctly.
Although checks likecheckBlocksBatchVSChunksConsistencyandconstructBlobare performed, you might consider adding dedicated unit tests to confirm that each block's data is fully represented in the constructed blob.encoding/codecv7_types.go (1)
254-263: Confirm skipping only L1MessageTx is desired.
InEncode, you skip adding L1MessageTx totransactionBytes. Verify that this is the correct logic for all use cases where only L2 transactions should be embedded in the payload.encoding/da.go (3)
147-185: Consider caching or indexing.
NumL1MessagesNoSkipping()re-scans transactions each time. If this method is called repeatedly in performance-sensitive code, caching or a precomputed index might help.
[performance]
700-703: Double-check Euclid/EUCLIDv2 boundaries.
GetHardforkNamereturns "euclid" if!config.IsEuclidV2(blockTimestamp), else "euclidV2". Confirm that there are no off-by-one or timing boundary conditions, especially around transitional epochs.
773-800: Validate L1 message sequences in multi-block scenarios.
MessageQueueV2ApplyL1MessagesFromBlocksmight handle multiple blocks with varying L1MessageTx entries. Confirm that all queue indices remain consecutive across block boundaries under “no skipping” assumptions.encoding/codecv7_test.go (7)
20-105: Well-structured table-driven tests for block encoding/decoding.
The coverage of various scenarios (empty blocks, multiple L1 messages, skipped indices) is comprehensive. Consider adding negative tests (e.g., malformed block data) to further validate error handling.
107-197: Robust checks for batch initialization and encodings.
The creation error tests (e.g., "L1 messages not consecutive") ensure correctness of batch logic. Add coverage for invalid block references (e.g., nil blocks) if needed.
365-504: Compression logic adequately tested.
You perform multiple scenarios (e.g., single block, multiple blocks, full-blob random data). Since some scenarios generate thousands of blocks, confirm test execution time remains manageable.
506-648: Disabling compression scenario is well-covered.
Similar to the enable-compression tests, the thorough approach for edge cases (max-size data, random repeated data) is commendable.
650-736: Clear testing of compressed data compatibility checks.
The table-driven approach for Blocks 02, 03, 04, 05, 06, 07 is solid. The fallback to error conditions on unexpected queue indices is consistent with the main code.
738-807: JSON marshaling/unmarshaling test coverage.
Verifying field-by-field equality ensures that structural changes won't silently break serialization.
809-896: Proof generation for point evaluation is well-validated.
The test cases effectively confirm that varying block contexts yield the expected proof.encoding/interfaces_test.go (2)
24-26: Extended codec version checks.
Ensuring recognition of CodecV5, CodecV6, and CodecV7 aligns the tests with the new expansions.
51-79: New Euclid/EuclidV2 test paths.
The test ensuresCodecFromConfigselectsDACodecV7orDACodecV6appropriately. This is critical for verifying correct codec selection at runtime.encoding/interfaces.go (2)
44-49: New DABlobPayload interface.
Exposing blocks, transactions, and queue-hash accessors fosters consistent decoding logic. Confirm that the interface behaviors remain stable if future fields are added.
91-122: CodecV7 introduction in version-switching logic.
Properly integrated inCodecFromVersionandCodecFromConfig, ensuring thatIsEuclidV2triggers the new codec. The fallback ordering is consistent with the code base’s multi-upgrade pattern.encoding/da_test.go (1)
22-23: LGTM!The changes correctly capture and propagate the test suite's exit code.
encoding/codecv0.go (2)
164-166: LGTM!The function is intentionally left as a no-op to maintain a consistent interface across codecs, which aligns with the existing pattern in
DACodecV0.
230-232: LGTM!The function is intentionally left as a no-op to maintain a consistent interface across codecs, which aligns with the existing pattern in
DACodecV0.
| func messageQueueV2EncodeRollingHash(rollingHash common.Hash) common.Hash { | ||
| // clear last 32 bits, i.e. 4 bytes. | ||
| rollingHash[28] = 0 | ||
| rollingHash[29] = 0 | ||
| rollingHash[30] = 0 | ||
| rollingHash[31] = 0 | ||
|
|
||
| return rollingHash | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential collision risk with partial zeroing.
messageQueueV2EncodeRollingHash clears the last 4 bytes of the rolling hash. If collisions become problematic, consider a stronger approach or additional domain separation.
Cannot merge #33 due to GitHub bug (require conversation resolution, but no unresolved conversation). Opened a new PR. See previous discussion and approval on #33.
Summary by CodeRabbit
New Features
Tests
Chores