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

Create script + docs to assist with forks #1903

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cburgdorf
Copy link
Contributor

What was wrong?

Forking is error prone and undocumented.

How was it fixed?

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

Copy link
Contributor

@veox veox left a comment

Choose a reason for hiding this comment

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

Highlighted a few typos.

Implementing VM forks
=====================

The Ethereum protocol follows specified rules which continue to be improved through so called
Copy link
Contributor

Choose a reason for hiding this comment

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

s/so called/so-called/

The Ethereum protocol follows specified rules which continue to be improved through so called
`Ethereum Improvement Proposals (EIPs) <https://eips.ethereum.org/>`_. Every now and then the
community agrees on a few EIPs to become part of the next protocol upgrade. These upgrades happen
through so called `Hardforks <https://en.wikipedia.org/wiki/Fork_(blockchain)>`_ which define:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/so called/so-called/

through so called `Hardforks <https://en.wikipedia.org/wiki/Fork_(blockchain)>`_ which define:

1. A name for the set of rule changes (e.g. the Istanbul hardfork)
2. A block number from which on blocks are processed according to these new rules (e.g. ``9069000``)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/on blocks are/blocks start being/



This guide covers how to implement new hardforks in Py-EVM. The specifics and impact of each rule
change many vary a lot between different hardforks and it is out of the scope of this guide to
Copy link
Contributor

Choose a reason for hiding this comment

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

s/many vary/may vary/

------------------------

Every fork is encapsulated in its own module under ``eth.vm.forks.<fork-name>``. To create the
scaffolding for a new fork run ``python scripts/forking/create_fork.py`` and follow the assistent.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/assistent/assistant/


Ethereum is a protocol that powers different networks. Most notably, the ethereum mainnet but there
are also other networks such as testnetworks (e.g. Görli) or xDai. If and when a specific network
will activate a concrete fork remains to be configured on a per network basis.
Copy link
Contributor

@veox veox Dec 19, 2019

Choose a reason for hiding this comment

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

Suggestion for the entire paragraph:

Ethereum is a protocol that powers different networks - most notably, the Ethereum mainnet; but there
are also other value-bearing networks (e.g. xDai), or test networks (e.g. Görli). Whether and when a specific network
will activate a fork remains to be configured on a per-network basis.


BTW, I think xDai is the project name, and they're using the POA side-chain.

are also other networks such as testnetworks (e.g. Görli) or xDai. If and when a specific network
will activate a concrete fork remains to be configured on a per network basis.

At the time of writing, Py-EVM has supports the following three networks:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/has supports/supports/


For each network that wants to activate the fork, we have to create a new constant in
``eth/chains/<network>/constants.py`` that describes the block number at which the fork becomes
active as seen in the following example:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/active/active,/


if not all(x.isalpha() or x.isspace() for x in fork_name):
print(f"Can't use {fork_name} as fork name, must be alphabetical")
return
Copy link
Member

Choose a reason for hiding this comment

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

Should probably return non-zero so sys.exit(1)?

print(f"Can't use {fork_name} as fork name, must be alphabetical")
return

print("Specify the fork base (e.g Istanbul):")
Copy link
Member

Choose a reason for hiding this comment

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

This strikes me as something we can pre-populate a default for by looking at the latest mainnet fork.

dash_case = normalized.replace(' ', '-')
pascal_case = normalized.title().replace(' ', '')

return Writing(
Copy link
Member

Choose a reason for hiding this comment

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

It would be ideal for this to output something like:

For name: "Istanbul"
python module: ./eth/forks/istanbul/
block number variable name: "ISTANBUL_BLOCK_NUMBER"
...

So that it's being very explicit about what the script is going to do.



def create_fork(writing_new_fork: Writing, writing_parent_fork: Writing) -> None:
fork_path = FORKS_BASE_PATH / writing_new_fork.lower_snake_case
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest writing all of this to a temporary directory and then moving the temporary directory into place once it's all done.

from .state import IstanbulState


class IstanbulVM(ConstantinopleVM):
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking these files should be templated rather than "white labeled"

from eth.vm.forks.<% parent_fork_snake_case %> import (
    <% parent_fork_name %>VM,
)

class <% fork_name %>VM(<% parent_fork_name %>VM):
    ...

This makes it future proof, otherwise we will likely run into a scenario where the find-replace regexing replaces text somewhere like a comment or something....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking these files should be templated rather than "white labeled"

Yeah, I also thought about that. The one downside is that you'd lose the ability to run simple flake8/mypy checks against it. You'd have to actually create a fork and run flake8/mypy checks against the result. Not necessarily bad though.

Anyway, this PR is just a very early draft anyway. But thanks for reviewing anyway (also to @veox 👍 )

@cburgdorf
Copy link
Contributor Author

@a1love Did you mean to tell us something?

@cburgdorf cburgdorf mentioned this pull request May 26, 2020
2 tasks
@carver carver mentioned this pull request Feb 19, 2021
2 tasks
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