Skip to content

op-program: Store created blocks to avoid needing to re-execute them#12382

Merged
ajsutton merged 2 commits intodevelopfrom
aj/exec-once
Oct 10, 2024
Merged

op-program: Store created blocks to avoid needing to re-execute them#12382
ajsutton merged 2 commits intodevelopfrom
aj/exec-once

Conversation

@ajsutton
Copy link
Copy Markdown
Contributor

@ajsutton ajsutton commented Oct 9, 2024

Description

Since op-program always creates a new block via engine_forkChoiceUpdated/engine_getPayload and then immediately sends the same block back to engine_newPayload to be imported, store the created block to the database without updating the chain head, so the engine_newPayload method is a no-op instead of having to execute the transactions again.

The engine is also used in e2e tests so the block caching is optional depending on the provided backend.

Since op-program always creates a new block via engine_forkChoiceUpdated/engine_getPayload and then immediately sends the same block back to engine_newPayload to be imported, store the created block to the database without updating the chain head, so the engine_newPayload method is a no-op instead of having to execute the transactions again.
@ajsutton ajsutton requested review from a team as code owners October 9, 2024 04:50
@ajsutton ajsutton requested review from Inphi, clabby and geoknee and removed request for clabby and geoknee October 9, 2024 04:50
Copy link
Copy Markdown
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

Basically, this ensures that the program enters this if-stmt, right?

Is there a test that already asserts the cache is hit in that case?

@ajsutton
Copy link
Copy Markdown
Contributor Author

ajsutton commented Oct 9, 2024

Test is a good callout. I had originally enforced in in the compiler but then looked at it and wondered why I had those unnecessary changes and undid them so seems like having a test is going to be a better idea. 😆

@ajsutton
Copy link
Copy Markdown
Contributor Author

And yes, that's the if statement we're targeting.

@ajsutton
Copy link
Copy Markdown
Contributor Author

@Inphi that test got much more interesting that I first expected. Have put up a stacked PR to make it easier to review: #12402
I hit some surprising issues with the block hash for the created block not matching what was expected and being rejected. Those actually boiled down to the chain config used in the test being for a non-optimism chain but I've made things more consistent between the geth miner and op-program's block_processor so they do match up and we're less likely to get unexpected surprises in the future.

…12402)

Also fixes differences in block hash for created payloads when using non-op stack chains.
@ajsutton ajsutton requested a review from a team as a code owner October 10, 2024 23:09
@ajsutton ajsutton requested a review from sebastianst October 10, 2024 23:09
@ajsutton ajsutton enabled auto-merge October 10, 2024 23:10
@ajsutton ajsutton added this pull request to the merge queue Oct 10, 2024
Merged via the queue into develop with commit d470c77 Oct 10, 2024
@ajsutton ajsutton deleted the aj/exec-once branch October 10, 2024 23:32
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
…thereum-optimism#12382)

* op-program: Store created blocks to avoid needing to re-execute them.

Since op-program always creates a new block via engine_forkChoiceUpdated/engine_getPayload and then immediately sends the same block back to engine_newPayload to be imported, store the created block to the database without updating the chain head, so the engine_newPayload method is a no-op instead of having to execute the transactions again.

* op-program: Add test that created block is not reprocessed on import (ethereum-optimism#12402)

Also fixes differences in block hash for created payloads when using non-op stack chains.
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.

2 participants