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

Add Support for BlockchainTests #2

Merged
merged 19 commits into from
Nov 3, 2022
Merged

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Oct 28, 2022

Changes Included

  • Adds support for multi-block tests, which are necessary to verify behavior that transcends a single block, such as basefee calculation, fork transitions, withdrawals, blobspace transaction fees calculation, etc.
  • Adds requirement for rlp module import, because t8n tool currently returns transactions' RLP that includes rejected ones, and these should be removed before producing the final block.
  • Introduces new types to better make a distinction between the types that are used to describe a test and the ones used to format the inputs to the transition or the block tool.

ToDos

  • Requires cmd/evm: calc base fee if parent data is present go-ethereum#26051 to properly work, since this PR introduces currentBasefee as output of the execution result.
  • mypy is currently complaining about the usage of kw_only property of dataclass.
  • Use kw_only on all dataclass classes to force test specs to specify keywords when declaring test objects. The argument to this is that it allows the objects to be updated in the future and not having to refactor all tests due to positional argument conflicts.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Some small comments and nits, overall looks great!

src/ethereum_test/base_test.py Outdated Show resolved Hide resolved
src/ethereum_test/blockchain_test.py Outdated Show resolved Hide resolved
src/ethereum_test/blockchain_test.py Outdated Show resolved Hide resolved
if self.ty is None:
# Try to deduce transaction type from included fields
if (
self.max_fee_per_gas is not None
Copy link
Member

Choose a reason for hiding this comment

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

If only one is set, will the other be set automatically? I think yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it will fail, I checked geth code and it requires both, since there is no way to deduce the other value if only one is set I think.

src/ethereum_test/types.py Show resolved Hide resolved
src/ethereum_test/types.py Outdated Show resolved Hide resolved
src/ethereum_test/types.py Outdated Show resolved Hide resolved
src/ethereum_test/types.py Outdated Show resolved Hide resolved
@marioevz marioevz marked this pull request as ready for review November 2, 2022 00:04
@marioevz
Copy link
Member Author

marioevz commented Nov 2, 2022

The only remaining issue seems to be whether to include or remove the invalid transactions from blocks.

I can follow up on this in a subsequent PR if that's ok.

@lightclient
Copy link
Member

I might be missing something, but it feels like this PR is making an active choice to remove invalid transactions from the RLP and so we should remove that logic from this PR and decide the best way to deal with invalid txs separately. Is there a reason that work is necessary for this specific PR?

@marioevz
Copy link
Member Author

marioevz commented Nov 2, 2022

I might be missing something, but it feels like this PR is making an active choice to remove invalid transactions from the RLP and so we should remove that logic from this PR and decide the best way to deal with invalid txs separately. Is there a reason that work is necessary for this specific PR?

No it was just simply to keep from adding more and more modifications to the PR, but yes now that you mention it, we should not be adding a new import that we are supposed to remove in another PR. Let me add the changes in this PR 👍

@lightclient
Copy link
Member

Looks good to merge once CI goes green!

@marioevz marioevz merged commit ec699de into ethereum:main Nov 3, 2022
danceratopz pushed a commit that referenced this pull request Jun 7, 2023
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.

3 participants