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

impose test format verification after fill #940

Open
winsvega opened this issue Nov 5, 2024 · 4 comments
Open

impose test format verification after fill #940

winsvega opened this issue Nov 5, 2024 · 4 comments
Assignees
Labels
good first issue Good for newcomers scope:fill Scope: fill command type:feat type: Feature

Comments

@winsvega
Copy link
Contributor

winsvega commented Nov 5, 2024

looks like evmone-t8n generated genesis with "baseFeePerGas": "0x00", on frontier network

so we need to check the generated test format with consume. like I did with retesteth. if blockchain test is of network frontier. the genesis must not have other fork fields
and vice versa. this is not being checked. I am getting test with genesis header with baseFeePerGas when using evmonet8n

Here separation of concern. first I have module that generates the tests. Second is the module that executes tests.
The execution module does not know what kind of json it gets therefor it has a scheme validator to analyze the correctness of json.

I would still vote on test json schema verification model. Maybe if not after fill, but in Dan's consume. although would be nice to have it in fill too.

yes it might be redundant perhaps it can be optional in fill.
the idea is to have all the logic related to json verification at one place.

I have the following checks:

  • verify that rlp fields are equal to it's json description. (although it is adviced to parse rlp only, some clients might parse json instead. so we need to make sure that test's json is equal to it's rlp.)
  • verify fixture structure. verify that it has only expected fields. and nothing is missing. depending on fixture format.
  • verify block header description is not only equal to it's rlp but also that if it's a valid block it must be of the type of network the test is on.
@danceratopz
Copy link
Member

Currently evmone's t8n returns invalid block header fields for Frontier (to be double-checked by @winsvega) - this is blocking #808.

@winsvega winsvega self-assigned this Nov 22, 2024
@winsvega winsvega added good first issue Good for newcomers type:feat type: Feature scope:fill Scope: fill command labels Dec 2, 2024
@winsvega
Copy link
Contributor Author

winsvega commented Dec 2, 2024

ethereum/evmone#1073

@winsvega
Copy link
Contributor Author

winsvega commented Feb 7, 2025

@marioevz if you think it can be redundant perhaps it should stay at least in consume.
I won't trust unit tests alone I think it is good to have a json reader validation scheme to yell an error if we detect malformed fixture. and this code better be separate from our internal filling logic to not be affected by it or for the case when we import generated json and don't know who has generated it.

@marioevz
Copy link
Member

marioevz commented Feb 7, 2025

@marioevz if you think it can be redundant perhaps it should stay at least in consume. I won't trust unit tests alone I think it is good to have a json reader validation scheme to yell an error if we detect malformed fixture. and this code better be separate from our internal filling logic to not be affected by it or for the case when we import generated json and don't know who has generated it.

I think FixtureHeader is already our validator scheme, it's broken at the moment but the fix is simple in my opinion.

This class can be the source of truth, I don't see why we have to create more structures since this one can already verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers scope:fill Scope: fill command type:feat type: Feature
Projects
None yet
Development

No branches or pull requests

3 participants