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

Add London Support #54

Merged
merged 29 commits into from
Oct 27, 2021
Merged

Conversation

joshuajbouw
Copy link
Contributor

@joshuajbouw joshuajbouw commented Aug 19, 2021

This is a working branch for bringing London into the EVM.

In order to keep to do date with Ethereum upstream, we must actively keep up to date with its milestone updates. This is the hard fork following Berlin, and the London hard fork went live as of August 5th, 2021.

To track the progress of the ETH tests, go to the PR here: TODO!

NOTE: #40 must be merged in first!

Todo

  • EIP-3198: BASEFEE opcode
    • Initial work
    • Reviewed and final
  • EIP-3529: Reduction in refunds
    • Initial work
    • Reviewed and final
  • EIP-3541: Reject new contracts starting with the 0xEF byte
    • Initial work
    • Reviewed and final

Other EIPs relating to London are not relating to the evm.

@joshuajbouw joshuajbouw marked this pull request as ready for review September 13, 2021 07:07
@joshuajbouw
Copy link
Contributor Author

@sorpaas this good to go?

@sorpaas
Copy link
Member

sorpaas commented Sep 28, 2021

Will do this soon. Sorry for the delay!

@gakonst
Copy link
Contributor

gakonst commented Oct 16, 2021

@sorpaas anything we can do to help push this PR forward?

@gakonst
Copy link
Contributor

gakonst commented Oct 19, 2021

@birchmd @joshuajbouw looks like the jsontests are fetched from master, think you'd need to checkout your branch here

@birchmd
Copy link
Contributor

birchmd commented Oct 19, 2021

@birchmd @joshuajbouw looks like the jsontests are fetched from master, think you'd need to checkout your branch here

On previous breaking changes like (e.g. Berlin) this we've left it up to @sorpaas to run the tests locally and see they pass, without worrying too much about the GitHub checks. We could make the check smarter if desired, but as I understand it that is not holding up this PR. I hope @sorpaas will take a look at this PR soon and let us know if it can be merged.

Note: all tests do indeed pass if running this branch on rust-blockchain/evm-tests#8

@@ -83,6 +83,18 @@ pub fn gasprice<H: Handler>(runtime: &mut Runtime, handler: &H) -> Control<H> {
Control::Continue
}

pub fn base_fee<H: Handler>(runtime: &mut Runtime, handler: &H) -> Control<H> {
if !runtime.config.has_base_fee {
return Control::Exit(ExitReason::Fatal(ExitFatal::NotSupported));
Copy link
Member

Choose a reason for hiding this comment

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

This should not be fatal. Fatal is reserved for situations that the evm library cannot handle at all (trying to allocate memory more than 64-bit, for example) and it will abruptly exit all evm execution stack. In short, it's not a normal EVM semantics.

I believe you can just remove this since it is handled already by gasometer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 14b82b7

Self::config_with_derived_values(100, 2100, 1900, true, true, true)
}

const fn config_with_derived_values(
Copy link
Member

Choose a reason for hiding this comment

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

Just for eye candy -- I wonder if you can create a private struct DerivedValues and put the parameters inside that struct. It will be easier to extend in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in cdb5a8c

@@ -351,6 +351,20 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
self.state.metadata().gasometer.gas()
}

fn create_init(
Copy link
Member

Choose a reason for hiding this comment

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

This function seems to do two different things record_create_transaction_cost and initialize_with_access_list which I'm not certain is strictly related. I suggest we split it out and name them explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 072d8bf

Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me but some grumbles.

@sorpaas
Copy link
Member

sorpaas commented Oct 27, 2021

Also please fix lints!

@birchmd
Copy link
Contributor

birchmd commented Oct 27, 2021

Thanks for the review @sorpaas ! I have addressed all the comments, please take another look.

@@ -680,6 +690,13 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
ExitReason::Succeed(s) => {
let out = runtime.machine().return_value();

// As of EIP-3541 code starting with 0xef cannot be deployed
if let Err(e) = check_first_byte(self.config, &out) {
self.state.metadata_mut().gasometer.fail();
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line. fail is for Fatal and I don't think this is what we want here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry. I got this wrong. This is correct.

Copy link
Member

Choose a reason for hiding this comment

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

rel #78

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 33d9e15

Copy link
Member

Choose a reason for hiding this comment

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

Sorry please revert this for now! We use gasometer.fail elsewhere in the same function for error/revert. They should be equivalent with or without, but let's keep it consistent and review the usage later in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverted in 0b61849

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.

4 participants