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

Move EIP Integration to Common #869

Merged
merged 4 commits into from
Sep 14, 2020
Merged

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Sep 11, 2020

Follow-up on #856 and #863

This PR moves EIP integration from the VM to Common to be able to spawn EIP implementations across different libraries touched (as currently possible along hardfork lines). Upon this base integration it updates the EIP2537 integration in the VM.

The Common integration is hopefully somewhat future proof. I already added an additional check for minimumHardfork requirements where Common initialization will throw if not met (EIP2537 is currently set to chainstart, let me know if this is too low for some reasons).

Not yet integrated is a possible EIP dependency, e.g. when one EIP depends on one or several others. This could be implemented by adding a parameter EIPdependencies or the like to the EIP*.json files. Since this comes with some mild extra complexity - e.g. on the param() functionality - I didn't want to integrate directly and eventually overload the PR. Think this can be added once this case occurs.

I also decided to change the semantics of the param() function and add changed demarcation lines to the existing paramByEIP() and a newly added paramByHardfork() function. param() removes the hardfork parameter and is now the function which just "does the right thing" [tm] - which means it either takes the parameter from the latest hardfork or - if present - from an EIP set. If an EIP is set and contains the parameter, this setting always takes precedence (which I think makes sense and would be expected e.g. on the VM instantiated with a certain HF and an additional EIP changing gas costs e.g.). paramByEIP() and paramByHardfork() can now be used to explicitly demand for an EIP respectively HF-specific parameter.

This makes use of theparam() function - especially in the VM - completely transparent and avoids code such as:

if (EIP_ACTIVATED) {
  common.paramByEIP(...)
} else {
  common.param()
}

Instead the current param() usage can stay completely "as is" and the parameter is chosen correctly based on the chain setup in Common.

…thod in Common, changed param() semantics, updated EIP2537 VM integration
@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #869 into master will decrease coverage by 0.07%.
The diff coverage is 78.37%.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 77.14% <66.66%> (ø)
#blockchain 80.79% <ø> (ø)
#common 93.04% <81.48%> (-1.70%) ⬇️
#ethash 83.33% <ø> (+1.11%) ⬆️
#tx 93.98% <ø> (-0.14%) ⬇️
#vm 82.21% <71.42%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

Ah, too many VM API tests still failing, will look into that later.

…ge.json test:state script skipping some tests on selected operating systems
@holgerd77
Copy link
Member Author

Fixed that remaining failing tests.

This is now ready for review. 😄

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

LGTM, good stuff!

@holgerd77 holgerd77 merged commit 10e76c9 into master Sep 14, 2020
@holgerd77 holgerd77 deleted the move-eip-support-to-common branch September 14, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants