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

Replace current fork >= tests everywhere with calculated EIP behaviour flags #640

Closed
jlokier opened this issue May 10, 2021 · 3 comments
Closed
Assignees
Labels
enhancement New feature or request refactoring

Comments

@jlokier
Copy link
Contributor

jlokier commented May 10, 2021

Following on from a conversation at #635 (comment)

Behaviour flags would say which EIP (and other) behaviours are active for the current block. They would replace all occurrences of fork >= in the codebase, except for the part which calculates the flags of course.

They are calculated from chain id, block number and therefore fork id. Then it's probably best to store them as flags, probably on the current block or transaction being processed, and quick to access.

Ideally they would be consistently named and greppable, so it's easy to find all the places where a particular behaviour is conditional, and also find all the places where any behaviour depends on the current chain/block/fork.

Probably we would make these flags configurable for custom chains, testing and so on, just like we currently make the block to fork thresholds configurable.

jlokier added a commit that referenced this issue Jun 8, 2021
Many places outside the EVM use `Fork` and the fork list, and in general we
want progressively fewer dependencies on EVM internal types and files.

This may prove to be a temporary location, especially when we implement
issue #640.  But it's a fine temporary location if so.

Signed-off-by: Jamie Lokier <[email protected]>
@jangko
Copy link
Contributor

jangko commented Nov 17, 2022

After the POS merge, and after we have more subsystem implemented, it become obvious we need to raise this issue priority.

New EF test cases sametimes require network configuration in the format "Fork+EIPS". Currently there is no way to enable or disable eips in nimbus-eth1. We need to manage EIPS in a more sophisticated way.

Other factors contribute to the importance of this issue are:

  • separate data structure to track POS transition
  • abuse of ChainDB to store network param, sync progress and genesis.
  • we have three separate hard forks enum:
    • chain forks
    • EVM forks
    • EVMC forks
  • we can eliminate EVM forks and use EVMC forks for both vm2 and evmc, the we can map chain forks to EVMC forks in an elegant way

There is a need to track POS transition by blockNumber and Total Difficulty in a centralized place rather than scattered around in many places.

note: remember to modify chain config fork block into all optional instead of using blocknumber.high. and also change the fork order validation algorithm

@jangko
Copy link
Contributor

jangko commented Nov 23, 2022

one of hive test, ethereum/consensus depends on this refactoring. currently, all post merge test case are failing.
the problem is: the hive only supply HIVE_MERGE_FORK without HIVE_TERMINAL_TOTAL_DIFFICULTY.

HIVE_TERMINAL_TOTAL_DIFFICULTY indicate the simulator is running engine api test.
if only HIVE_MERGE_FORK present, then it is not engine api test, and it is a regular consensus test.

what's wrong in nimbus-eth1: currently it is confused between merge fork and ttd.
what should nimbus-eth1 do: if merge fork is present, it takes precedence over ttd. if merge fork present, ttd check should be bypassed.

if only ttd exists and no merge fork, then it is a pre merge waiting for merge transition.
after merge transition:

  • consensus switch from POA/POW to POS.
  • sync algo switch from legacy/full to beacon sync.

@jangko
Copy link
Contributor

jangko commented Dec 6, 2022

fixed by #1344, closing

@jangko jangko closed this as completed Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring
Projects
None yet
Development

No branches or pull requests

2 participants