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

feat(fw): Add is_state_test flag to Environment #652

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jun 26, 2024

🗒️ Description

Adds a flag isStateTest into the environment information that is passed to the t8n tool which is only set to true when executing the state tests, and should inhibit any block-level operations in order to reduce the changes to the state that are produced by:

  • The mining reward
  • Block Withdrawals
  • System operations

🔗 Related Issues

None

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@marioevz marioevz added scope:tests Scope: Test cases type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) labels Jun 26, 2024
@marioevz
Copy link
Member Author

Requires: ethereum/go-ethereum#30074

@chfast
Copy link
Member

chfast commented Jun 28, 2024

I like the change however, there are some other quirks around "block processing" in t8n, e.g. you can pass --state.reward=-1 to signal the block reward should not be applied.

How will this accept state tests themselves? They are currently expected to process system calls, withdrawals and apply the block reward of 0 👿.

@marioevz marioevz requested a review from winsvega July 8, 2024 17:00
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

I remember I offered this as a flag to t8n a year ago, but it was rejected.
--statetest

But even better:

--mode=statetest [default blockchain]

Or in env file

transitionMode: "statetest"

IMHO its more of a cmd flag than env option.

The state reward=-1 will be consumed by this option. And existing state tests would be possible to regenerate as a true state transition only. Currently state tests are blockchain tests with reward 0. (Because created by t8n)

Also what happened to state test depreciation idea?

@winsvega
Copy link
Collaborator

winsvega commented Jul 9, 2024

I like the change however, there are some other quirks around "block processing" in t8n, e.g. you can pass --state.reward=-1 to signal the block reward should not be applied.

How will this accept state tests themselves? They are currently expected to process system calls, withdrawals and apply the block reward of 0 👿.

only block reward of 0 as coinbase touch. no withdrawals in state tests. (the state tests currently are blockchain tests with reward 0, 1 block, empty uncles/ withdrawals)
lets make a list of what should be disabled in a pure state test and check with the teams if they can easily support that in t8n.

@marioevz
Copy link
Member Author

I like the change however, there are some other quirks around "block processing" in t8n, e.g. you can pass --state.reward=-1 to signal the block reward should not be applied.

How will this accept state tests themselves? They are currently expected to process system calls, withdrawals and apply the block reward of 0 👿.

I think --state.reward < 0 should be deprecated eventually, I don't think it can be done immediately because retesteth still sends this value for state tests, but is_state_test=True should be equivalent to --state.reward=-1 and also remove the block processing.

@marioevz
Copy link
Member Author

transitionMode: "statetest"

I like this one, maybe transitionMode could be transaction or block, where transaction is equivalent to is_state_test=True.

Wdyt @chfast ?

@spencer-tb
Copy link
Collaborator

spencer-tb commented Jul 17, 2024

I like this one, maybe transitionMode could be transaction or block, where transaction is equivalent to is_state_test=True.

Just to chime in! I'm a fan of having the mode in the env, transaction/block sounds good to me as options.

Would executionLevel or executionContext make more sense than transitionMode.

 --executionContext <type>  Specifies the execution context for the state transition:
                           transaction - Apply transactions without block-level operations 
                           block          - Includes block-level operations when applying txs

@winsvega
Copy link
Collaborator

Ah wait. In state tests reward 0.
Reward -1 is used to calculate state root of genesis
I never send txs with reward -1 so I don't think there is state mode currently supported.

@chfast
Copy link
Member

chfast commented Jul 21, 2024

I don't care so much about the specific name so you can pick the one you like the best.

I have small preference for the env JSON over command line flag. The invocation is already not commnad-line friendly because there are 3 mandatory JSON files to be used. From my point of view the t8n is the same as statetest just the input is provided differently and in statetest there is the "expected" section. However, the statetest could be easily used without the "expected" section as some EVM fuzzers already do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fw Scope: Framework (evm|tools|forks|pytest) scope:tests Scope: Test cases type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants