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: cl-sync using events #10903

Merged
merged 1 commit into from
Jun 19, 2024
Merged

op-node: cl-sync using events #10903

merged 1 commit into from
Jun 19, 2024

Conversation

protolambda
Copy link
Contributor

Description

This is a step closer to making each driver-component communicate with events, and not attach synchronously to the engine-controller.

The CL-sync now monitors forkchoice updates and invalid-payload reports to maintain what payload should be suggested to the execution engine next.
It no longer directly interfaces with the engine-controller state.

The CL-sync checks are also robust enough to just drop payloads if the forkchoice moves past these at a later point. I.e. the node does not get stuck when a forkchoice-update event is missed.

Temporary errors are assumed to not bubble back any useful events, and instead the CL-sync waits for the next forkchoice-update to re-attempt.

TODO: should we poke it more explicitly after a temporary error, to not rely on new incoming blocks for the retry attempt?
Going to draft stacked PR on top of this first, the engine-controller really needs some simplification/improvement/testing.

To handle the "dead" moment when there's a new payload, but no forkchoice-updates to trigger processing, the CL-sync can request from the engine to communicate the latest forkchoice. That way no state is shared, and the CL-sync can continue processing.

Note: the engine-controller is enhanced with event-emits of forkchoice changes and payload-insertion (in the full payload case, not when building a payload from block-attributes).

Side-note: also moved the syncronous-events-emitter from driver into rollup package, but did not end up needing it for cl-sync unit-testing. Added a mock-emitter instead, to catch/check emitted events.

Tests

  • updated CL-sync unit tests
  • updated op-e2e action-test verifier to make cl-sync a deriver.
  • re-enabled op-e2e system gossip test to confirm that block gossip sync still works.

Metadata

Fix #10853

Copy link
Contributor

semgrep-app bot commented Jun 18, 2024

Semgrep found 1 sol-style-return-arg-fmt finding:

  • packages/contracts-bedrock/src/cannon/libraries/MIPSInstructions.sol

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

Semgrep found 1 sol-style-malformed-revert finding:

  • packages/contracts-bedrock/src/cannon/MIPS.sol

Malformed revert statement style.

Ignore this finding from sol-style-malformed-revert.

Semgrep found 1 sol-style-input-arg-fmt finding:

  • packages/contracts-bedrock/src/cannon/libraries/MIPSInstructions.sol

Inputs to functions must be prepended with an underscore (_)

Ignore this finding from sol-style-input-arg-fmt.

@protolambda protolambda marked this pull request as ready for review June 18, 2024 17:38
@protolambda protolambda requested review from a team and ajsutton as code owners June 18, 2024 17:38
@protolambda
Copy link
Contributor Author

Todo: upon the temporary-error event we may want to request a forkchoice-update, so the CL-sync doesn't get stuck until the next payload/forkchoice update, to perform the retry of a temporarily failed payload.

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. Just probably should leave TestSystemMockP2P skipped if we haven't actually fixed the cause of its flakiness. Definitely good to have identified and run it manually though to confirm these changes.

op-e2e/system_test.go Show resolved Hide resolved
op-node/rollup/clsync/clsync.go Show resolved Hide resolved
@protolambda protolambda added this pull request to the merge queue Jun 19, 2024
Merged via the queue into develop with commit ea4d1fd Jun 19, 2024
56 of 57 checks passed
@protolambda protolambda deleted the cl-sync-events branch June 19, 2024 20:25
rdovgan pushed a commit to rdovgan/optimism that referenced this pull request Jun 24, 2024
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.

Interop: Event-handling update of CLSync
2 participants