Skip to content

feat(tests): Additional EIP-7934 test cases #1890

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

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

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jul 10, 2025

🗒️ Description

  • Add test case for RLP block size at limit with all transaction types, including blobs, access lists, and authorizations
  • Add test case for RLP block size at limit with logs emitted

🔗 Related Issues or PRs

Related to #1772

TODO:

  • Refactor built transactions for with_all_tx_types into something that can be more useful across the board. I'm thinking default transactions that each fork can define if it includes a new tx type and tests can automatically use these transactions, override them as fixtures, or some other more useful flow than re-building every time.
    • This was done via with_all_typed_transactions indirect (new feature) marker

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.

@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch 4 times, most recently from 6335dc3 to cd39587 Compare July 11, 2025 20:55
@fselmo fselmo marked this pull request as ready for review July 11, 2025 20:55
@fselmo fselmo added scope:pytest Scope: Changes EEST's pytest plugins scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Jul 11, 2025
@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch from cd39587 to a2146f9 Compare July 11, 2025 21:52
fselmo added 2 commits July 11, 2025 16:47
- Create global fixtures for default transactions that are covariant with ``fork``
- These should allow overriding particular default transactions by re-defining the
  fixture for the particular tx type e.g.:
  ```python
  @pytest.fixture
  def type_2_default_transaction(pre):
      return Transaction(...)
  ```
- Use these as part of the ``fork`` plugin
@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch from a2146f9 to eab5984 Compare July 11, 2025 22:48
@fselmo
Copy link
Collaborator Author

fselmo commented Jul 11, 2025

Hmm having a difficult time reproducing this CI error locally. I added the concept of a @with_all_typed_transactions as a fixture that uses the tx_type covariant with fork to build default transactions. I successfully implemented this in the test that I wanted for testing the rlp block size limit. Each default transaction can be overridden if desired with:

@pytest.fixture
def type_2_default_transaction(sender):
    return Transaction(...)

Something in the stdout is being changed and ironically I am on a macos and I pass this test locally but the CI does not. I will keep looking into this but it would be nice to get a review here on the implementation and if anyone has an idea of what's causing this error, lmk.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for implementing this.

I wrote a couple of comments that might be worth considering.

Comment on lines 130 to 131
@pytest.fixture
def typed_transaction(request, fork):
Copy link
Member

Choose a reason for hiding this comment

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

I think we could create ./tests/conftest.py and move all of these fixtures there with some minor changes to not require a change in the plugins:

Suggested change
@pytest.fixture
def typed_transaction(request, fork):
@pytest.fixture
def typed_transaction(tx_type: int, fork: Fork) -> Transaction:

Then the test adds typed_transaction: Transaction as a parameter, but it might have the downside that the test has to also include the tx_type: int parameter to simply not use it, and maybe some other downside I could be missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok yeah, this was the route I was originally going to take but wasn't sure why a global conftest.py didn't exist yet and thought it may not be desirable. I think if you're good with that I am good with it as well 👍🏼

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it might have the downside that the test has to also include the tx_type: int parameter to simply not use it

I implemented it so that the typed_transaction fixture uses the tx_type and so all you have to do is use the @pytest.mark.with_all_tx_types and you can use the typed_transaction directly. If it's better to keep this as a separate marker, just let me know, but I think if we document it well enough that you can use either the tx_type or the typed_transaction then it's fairly frictionless and versatile.

lmk what you think about that or if the separation of concerns is better.

Copy link
Collaborator Author

@fselmo fselmo Jul 16, 2025

Choose a reason for hiding this comment

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

Ok. So based on @danceratopz comments, I moved it back to the plugins. But based on your request @marioevz, I put this inside shared and available to shared/execute_fill.py plugin. I refactored the logic for the indirect covariant checks so it could be used by other markers in the future if needed. The CovariantDecorator takes an indirect kwarg that will run the values of the fork_attribute_name in this case through the fixtures (argnames) which is then what is used in the test.

I ended up creating a more global sender and I did so inside execute_fill.py but perhaps this can live somewhere else or this may not be ideal, so let me know there. But we can remove the redundant re-definitions of sender fixture that don't need some special type of logic if we have this more global one set here.

Would love thoughts on this from you both.

Copy link
Collaborator Author

@fselmo fselmo Jul 16, 2025

Choose a reason for hiding this comment

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

Global sender allows us to remove quite a few redundancies in tests/ fixtures, as per the last commit in this PR.

fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Jul 15, 2025
- Move default transaction logic to a ``conftest.py`` setup.
- Add ``conftest.py`` files for each fork with the respective
  transaction pytest fixtures.
- Opt for `Type | None` over `Optional[Type]` to avoid an extra import.
fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Jul 15, 2025
- Move default transaction logic to a ``conftest.py`` setup.
- Add ``conftest.py`` files for each fork with the respective
  transaction pytest fixtures.
- Opt for `Type | None` over `Optional[Type]` to avoid an extra import.
@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch from 8451823 to d26c90b Compare July 15, 2025 16:55
@fselmo
Copy link
Collaborator Author

fselmo commented Jul 15, 2025

I'm just missing the documentation here which I can update but we should get consensus on the code first and then I can document the changes appropriately and add the release notes.

fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Jul 15, 2025
- Move default transaction logic to a ``conftest.py`` setup.
- Add ``conftest.py`` files for each fork with the respective
  transaction pytest fixtures.
- Opt for `Type | None` over `Optional[Type]` to avoid an extra import.
@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch from d26c90b to 39d2880 Compare July 15, 2025 18:35
  - Move transaction fixtures to a `shared` pytest plugin.
  - Opt for `Type | None` over `Optional[Type]` to avoid an extra import.

Bonus:
  - Refactor the `indirect` marker logic to be more generic for future use.
    If a marker uses `indirect`, it will take the values from the
    `fork_attribute_name` and run it through a fixture of name `argnames`
    which provides some added logic. In this case it builds default
    transactions from the `tx_types` values for each fork.
@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch from 39d2880 to d03452a Compare July 16, 2025 22:05
@fselmo fselmo force-pushed the feat/additional-eip7934-tests branch from 3fc79fd to 619bbaa Compare July 16, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:pytest Scope: Changes EEST's pytest plugins scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants