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(plugins): Features plug-in, EIP-7702 mode #781

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

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Sep 2, 2024

🗒️ Description

Introduces a new pytest plugin called "features" which allows enabling a specific feature globally for all tests.

EIP-7702 Mode

Fills all tests with each deployed contract being a set-code-delegated account instead of an actual contract, i.e. the behavior of pre.deploy_contract changes to the following:

  1. Deploy the requested code at some location A
  2. Generate an EOA account B
  3. Set storage of B to the requested initial storage (In case of normal allocation during filling, the storage is simply set in the dictionary, but if the test was to be executed on a live network, there would need to be a first authorization transaction to a contract which only purpose is to set the storage)
  4. Create an authorization to set-code of B to A
  5. Return address B

EOF V1 Mode

Fill all tests with each deployed contract (with unspecified EVM code type) set to EOF V1 code type, i.e. pre.deploy_contract now first checks if the parameter evm_code_type is unset, and if the provided deployment code is not already ethereum_test_tools.eof.v1.Container, and if so then wraps the code in an EOF container before setting/deploying the code.

🔗 Related Issues

✅ 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:pytest Scope: Pytest plugins type:feat type: Feature labels Sep 2, 2024
@marioevz marioevz marked this pull request as ready for review September 2, 2024 22:47
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Very powerful! Few small comments below.

Maybe I missed this somewhere: For EOF_V1 mode, could we add some checks and potentially pytest.fail() if it tries to write invalid EOF bytecode to the container (or simply defer to legacy EVM code deployment)? Or do we have state_tests with intentionally invalid EOF bytecode? cf code_pre_processor in https://github.com/ethereum/execution-spec-tests/pull/781/files#diff-e8af78ece9137088fbc9a58dc792341b4881687217d5375010daf85a98d5cd6dR124

What's the intent with regards to handling fork support of features? Should we check that the current fork supports the feature and only enable it, if so?

Btw, I really wish we could add a marker to mark all tests that do call pre.deploy_contract(), but I don't think it's possible?

docs/CHANGELOG.md Outdated Show resolved Hide resolved
src/pytest_plugins/features/features.py Outdated Show resolved Hide resolved
from ethereum_test_vm import EVMCodeType


class Features(Flag):
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Didn't know Flag. Love the example from the docs: https://docs.python.org/3/library/enum.html#enum.Flag

src/pytest_plugins/features/features.py Outdated Show resolved Hide resolved
src/pytest_plugins/filler/pre_alloc.py Outdated Show resolved Hide resolved
@marioevz
Copy link
Member Author

marioevz commented Sep 3, 2024

What's the intent with regards to handling fork support of features? Should we check that the current fork supports the feature and only enable it, if so?

I don't see the flag as being used either automatically by the CI or to fill tests for a release just yet. Right now if you enable the flag you should expect failures because not all tests are compatible, and I think that should be ok. Purpose as I see it is to analyze the errors and check why the incompatible tests are incompatible, fill the ones that can be filled and run those against all clients.

Maybe I missed this somewhere: For EOF_V1 mode, could we add some checks and potentially pytest.fail() if it tries to write invalid EOF bytecode to the container (or simply defer to legacy EVM code deployment)? Or do we have state_tests with intentionally invalid EOF bytecode? cf code_pre_processor in https://github.com/ethereum/execution-spec-tests/pull/781/files#diff-e8af78ece9137088fbc9a58dc792341b4881687217d5375010daf85a98d5cd6dR124

I feel like it's not straightforward to check whether any code is valid EOF code, and the result could be just a different failure? (Is the evm tool failing != pytest.fail()?)

Btw, I really wish we could add a marker to mark all tests that do call pre.deploy_contract(), but I don't think it's possible?

I've been thinking that we could run the test functions during collection with a mock state_test or blockchain_test, and a mock pre that only gathers info and never runs the transition tool for example, but for other purposes such as automatically marking the opcodes used in each test.

marioevz and others added 2 commits September 3, 2024 09:39
Co-authored-by: danceratopz <danceratopz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:pytest Scope: Pytest plugins type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants