op-node: Add syncmode flag and remove old snap sync flag

[trianglesphere]

Description

This PR adds a new syncmode flag and removes the old snap sync flag (though setting the old snap sync flag will still set the correct syncmode). This also rejiggers some of the configuration.

Tests

No tests. Should be cosmetic changes.

Additional context

First step (of many) to rolling out snap sync.

@coderabbitai ignore

[coderabbitai]

Walkthrough

The changes encompass a fundamental shift in synchronization strategy within an Ethereum Optimism node, transitioning from P2P sync to EL (Execution Layer) sync. A new SyncMode configuration has been introduced, deprecating the old EngineSync flag. This update affects error handling, logging, and conditional checks across various components, ensuring that the system now operates with the new sync mode paradigm.

Changes

File Path	Change Summary
op-e2e/actions/l2_verifier.go op-e2e/actions/sync_test.go	Error comparison changed from derive.EngineP2PSyncing to derive.EngineELSyncing; sync config updated to use SyncMode.
op-node/flags/flags.go op-node/service.go	Added SyncModeFlag, deprecated L2EngineSyncEnabled, and updated sync config creation with error handling and logging.
op-node/rollup/derive/... (multiple files)	Updated error handling and conditional checks to align with the new SyncMode configuration.
op-node/rollup/sync/config.go	Introduced Mode type for sync modes, added methods for mode handling, and updated Config struct.

"In the node's heart, a change does beat,
Syncing now with EL's discreet.
Flags retire, with modes anew,
Optimism's sync, refined and true." 🐇✨

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• 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/coderabbit-overrides.v2.json

[coderabbitai]

Review Status

Actionable comments generated: 5

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e624c63 and 3566a06.

Files selected for processing (9)

• op-e2e/actions/l2_verifier.go (1 hunks)
• op-e2e/actions/sync_test.go (1 hunks)
• op-node/flags/flags.go (3 hunks)
• op-node/rollup/derive/engine_queue.go (4 hunks)
• op-node/rollup/derive/error.go (1 hunks)
• op-node/rollup/derive/pipeline.go (1 hunks)
• op-node/rollup/driver/state.go (1 hunks)
• op-node/rollup/sync/config.go (1 hunks)
• op-node/service.go (3 hunks)

Additional comments: 17

op-e2e/actions/l2_verifier.go (1)

• 217-222: The change from derive.EngineP2PSyncing to derive.EngineELSyncing is consistent with the summary and the pull request's goal to update synchronization terminology. The logic to handle the error seems correct, as it sets l2PipelineIdle to true and returns early if the error is io.EOF or derive.EngineELSyncing.

op-e2e/actions/sync_test.go (1)

• 174-174: The code correctly reflects the changes described in the summary, updating the sync.Config to use the new SyncMode field with the ELSync value.

op-node/flags/flags.go (4)

• 9-9: The import of the sync package is correctly added to support the new SyncModeFlag.

• 49-58: The SyncModeFlag is added as an optional flag and is marked as hidden, which is appropriate if the feature is still in development. However, ensure that this flag is made visible once the feature is ready for use.

• 243-250: The L2EngineSyncEnabled flag is deprecated with a clear usage warning. Ensure that documentation and migration guides are updated to instruct users on how to transition to the new SyncModeFlag.

• 46-61: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-251]

No further issues found in the rest of the file. All other flags and configurations appear to be unchanged and unrelated to the new SyncModeFlag.

op-node/rollup/derive/engine_queue.go (4)

• 267-271: The change from EngineP2PSyncing to EngineELSyncing aligns with the new synchronization terminology and logic.

• 458-465: The updated conditional logic in checkNewPayloadStatus to handle the new sync.ELSync mode is consistent with the changes described in the summary.

• 468-474: The updated conditional logic in checkForkchoiceUpdatedStatus to handle the new sync.ELSync mode is consistent with the changes described in the summary.

• 490-496: The updated conditional logic in tryNextUnsafePayload to handle the sync.CLSync mode is consistent with the changes described in the summary.

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

• 100-101: Ensure that all references to the old EngineP2PSyncing error have been updated to EngineELSyncing throughout the entire codebase to maintain consistency.

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

• 327-330: The change from derive.EngineP2PSyncing to derive.EngineELSyncing aligns with the new synchronization terminology and logic. This update should be cross-checked with all other parts of the codebase to ensure consistency.

op-node/rollup/sync/config.go (2)

• 5-54: The implementation of the Mode type, its methods, and the ValidMode function are well-defined and follow Go conventions. The comments provide good context for the internal naming conventions.

• 46-54: The Config struct is updated to include the SyncMode field, and the comments provide clear explanations of the sync modes.

op-node/service.go (3)

• 3-9: The addition of the errors package is appropriate for the error handling in NewSyncConfig.

• 250-265: The changes to NewSyncConfig correctly handle the deprecation of --l2.engine-sync and the conflict with --syncmode. Ensure that the rest of the codebase and documentation are updated to reflect these changes.

• 256-262: The initialization of sync.Config correctly sets the SyncMode based on the --syncmode flag or the deprecated --l2.engine-sync. It's important to ensure that the sync.Config struct has been updated accordingly in sync/config.go to include the SyncMode field of the new Mode type.

[codecov]

Codecov Report

Merging #8333 (e08bd5b) into develop (99cc9a7) will decrease coverage by 19.01%.
Report is 82 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #8333       +/-   ##
============================================
- Coverage    40.88%   21.87%   -19.01%     
============================================
  Files          163       87       -76     
  Lines         6054     1938     -4116     
  Branches       970      438      -532     
============================================
- Hits          2475      424     -2051     
+ Misses        3503     1484     -2019     
+ Partials        76       30       -46     

Flag	Coverage Δ
cannon-go-tests	?
chain-mon-tests	?
common-ts-tests	?
contracts-bedrock-tests	21.87% <ø> (+0.05%)	⬆️
contracts-ts-tests	?
core-utils-tests	?
sdk-next-tests	?
sdk-tests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 77 files with indirect coverage changes

[ajsutton]

Looks good generally, but I think we need to avoid using the name snap sync since it doesn't necessarily use snap sync - just allows the EL to sync. On L1 that's an optimistic sync but not sure that fits as well here.

Or it potentially could be a checkpoint sync given validation will start from a specific checkpoint (though I don't think we've actually implemented that yet).

Maybe strict or fast sync modes? Or l1 and p2p/engine/el?

[trianglesphere]

@coderabbitai pause

[sebastianst]

LGTM besides a few minor remarks