Skip to content

Conversation

kangsorang
Copy link
Contributor

Description

This PR includes modifications to ensure that the current version of Kroma (Cancun and OP Ecotone) is compatible with the L1 Pectra upgrade.
Pair with PR : kroma-network/go-ethereum#150

geoknee and others added 3 commits February 20, 2025 16:02
* add RequestsHash to op-service RPCHeader type

* sources.RPCHeader.RequestsHash needs "requestsHash" json struct tag

not "requestsHash" as in geth

* remove comment

This was fixed upstream ethereum/go-ethereum#30926

* add comment

* Update op-service/sources/types.go

Co-authored-by: protolambda <proto@protolambda.com>

---------

Co-authored-by: protolambda <proto@protolambda.com>
@kangsorang kangsorang requested a review from a team as a code owner February 21, 2025 08:40
Copy link
Contributor

coderabbitai bot commented Feb 21, 2025

Walkthrough

The changes update several functions across Rollup and service packages to include an additional logger parameter. This parameter is now passed along in functions that process transactions or headers, such as dataAndHashesFromTxs, DataFromEVMTransactions, and isValidBatchTx, thereby improving logging during transaction validation and debugging. The tests have been updated accordingly to supply a logger and to switch from the old signer to a new one, as well as to verify that SetCodeTx transactions are ignored. In addition, a new RequestsHash field is added to the header data structure for EIP-7685 compliance.

Changes

File(s) Change Summary
op-node/rollup/.../blob_data_source.go, op-node/rollup/.../calldata_source.go, op-node/rollup/.../data_source.go Updated function signatures to add a logger parameter. In blob_data_source.go, dataAndHashesFromTxs now accepts and passes the logger; in calldata_source.go, DataFromEVMTransactions forwards the logger to isValidBatchTx; in data_source.go, isValidBatchTx includes enhanced validations with logger-based warnings.
op-node/rollup/.../blob_data_source_test.go Test modifications to pass the new logger parameter, switch the transaction signer from CancunSigner to PragueSigner, and add a test case ensuring SetCodeTx transactions are ignored.
op-service/sources/types.go Added a new RequestsHash *common.Hash field to the RPCHeader struct and updated createGethHeader to include this field, reflecting EIP-7685 requirements.

Sequence Diagram(s)

sequenceDiagram
    participant O as "open (BlobDataSource)"
    participant D as "dataAndHashesFromTxs"
    participant V as "isValidBatchTx"
    participant L as "Logger"

    O->>D: Call dataAndHashesFromTxs(txs, config, batcherAddr, logger)
    D->>L: Log start of processing
    D->>V: Validate transaction passing logger
    V->>L: Log warnings/errors during validation
    V-->>D: Return validation result
    D-->>O: Return data and blob hashes
Loading

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7140c8d and 7131e31.

⛔ Files ignored due to path filters (2)
  • go.mod is excluded by !**/*.mod
  • go.sum is excluded by !**/*.sum, !**/*.sum
📒 Files selected for processing (5)
  • op-node/rollup/derive/blob_data_source.go (2 hunks)
  • op-node/rollup/derive/blob_data_source_test.go (5 hunks)
  • op-node/rollup/derive/calldata_source.go (1 hunks)
  • op-node/rollup/derive/data_source.go (1 hunks)
  • op-service/sources/types.go (2 hunks)
🔇 Additional comments (9)
op-node/rollup/derive/calldata_source.go (1)

82-82: LGTM: Logger parameter added to isValidBatchTx call.

The change correctly propagates the logger instance to improve transaction validation logging.

op-node/rollup/derive/data_source.go (3)

98-101: LGTM: Updated transaction type validation.

The function documentation clearly specifies the allowed transaction types (Legacy, ACL, DynamicFee, Blob, or Deposit) for L3s.


113-114: LGTM: Enhanced logging with structured fields.

The logging improvements help with debugging by including transaction hash and submitter address.

Also applies to: 118-119


102-105:

✅ Verification successful

Verify SetCodeTx restriction implementation.

The code correctly disallows SetCodeTx type and future transaction types, which is essential for the L1 Pectra upgrade.

Let's verify there are no unhandled SetCodeTx usages:


🏁 Script executed:

#!/bin/bash
# Search for SetCodeTx usage to ensure proper handling
rg -l "SetCodeTx" | xargs -I {} rg -A 5 "SetCodeTx" {}

Length of output: 393


Verified: SetCodeTx Restriction Properly Implemented

  • The check in op-node/rollup/derive/data_source.go (lines 102-105) correctly disallows any transaction with a type greater than types.BlobTxType (except for deposit transactions), thus effectively preventing SetCodeTx and future types from being processed.
  • The search confirmed that the only occurrences of SetCodeTx are within this disallow check and in a controlled instantiation (likely used for testing), with no unexpected or unhandled usages.

This implementation meets the requirements for the L1 Pectra upgrade.

op-node/rollup/derive/blob_data_source_test.go (2)

28-28: LGTM: Updated test setup for L1 Pectra compatibility.

The changes correctly initialize the logger and update the signer from Cancun to Prague.

Also applies to: 31-31


91-102: LGTM: Added test coverage for SetCodeTx handling.

The new test case properly verifies that SetCodeTx transactions are ignored, which is crucial for L1 Pectra upgrade compatibility.

op-node/rollup/derive/blob_data_source.go (1)

118-124: LGTM: Logger propagation in blob data processing.

The function signature update and logger propagation to isValidBatchTx maintain consistent logging throughout the blob data processing pipeline.

op-service/sources/types.go (2)

160-161: LGTM! Clean implementation of EIP-7685 RequestsHash field.

The field is properly documented and follows the established pattern for optional header fields, with appropriate JSON and RLP tags.


218-219: LGTM! Proper integration of RequestsHash in header creation.

The RequestsHash field is correctly integrated into the header creation process, maintaining consistency with the Prague hardfork changes.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Pangssu Pangssu requested a review from dongchangYoo February 21, 2025 08:41
Copy link

nx-cloud bot commented Feb 21, 2025

View your CI Pipeline Execution ↗ for commit 7131e31.

Command Status Duration Result
nx run-many --target=test ✅ Succeeded 3s View ↗
nx run-many --target=build ✅ Succeeded 2s View ↗

☁️ Nx Cloud last updated this comment at 2025-02-21 08:59:34 UTC

@dongchangYoo
Copy link
Contributor

dongchangYoo commented Feb 21, 2025

The title of this commit doesn’t seem to match its actual changes. Could you clarify what the intended purpose of the commit was? It might help to adjust the title to better reflect the modifications made. @kangsorang

Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kangsorang kangsorang merged commit 4e9b99e into dev Feb 21, 2025
3 checks passed
@kangsorang kangsorang deleted the feat/pectra branch February 21, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants