ethereum / evmone

Fast Ethereum Virtual Machine implementation
Apache License 2.0
863 stars 285 forks source link

test: Ensure exported tests have accessLists for eip1559 txs #1027

Closed pdobacz closed 2 months ago

pdobacz commented 2 months ago

Opening as draft to discuss if I'm understanding things correctly here.

The problem was that our exported test txs were missing accessLists field, even though they contained eip1559 fields. I think making the default state_transition tx be eip1559, and making it explicit wherever we want legacy instead is the solution, but I'm not sure.

Example exported JSON without accessLists:

    "transaction": {
      "data": [
        "0x"
      ],
      "gasLimit": [
        "0xf4240"
      ],
      "maxFeePerGas": "0x3e8",
      "maxPriorityFeePerGas": "0x3e8",
      "nonce": "0x1",
      "secretKey": "0x00000000000000000000000000000000000000000000000000000002b1263d2b",
      "sender": "0xe100713fc15400d1e94096a545879e7c6407001e",
      "to": "0x000000000000000000000000000000000000c0de",
      "value": [
        "0x0"
      ]
    }

from fixtures/state_tests/state_transition/eof_examples_callf.json.

Does this make sense?

chfast commented 2 months ago

It depends who is complaining about the missing accessLists. If only evmone, then we can just relax the parsing rules. All this is likely because we are forced to guess the transaction type by the fields...

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.21%. Comparing base (c885db3) to head (86fc519). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1027 +/- ## ========================================== + Coverage 94.19% 94.21% +0.01% ========================================== Files 149 149 Lines 15896 15927 +31 ========================================== + Hits 14974 15005 +31 Misses 922 922 ``` | [Flag](https://app.codecov.io/gh/ethereum/evmone/pull/1027/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [eof_execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1027/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `17.28% <0.00%> (-0.04%)` | :arrow_down: | | [ethereum_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1027/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `27.33% <0.00%> (-0.06%)` | :arrow_down: | | [ethereum_tests_silkpre](https://app.codecov.io/gh/ethereum/evmone/pull/1027/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `19.14% <0.00%> (-0.05%)` | :arrow_down: | | [execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1027/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `20.44% <0.00%> (-0.05%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/ethereum/evmone/pull/1027/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `89.01% <100.00%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/ethereum/evmone/pull/1027?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [test/unittests/state\_transition.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1027?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbi5jcHA=) | `98.37% <100.00%> (ø)` | | | [test/unittests/state\_transition.hpp](https://app.codecov.io/gh/ethereum/evmone/pull/1027?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbi5ocHA=) | `0.00% <ø> (ø)` | | | [test/unittests/state\_transition\_create\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1027?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition_create_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbl9jcmVhdGVfdGVzdC5jcHA=) | `100.00% <100.00%> (ø)` | | | [test/unittests/state\_transition\_extcode\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1027?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition_extcode_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbl9leHRjb2RlX3Rlc3QuY3Bw) | `100.00% <100.00%> (ø)` | | | [test/unittests/state\_transition\_touch\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1027?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition_touch_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbl90b3VjaF90ZXN0LmNwcA==) | `100.00% <100.00%> (ø)` | | | [test/unittests/state\_transition\_tx\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1027?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition_tx_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbl90eF90ZXN0LmNwcA==) | `100.00% <100.00%> (ø)` | |
pdobacz commented 2 months ago

It depends who is complaining about the missing accessLists. If only evmone, then we can just relax the parsing rules. All this is likely because we are forced to guess the transaction type by the fields...

Nope, it's consumers of the exported tests.

pdobacz commented 2 months ago

FWIW I diffed the 0.13.0 tests and ones exported from this PR branch and the only difference is the addition of some empty accessLists fields in some JSONs, which seemed to already contain 1559 fields

chfast commented 2 months ago

Ok, let's go with this. The only alternative I have is to do the tx type guessing during export...

Just add a comment why this is the default transaction type.