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

Support for the Paris hard fork ("The Merge") #2080

Merged
merged 9 commits into from Nov 8, 2022

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Sep 15, 2022

What was wrong?

Paris "the merge" fork support is needed.

How was it fixed?

Support for Paris logic added:

  • Paris vm added and relevant classes.
  • EIP-4399: Supplant DIFFICULTY opcode with PREVRANDAO.
  • Turn on merge-related ethereum/tests.
  • Address some issues with our header db. Difficulty was used to calculate keys for scores and difficulty is always 0 now. This caused issues with overwriting same values for keys.
  • Make Merge tests pass by poking around the library and addressing other issues.
    • The transaction fee being 0 and going to the coinbase address never triggered EIP-161 to actually remove the account from state trie calculation if the fee recipient account was in a zero-state at the end of the computation. For the last 15 remaining tests, this ended up being what was causing a wrong state root. (i.e. The coinbase account is always touched, so if it is in a zero-state after the computation we have to mark it for deletion)

Todo:

  • Clean up commit history
  • Clean up "minging" nomenclature around base classes, add PoS consensus... these will likely be done in a separate PR to keep things smaller
  • Address failing tests
  • Add entry to the release notes

Cute Animal Picture

put a cute animal picture link inside the parentheses

@fselmo fselmo force-pushed the merge-gradual-changes branch 2 times, most recently from 5b8e3d1 to 1be70a9 Compare September 15, 2022 00:16
@fselmo fselmo changed the title Merge gradual changes [WIP] Merge gradual changes Sep 15, 2022
@fselmo fselmo force-pushed the merge-gradual-changes branch 4 times, most recently from a06784a to 541495b Compare September 21, 2022 21:48
@fselmo fselmo force-pushed the merge-gradual-changes branch 3 times, most recently from 8beb3fa to e20500d Compare September 30, 2022 00:18
@fselmo
Copy link
Collaborator Author

fselmo commented Sep 30, 2022

This PR has the rough implementation necessary to pass all the logic behind the merge but I think what comes after this should be the cleaning up of nomenclature around mining, etc... to be a bit more generalized, at least in the base classes. We can probably take this time to refactor some code along the way as well.

I created #2079 to track that.

@fselmo fselmo changed the title [WIP] Merge gradual changes Support for the Paris hard fork (Merge) Sep 30, 2022
@fselmo fselmo changed the title Support for the Paris hard fork (Merge) Support for the Paris hard fork ("The Merge") Sep 30, 2022
@fselmo fselmo marked this pull request as ready for review September 30, 2022 00:33
Copy link
Collaborator Author

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

Added a few notes that still need addressing [PR is ready]

eth/vm/base.py Show resolved Hide resolved
eth/vm/forks/paris/headers.py Outdated Show resolved Hide resolved
tests/core/builder-tools/test_chain_construction.py Outdated Show resolved Hide resolved
@fselmo fselmo force-pushed the merge-gradual-changes branch 3 times, most recently from ed19891 to 41b7029 Compare September 30, 2022 15:27
fselmo added a commit to fselmo/py-evm that referenced this pull request Sep 30, 2022
Copy link
Collaborator

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm!

fselmo added a commit to fselmo/py-evm that referenced this pull request Oct 20, 2022
fselmo added a commit to fselmo/py-evm that referenced this pull request Oct 20, 2022
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Awesome! This looks good to me! I just had a couple of clarifying questions, but nothing to stop merging this.

eth/vm/forks/arrow_glacier/headers.py Show resolved Hide resolved
eth/vm/forks/paris/blocks.py Outdated Show resolved Hide resolved
tests/core/consensus/test_clique_consensus.py Show resolved Hide resolved
- Use the block header class for the appropriate VM by validating up to the previous VM, extracting the params, and plugging into the appropriate VM class.
- Remove unnecessary fields in London state since they are already inheritted and do not need to be re-imported / overwritten
- Add ``Paris`` fork VM classes.
- Supplant ``DIFFICULTY`` opcode at ``0x44`` with ``PREVRANDAO``.
- Allow ``mix_hash`` to be retrieved from the execution context and add ``mixhash`` opcode logic function to be used in new ``PREVRANDAO`` opcode.
- Some renaming of "Mining" nomenclature found within more general classes / methods - created an issue at ethereum#2079 to track more of these changes in a separate PR.
- Minor cleanups along the way.
- Turn on ``ethereum/tests`` "Merge" tests to test against ``Paris`` VM implementations.
- Skip slow tests related to previous VM tests (london and berlin mostly).
- Update ``eth-typing`` and pull in updated ``ForkName`` enum with ``Paris`` added.
- Fix some blips and get tests to pass.
- Our header db relied on difficulty to generate keys. Since the merge / PoS, difficulty is always ``0`` so this led to overwriting values due to the same key being generated (e.g. new_score = old_score + block.difficulty). Use the block number instead for PoS (or when difficulty=0).

- The ``coinbase`` account is always touched, even when it receives a transaction fee with a value of ``0```. With recent changes for the ``Merge`` implementation, this was creating an issue for "Merge" tests where the ``stateRoot`` did not match. This seems to be because in every case for PoW, the ``coinbase`` account will get a block reward. This means that if the transaction fee was ``0``, it wouldn't matter because the ``coinbase`` would never end up with ``0`` balance, so all tests before PoS pass. Now that we are in a PoS context, there is no block reward, thus, when the transaction fee is ``0``, we will touch the account and leave it at a zeroed state. In order to implement EIP-161 correctly, if the ``coinbase`` account has ``0`` balance, empty code, ``0`` nonce, and no storage, it should be marked for deletion and not be included in the state root calculation.
@fselmo fselmo merged commit 40b9881 into ethereum:master Nov 8, 2022
fselmo added a commit that referenced this pull request Nov 8, 2022
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.

None yet

3 participants