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

fix: Optimism execution #789

Merged
merged 6 commits into from Oct 12, 2023
Merged

Conversation

Wollac
Copy link
Contributor

@Wollac Wollac commented Oct 10, 2023

This PR fixes the issues with the Optimism execution described in #781:

  • Handler::optimism() is used instead of Handler::mainnet() when the optimism feature is enabled and env.cfg.optimism is true.
  • The base fee of a transaction is now calculated using env.block.basefee * (gas.spend() - gas_refund).
  • The panic when executing Ethereum transactions has been fixed by calculating tx_l1_cost only when env.cfg.optimism is true and the corresponding transaction is not an Optimism deposit transaction. This supports regular Ethereum transactions when env.cfg.optimism is false and env.tx.optimism.enveloped_tx being None for non-deposit transactions otherwise.

Comment on lines 81 to 82
// input must not be an deposit transaction
debug_assert!(!input.is_empty() && input[0] != 0x7E);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just return U256::ZERO if either of these two conditions are met? This makes the function more abstract as opposed to it only being used for non-deposit, non-empty transactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure changed that, but I was wondering what the expected behavior should actually be for an empty transaction? If anything, shouldn't it return (0 + l1FeeOverhead) * l1Basefee * l1FeeScalar / 1000000? To be consistent with hypothetical one-byte transactions?

Comment on lines -97 to +89
.checked_div(U256::from(1_000_000))
.unwrap_or_default()
/ U256::from(1_000_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer if we kept this as a checked div just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked_div only checks if the divisor is 0. Since it is always hardcoded to one million, I don't think checked div would make any difference.
However, checked operations might make sense before multiplication and addition, as there should be no overflows there.

crates/revm/src/optimism.rs Outdated Show resolved Hide resolved
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm, waiting for you approval from @refcell @clabby

Copy link
Contributor

@clabby clabby left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rakita rakita merged commit 68f78ae into bluealloy:main Oct 12, 2023
8 checks passed
@Wollac Wollac deleted the fix/optimism-execution branch October 12, 2023 18:20
@Wollac Wollac mentioned this pull request Oct 13, 2023
clabby pushed a commit to anton-rs/op-revm that referenced this pull request Oct 15, 2023
* use l2 base fee

* use optimism handler when activated

* fix panic for eth execution with optimism flag

* use is_none

* address review comments
@github-actions github-actions bot mentioned this pull request Jan 12, 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.

None yet

4 participants