Skip to content
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

op-node: extract unsafe-block processing from derivation code-path #10599

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

protolambda
Copy link
Contributor

Description

Extracts the unsafe-block processing code-path from the EngineQueue and encapsulates it in a new clsync package. It buffers unsafe blocks, and applies as necessary, for step-wise CL sync.
The EL-sync is triggered as before still, upon new payloads received from gossip, not using the CL sync code-path.

This PR depends on #10580

Marked as do-not-merge until dependency is merged into develop.

This is the second step towards removal of the EngineQueue in favor of a more encapsulated pure derivation pipeline.

Tests

Adds missing unit tests: previously only the act of dropping an older unsafe block was tested.
Now it tests all combinations of existing unsafe head vs new unsafe payloads.

Metadata

Fix https://github.com/ethereum-optimism/protocol-quest/issues/270

@protolambda protolambda added the M-do-not-merge Meta: Do not merge label May 21, 2024
@protolambda protolambda requested review from trianglesphere, ajsutton and a team as code owners May 21, 2024 13:50
Copy link
Contributor

semgrep-app bot commented May 21, 2024

Semgrep found 2 golang_fmt_errorf_no_params findings:

No fmt.Errorf invocations without fmt arguments allowed

Ignore this finding from golang_fmt_errorf_no_params.

Copy link
Contributor

semgrep-app bot commented May 24, 2024

Semgrep found 2 invalid-usage-of-modified-variable findings:

Variable unsafeInNode is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Ignore this finding from invalid-usage-of-modified-variable.

Semgrep found 1 todos_require_linear finding:

  • packages/core-utils/src/common/bn.ts

Please create a GitHub ticket for this TODO.

Ignore this finding from todos_require_linear.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. Really just Boy Scout, make the place a little better suggestions.

@protolambda protolambda force-pushed the unsafe-blocks-refactor branch from 06e3ba3 to 113cde7 Compare May 30, 2024 18:55
@protolambda
Copy link
Contributor Author

Rebased on new finality-refactor base, which was rebased on develop

Base automatically changed from finality-refactor to develop June 3, 2024 16:12
@protolambda protolambda force-pushed the unsafe-blocks-refactor branch from 113cde7 to 22c10ae Compare June 3, 2024 16:43
@protolambda protolambda removed the M-do-not-merge Meta: Do not merge label Jun 3, 2024
@protolambda
Copy link
Contributor Author

Rebased on develop, dependency PR landed.

@protolambda protolambda enabled auto-merge June 3, 2024 16:45
@protolambda protolambda added this pull request to the merge queue Jun 3, 2024
Merged via the queue into develop with commit 9c09648 Jun 3, 2024
64 checks passed
@protolambda protolambda deleted the unsafe-blocks-refactor branch June 3, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants