Skip to content

op-node: implement EngineControl interface#4755

Merged
mergify[bot] merged 2 commits intodevelopfrom
engine-controls
Jan 26, 2023
Merged

op-node: implement EngineControl interface#4755
mergify[bot] merged 2 commits intodevelopfrom
engine-controls

Conversation

@protolambda
Copy link
Copy Markdown
Contributor

@protolambda protolambda commented Jan 21, 2023

Description

This introduces the EngineState and EngineControl interfaces, to read and interact with the engine.
The interfaces are minimal & created to avoid external engine state such as forkchoice-state arguments as much as possible.

In the future I would like to split the EngineQueue into two parts:

  • The actual EngineQueue stage with just buffered safe attributes, unsafe payloads, and forkchoice update flag.
  • The Engine, which will implement the engine-control interface, extended with the finality signal function as optional thing.
    Finality signals are otherwise unrelated to the derivation since the input is not stable, and by doing it this way we can keep the engine updated without conflicts.
    • This will also split the largest stage, which is great for testability and readability.

To keep this PR scope small, I just modified the engine queue for initial EngineControl support,
but in the next PRs of this stack I expand on it by adding optional engine metering and cleaning up the sequencer code path by using this.
By using a handle to an EngineControl, the sequener code does not have to maintain its own engine state anymore, and we avoid lots of potential inconsistencies,
as well as making the sequencer a lot more testable by not binding it straight to an Engine-API client.

Minor functional change: a payload result that fails to be converted into a block-ref is now handled with a reset instead of temporary error just in case, because the payload data is static so a temporary error doesn't make sense.

Tests

No notable functionality changes, same engine-queue codepath is already tested.

Full PR stack:

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jan 21, 2023

⚠️ No Changeset found

Latest commit: 563d500

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 21, 2023

Codecov Report

Merging #4755 (563d500) into develop (9be7933) will decrease coverage by 5.30%.
The diff coverage is 6.66%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4755      +/-   ##
===========================================
- Coverage    40.79%   35.50%   -5.30%     
===========================================
  Files          326      179     -147     
  Lines        18498    14421    -4077     
  Branches       768        0     -768     
===========================================
- Hits          7547     5120    -2427     
+ Misses       10388     8773    -1615     
+ Partials       563      528      -35     
Flag Coverage Δ
bedrock-go-tests 35.50% <6.66%> (-0.11%) ⬇️
contracts-bedrock-tests ?
contracts-periphery-tests ?
contracts-tests ?
core-utils-tests ?
dtl-tests ?
fault-detector-tests ?
sdk-tests ?

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

Impacted Files Coverage Δ
op-node/rollup/derive/engine_update.go 0.00% <ø> (ø)
op-node/rollup/derive/pipeline.go 0.00% <0.00%> (ø)
op-node/rollup/derive/engine_queue.go 21.66% <7.46%> (-2.03%) ⬇️
...-periphery/contracts/universal/op-nft/Optimist.sol
...s-bedrock/contracts/governance/GovernanceToken.sol
...rock/contracts/libraries/trie/SecureMerkleTrie.sol
...ntracts/contracts/libraries/codec/Lib_OVMCodec.sol
packages/data-transport-layer/src/utils/eth-tx.ts
packages/fault-detector/src/index.ts
...rock/contracts/universal/OptimismMintableERC20.sol
... and 140 more

Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

lgtm

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 26, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 26, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot merged commit 7abde92 into develop Jan 26, 2023
@mergify mergify bot deleted the engine-controls branch January 26, 2023 01:26
@mergify mergify bot removed the on-merge-train label Jan 26, 2023
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.

3 participants