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

Articulate EIP parameters and feature implementations #6

Closed
wants to merge 7 commits into from
Closed

Conversation

whilei
Copy link
Owner

@whilei whilei commented Dec 27, 2018

Refactors chain configuration and respective feature implementations to use IsEIP<NUMBER> definitions and methods, instead of Is<HardForkName>, whenever possible. Doing so attempts to address problems of ambiguity and complexity in chain configuration and feature implementation.

As I see it, this refactoring has a few benefits:

  1. More descriptive code. By describing and implementing client configuration with feature-based definitions, instead of "arbitrary" and opaque hard-fork feature groups, feature implementations become clearer. This improves the code's legibility and accessibility, separates logical concerns, documents specification references, and allows more granular testing.

  2. It's more interoperable. Clients choosing to adopt a subset of EIP-derived changes, normally inextricably bundled in a hard-fork identity, are able to toggle individual features. This establishes an extensible pattern that alternative implementations can use to build clients with supersets or subsets of features.

  3. It doesn't break backwards compatibility. All existing hardcoded or external chain configurations continue to operate as expected, tests pass, and named hard-fork keys and methods take priority.

What this patch doesn't do:

  1. Handle every feature ever introduced via the EIP/Hard-fork processes. For example, of the changes introduced with the Homestead fork, only EIP 7's DELEGATECALL has been extracted, and the DAO fork is untouched. Just low-hanging fruit.

  2. Implement correlated refactoring across the tests/[testdata/**/*.json] tests and test runners. These are located in a submodule, very numerous, very opinionated toward the hardfork schema, and their relevance and applicability is not impacted by leaving them as-is.

  3. Implement new distinct difficulty calculators for EIP100 (Change difficulty adjustment to target mean block time including uncles) vs. EIP649 (Delaying the difficulty bomb and reducing the block reward), which were two modifications to the difficulty algorithm introduced simultaneously at the Byzantium hard-fork. I'm trying to limit the scope of changes to introduce as little new consensus logic as possible.

  4. Attempt to differentiate features beyond the specifications of EIPs.

  5. Move beyond supplemental modification of existing patterns.


For review and reference, I compiled this gist along the way.

params/config_test.go Outdated Show resolved Hide resolved
@whilei whilei mentioned this pull request Dec 29, 2018
core/vm/jump_table.go Outdated Show resolved Hide resolved
@whilei whilei mentioned this pull request Jan 3, 2019
@whilei whilei changed the title Features not forks w/o submodule Articulate EIP parameters and feature implementations Jan 4, 2019
Refactors chain configuration and respective feature
implementations to use `IsEIP<NUMBER>` definitions and methods,
instead of `Is<HardForkName>`, whenever possible. Doing so
attempts to address problems of ambiguity and complexity in chain
configuration and feature implementation.

Signed-off-by: Isaac Ardis (isaac.ardis@gmail.com)
@whilei whilei closed this Jan 4, 2019
whilei pushed a commit that referenced this pull request Feb 7, 2019
…) (#6)

Makes Interface interface a bit more stateless and abstract.

Obviously this change is dictated by EVMC design. The EVMC tries to keep the responsibility for EVM features totally inside the VMs, if feasible. This makes VM "stateless" because VM does not need to pass any information between executions, all information is included in parameters of the execute function.
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.

6 participants