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

bump evm + remove EIP-3607 extension for precompiles #1032

Conversation

nanocryk
Copy link
Contributor

Follow up to rust-ethereum/evm#157.

Cost of calling is_precompile is now accounted for. To avoid any increase in cost for the top level call, I removed the extension of EIP-3607 which prevented to transact from a precompile (which is very improbable anyway).

@nanocryk nanocryk requested a review from sorpaas as a code owner March 30, 2023 12:53
@sorpaas
Copy link
Member

sorpaas commented Mar 30, 2023

I'm super confused. Why is EIP-3607 improbable and how does this relate to evm#157?

if is_transactional
&& (!<AccountCodes<T>>::get(source).is_empty() || precompiles.is_precompile(source))
{
if is_transactional && !<AccountCodes<T>>::get(source).is_empty() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_precompile now takes an extra argument being the remaining gas, and if not OOG will return the bool and the cost of performing that check. This function is used in only 2 places in frontier and evm:

  • evm: inside is_cold, which occurs when we interact with a smart contract (getting code, doing a subcall)
  • frontier (where this comment is): to know if the from address is the one of a precompile. EIP-3607 forbids from to be the address of a smart contract, and we extended it in frontier so that it cannot be the one of a precompile. The precompile part is NOT part of the EIP.

Now it is required to take into account the cost of calling is_precompile, which is less trivial than the change in the EVM. In this frontier code it is a non trivial change, I thus suggest we get rid of that part to keep things simple (we could maybe deduct the cost from the gas limit or whatever, but it may have unwanted side effects).

@nanocryk
Copy link
Contributor Author

Replaced by #1058

@nanocryk nanocryk closed this May 25, 2023
@koushiro
Copy link
Collaborator

@nanocryk Do you need to remove the extension of EIP-3607 again in other PR?

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