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

Optimism execution #781

Closed
Wollac opened this issue Oct 6, 2023 · 6 comments
Closed

Optimism execution #781

Wollac opened this issue Oct 6, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@Wollac
Copy link
Contributor

Wollac commented Oct 6, 2023

When I tried to use the Optimism execution behind the optimism feature flag of version v3.5.0, I ran into the following problems:

  • Handler::optimism() is never used. So even if env.cfg.optimism is true, the Optimism specific fee handling is not applied.
  • The Base Fee Vault calculation in the Optimism handler uses the L1 base, but according to the L2 Execution Engine spec, I was under the impression that env.block.basefee * (gas.spend() - gas_refund) should be used instead.
  • When the optimism feature is enabled while env.cfg.optimism is false for Ethereum transactions, the tx_l1_cost is still computed, which can lead to a panic as obviously the enveloped_tx is not provided in this case.

If the above observations are indeed correct and it would be helpful, I can provide a PR for these issues.

@refcell
Copy link
Contributor

refcell commented Oct 7, 2023

Thank you for reporting this! Will take a look, reproduce, and get back to you!

As a side note, if you make a PR, it would be preferable to split it up to make reviewing and adding the relevant unit tests easier.

@rakita
Copy link
Member

rakita commented Oct 8, 2023

@Wollac thank you!

Handler::optimism() should have been used if optimism flag is true here:

handler: Handler::mainnet::<GSPEC>(),

For the future, the idea is to create the RevmBuilder that would allow setting either of the handler (Still need to flush some thing for it), but for now, just changing the code to ::optimism() should do the work.

The Second point I will leave for @refcell, for the third point we definitely should not panic.

@Wollac
Copy link
Contributor Author

Wollac commented Oct 10, 2023

I have created a PR that addresses these issues. It has been validated in zeth(https://github.com/risc0/zeth/tree/feat/revm-optimism) against a handful of Mainnet Ethereum and Optimism blocks. After these changes, blocks can now be constructed that lead to the correct state.

It does not yet include specific unit tests, as @refcell mentioned. None of the affected functions had tests before, so designing relevant tests would require more thought.

@refcell
Copy link
Contributor

refcell commented Oct 10, 2023

Left a couple of nits, but besides those, you're pr looks great! Really exciting that you were able to test this against Optimism Blocks!!

On the topic of unit tests, I think pushing those into other prs is totally reasonable.

@clabby
Copy link
Contributor

clabby commented Oct 10, 2023

Very exciting news that you guys are successfully executing Optimism blocks! Thanks a ton for fixing these up :)

@rakita rakita added the bug Something isn't working label Oct 11, 2023
@Wollac
Copy link
Contributor Author

Wollac commented Oct 13, 2023

Fixed with #789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants