Fjord: Implement max sequencer drift change to a constant

[sebastianst]

Description

Changes the max sequencer drift from a rollup config parameter to a constant, as part of the upcoming Fjord protocol upgrade.

Tests

I've used the new ChainSpec to implement this change, and added a test for it.

Added two tests to the batch validation and l1 origin selector that it correctly behaves with the Fjord seq drift.

Additional context

Also see specs.

Metadata

• Fixes https://github.com/ethereum-optimism/protocol-quest/issues/205

[coderabbitai]

Walkthrough

Walkthrough

The recent updates streamline the management of maximum sequencer drift by consolidating it within the ChainSpec structure. This change ensures a consistent approach to calculating and applying sequencer drift across different components, simplifying configuration handling and potentially reducing errors in drift management.

Changes

Files	Change Summary
op-e2e/actions/l2_sequencer_test.go op-node/rollup/driver/origin_selector.go op-node/rollup/driver/sequencer.go	Updated to utilize ChainSpec.MaxSequencerDrift for sequencer drift calculation, replacing direct configuration references.
op-e2e/e2eutils/setup.go op-node/rollup/chain_spec.go op-node/rollup/driver/origin_selector.go op-node/rollup/driver/sequencer.go	Added ChainSpec field initialization and integration in various structures and functions.
op-node/rollup/chain_spec.go op-node/rollup/chain_spec_test.go	Introduced maxSequencerDriftFjord constant and included new tests for the MaxSequencerDrift method in ChainSpec.
op-node/rollup/derive/batches.go	No changes in the exported entities, only reformatting and documentation enhancements.
op-node/rollup/types.go	Updated MaxSequencerDrift field to support omitempty for Fjord-activating chains at genesis, emphasizing ChainSpec for drift values.

---

Recent Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8440acf and a9bd54a.

Files selected for processing (1)

• op-node/rollup/derive/batches_test.go (55 hunks)

Additional comments not posted (2)

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

61-69: Validate the initialization values in defaultConf.

Ensure that the values set in the defaultConf function, such as BlockTime, SeqWindowSize, and MaxSequencerDrift, align with the documented or otherwise expected default values for the project.

---

410-426: Examine the handling of sequencer drift with Fjord activation.

This test case introduces Fjord-specific logic. It is crucial to ensure that the implementation correctly handles the sequencer drift in the context of Fjord activation, as this could significantly impact the system's behavior.

Verification successful

This script aims to find any mention of "Fjord" in the codebase to understand its implementation and interactions, and also to inspect the specific test case implementation for handling Fjord activation.

---

The search results provide extensive information about how Fjord activation is handled across various parts of the codebase, particularly in relation to the configuration and operational parameters that change upon its activation. The references to Fjord in the op-node/rollup/derive/batches_test.go file, which includes the test case in question, indicate that Fjord activation is indeed considered in the context of sequencer drift.

The test case "no sequencer time drift on same epoch with non-empty txs and Fjord" specifically modifies the configuration to activate Fjord at a certain time (fjordAt(&l1A.Time)), and this modification is expected to influence the behavior of the system, particularly in how it handles sequencer time drift. The presence of this configuration in the test case confirms that the test is designed to evaluate the system's behavior with Fjord activated, addressing the concerns raised in the review comment.

Given this information, it appears that the test case does indeed handle the scenario of sequencer drift with Fjord activation as intended. Therefore, the review comment's concern seems to be addressed correctly by the existing test implementation.

• The test case explicitly modifies the configuration to activate Fjord, which is a critical aspect in testing the sequencer drift under Fjord conditions.
• The broader search across the codebase shows that Fjord-related configurations and their impacts are well-documented and integrated into various tests and operational scripts, suggesting a thorough consideration of Fjord's effects in different scenarios.

Based on these findings, the handling of sequencer drift with Fjord activation in the test case is verified to be implemented as expected.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the handling of sequencer drift with Fjord activation in the test case "no sequencer time drift on same epoch with non-empty txs and Fjord".

# Test: Check if the Fjord activation logic is correctly implemented and interacts as expected with the sequencer drift settings.
grep -r 'Fjord activation logic' .

Length of output: 36

---

---

Script:

#!/bin/bash
# Description: Broaden the search to find references to Fjord activation and its interaction with sequencer drift settings.

# Search for any mention of "Fjord" in the codebase to understand how it is implemented and interacts with other components.
grep -ri 'Fjord' .

# Additionally, inspect the implementation of the test case mentioned in the review comment to see how Fjord activation is handled.
ast-grep --lang go --pattern $'func($_, $_) {
  $$$
  "no sequencer time drift on same epoch with non-empty txs and Fjord" {
    $$$
  }
  $$$
}'

Length of output: 9983

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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>.
  • 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 generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @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.

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 as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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.

[coderabbitai]

Actionable comments posted: 0

Out of diff range and nitpick comments (2)

op-node/rollup/driver/origin_selector_test.go (1)

180-182: Consider adding a docstring for u64ptr.

Adding a brief docstring to the u64ptr function would improve code readability and maintainability by explaining its purpose and usage.

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

27-29: Consider adding more detailed comments for the fields ExpectedLog and NotExpectedLog to clarify their usage in the context of these tests.

[coderabbitai]

Actionable comments posted: 2

Out of diff range and nitpick comments (2)

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

27-29: Introduce documentation for the ConfigMod field in ValidBatchTestCase.

It would be beneficial to add a comment explaining the purpose of the ConfigMod field in the ValidBatchTestCase struct. This helps maintain clarity and understandability, especially for new contributors or when revisiting the code after some time.

---

Line range hint 124-221: Review the logic for time calculations in L2 block references.

The time calculations for L2 block references (e.g., l2A0.Time + defaultBlockTime) are critical for ensuring the correct sequencing and timing of blocks. It's important to verify that these calculations correctly implement the intended logic, especially in the context of changes related to sequencer drift handling.