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

new(cli): Introduce eofwrap tool #896

Merged
merged 12 commits into from
Oct 25, 2024
Merged

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented Oct 15, 2024

🗒️ Description

There are many general EVM tests in ethereum/tests which provide coverage for opcodes and patterns which should behave independently from whether they run as legacy or as EOF code. Think ADDMOD, SSTORE, SHL etc... These likely will eventually be migrated manually to EEST, and then the existing tools will be there to wrap them in EOF containers. However, this is a long term task, and we would like to have the coverage early, in preparation for the EOF fork.

It turns out many of ethereum/tests state tests (here using their blockchain_test format, read on to see why) can be opportunistically (that is if they "wrap" easily - good, if not - skip) wrapped into EOF containers and executed and the same expectations on post state will apply.

This PR introduces a new CLI tool eofwrap accomplishes this. I've used it on v14.1 release ethereum/tests (subdir BlockchainTests/GeneralStateTests/). The metrics.json contents is:

{
    "files_generated": 1108,
    "files_skipped": 1656,
    "fixtures_generated": 11110,
    "fixtures_cant_wrap": 7973,
    "fixtures_cant_generate": 262,
    "accounts_wrapped": 53344,
    "unique_accounts_wrapped": 1165,
    "accounts_invalid_eof": 102192,
    "accounts_cant_generate": 855,
    "validation_errors": {
        "err: undefined_instruction": 89507,
        "err: stack_underflow": 12346,
        "err: no_terminating_instruction": 2,
        "err: truncated_instruction": 106,
        "err: invalid_non_returning_flag": 1,
        "err: invalid_container_section_index": 2,
        "err: unreachable_instructions": 203,
        "err: invalid_code_section_index": 1,
        "err: container_size_above_limit": 12,
        "err: max_stack_height_above_limit": 12
    },
    "generation_errors": {
        "unexpected balance for account...": 223,
        "incorrect value in address 0x0...": 9,
        "Account missing from allocatio...": 19,
        "unexpected nonce for account 0...": 9,
        "incorrect value in address 0xc...": 2
    }
}

If one collects the traces with evmone, but modifying it to only trace EOF code execution, then after some sorting/counting (cat trace.log | grep -o '\"opName\"\:\".*\"' | sort | uniq -c), one can get the following summary of the opcodes executed in EOF:

 305517 "opName":"ADD"
    131 "opName":"ADDMOD"
     15 "opName":"ADDRESS"
    105 "opName":"AND"
    185 "opName":"BALANCE"
     24 "opName":"BASEFEE"
     23 "opName":"BLOBBASEFEE"
     34 "opName":"BLOBHASH"
      1 "opName":"BLOCKHASH"
    114 "opName":"BYTE"
  17335 "opName":"CALLDATACOPY"
 151069 "opName":"CALLDATALOAD"
     28 "opName":"CALLDATASIZE"
     30 "opName":"CALLER"
     14 "opName":"CALLVALUE"
      2 "opName":"CHAINID"
     64 "opName":"COINBASE"
      1 "opName":"DATASIZE"
    159 "opName":"DIV"
     51 "opName":"DUP1"
      3 "opName":"DUP10"
      2 "opName":"DUP11"
      2 "opName":"DUP12"
      2 "opName":"DUP13"
      2 "opName":"DUP14"
      2 "opName":"DUP15"
      2 "opName":"DUP16"
     30 "opName":"DUP2"
     22 "opName":"DUP3"
     19 "opName":"DUP4"
     22 "opName":"DUP5"
     17 "opName":"DUP6"
     19 "opName":"DUP7"
     17 "opName":"DUP8"
      2 "opName":"DUP9"
    116 "opName":"EQ"
    883 "opName":"EXP"
     45 "opName":"GASLIMIT"
     32 "opName":"GASPRICE"
    111 "opName":"GT"
     34 "opName":"INVALID"
    105 "opName":"ISZERO"
1193406 "opName":"JUMPDEST"
    265 "opName":"KECCAK256"
     40 "opName":"LOG0"
     40 "opName":"LOG1"
     35 "opName":"LOG2"
     36 "opName":"LOG3"
     31 "opName":"LOG4"
    107 "opName":"LT"
     22 "opName":"MCOPY"
 205043 "opName":"MLOAD"
    112 "opName":"MOD"
     60 "opName":"MSIZE"
 259944 "opName":"MSTORE"
    180 "opName":"MSTORE8"
    551 "opName":"MUL"
    161 "opName":"MULMOD"
    112 "opName":"NOT"
     61 "opName":"NUMBER"
    105 "opName":"OR"
      3 "opName":"ORIGIN"
    693 "opName":"POP"
     30 "opName":"PREVRANDAO"
     26 "opName":"PUSH0"
1708938 "opName":"PUSH1"
      5 "opName":"PUSH10"
      7 "opName":"PUSH11"
      3 "opName":"PUSH12"
     18 "opName":"PUSH13"
     24 "opName":"PUSH14"
      9 "opName":"PUSH15"
    282 "opName":"PUSH16"
      5 "opName":"PUSH17"
     11 "opName":"PUSH18"
      1 "opName":"PUSH19"
 169477 "opName":"PUSH2"
    159 "opName":"PUSH20"
      4 "opName":"PUSH21"
      2 "opName":"PUSH22"
      1 "opName":"PUSH23"
      1 "opName":"PUSH24"
      4 "opName":"PUSH25"
      1 "opName":"PUSH26"
      3 "opName":"PUSH27"
      3 "opName":"PUSH28"
      3 "opName":"PUSH29"
     66 "opName":"PUSH3"
     12 "opName":"PUSH30"
     11 "opName":"PUSH31"
   2280 "opName":"PUSH32"
    341 "opName":"PUSH4"
     18 "opName":"PUSH5"
      8 "opName":"PUSH6"
   1156 "opName":"PUSH7"
    145 "opName":"PUSH8"
      8 "opName":"PUSH9"
 151214 "opName":"RETURN"
      6 "opName":"RETURNDATACOPY"
      2 "opName":"RETURNDATASIZE"
   3664 "opName":"REVERT"
    115 "opName":"SAR"
    119 "opName":"SDIV"
     31 "opName":"SELFBALANCE"
    106 "opName":"SGT"
    232 "opName":"SHL"
    108 "opName":"SHR"
     22 "opName":"SIGNEXTEND"
 100951 "opName":"SLOAD"
    103 "opName":"SLT"
    115 "opName":"SMOD"
 343150 "opName":"SSTORE"
 193260 "opName":"STOP"
    279 "opName":"SUB"
     10 "opName":"SWAP1"
      2 "opName":"SWAP10"
      2 "opName":"SWAP11"
      2 "opName":"SWAP12"
      2 "opName":"SWAP13"
      2 "opName":"SWAP14"
      2 "opName":"SWAP15"
      2 "opName":"SWAP16"
      7 "opName":"SWAP2"
      4 "opName":"SWAP3"
      7 "opName":"SWAP4"
      4 "opName":"SWAP5"
      4 "opName":"SWAP6"
      4 "opName":"SWAP7"
      3 "opName":"SWAP8"
      4 "opName":"SWAP9"
     50 "opName":"TIMESTAMP"
     22 "opName":"TLOAD"
     22 "opName":"TSTORE"
    110 "opName":"XOR"

Open questions

  1. Right now eofwrap takes an opportunistic approach - if the test wraps something and passes, it's worthwhile to wrap and run it. For example, there are tests where it is not the most important code under test being EOF-wrapped, but some auxiliary contract which merely interacts with the important one. Opportunistic here means we still keep this test, to potentially test some edge cases of interactions between legacy and EOF contexts. Other aspect is that there are test categories where many of the fixtures are unwrappable, but several remain. Opportunistic means then that we nevertheless keep the ones which wrapped.
    The opposite side of the spectrum is to be more "systematic", i.e. filter out more test categories where the test is "weaker" and include tests more coarsely, only where we're certain EOF opcodes are executed.
    For now we err on the side of opportunistic, since this is intended to be a low-cost, stop-gap solution, and postpone the systematic effort until proper migration to EEST.
  2. Not sure how would those JSON files be published. One idea is to include an invocation of such eofwrap in the eip7692 prerelease builds, and just include the JSON files in the resulting directory structure. This maximizes the chance that the EVM implementations will actually run these tests, but might come off as messy.
  3. Why use blockchain test format even though those are actually state tests? It includes the detailed post state assertions, which makes it a lot easier to generate meaningful tests (credit to @chfast for this idea)

🔗 Related Issues

NA

✅ 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.

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.

Looks good! I think we could apply this in a post-fill script.

I think @danceratopz might have some comments, and I see one regarding how this could be a pytest plugin, and I agree 😄

src/cli/eofwrap.py Show resolved Hide resolved
src/cli/eofwrap.py Show resolved Hide resolved
src/cli/eofwrap.py Show resolved Hide resolved
src/cli/eofwrap.py Show resolved Hide resolved
src/cli/eofwrap.py Show resolved Hide resolved
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.

Nice idea! I agree with Mario that we could make a pytest plugin out of this 🙂 However, I think we can merge this as-is and then consider whether we do this later.

To explain a little how that could be helpful, you can try out consume direct, which is a small wrapper framework to help execute fixtures against client statetest and blocktest commands. Currently, only geth is supported, but we're currently refactoring this code and plan to add other clients soon. Here's an example:

uv run consume direct --input=../../ethereum/tests/BlockchainTests/GeneralStateTests/Cancun/ --evm-bin=~/bin/evm

All the usual pytest flags are available. For example, tests can be filtered as for fill with -k. The consume command also generates an index file of all the discovered test fixtures - this can be used for subsequent runs, speeding up test discovery.

Of course, if this is more a "single shot" kinda tool, then this CLI could already be sufficient! If we do decide to refactor this to be a pytest plugin, we'd be very happy to take the implementation over or help out. More than likely, we would try to generalize it a bit further in this case.

Anyway, one small comment about this PR below.

src/cli/eofwrap.py Show resolved Hide resolved
src/cli/eofwrap.py Show resolved Hide resolved
@pdobacz pdobacz force-pushed the ethtests-translate branch 4 times, most recently from 32f616d to bf73941 Compare October 18, 2024 12:03
@pdobacz
Copy link
Collaborator Author

pdobacz commented Oct 18, 2024

@marioevz @danceratopz Thank you for your comments, I think I've addressed all except the plugin - would be better to leave for a separate PR indeed. I see the benefits, but not sure if I know where to start, and also it is indeed more of a one-off tool (hopefully temporary, until migration, but I have some level of fear that it will be "permanently-temporary", the migration is a lot to take on).

Do we want to tackle including of these tests in the eip7962 prerelease at this PR? I thought I've seen a comment on this somewhere, but I cannot find it anymore. How can we approach it?

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.

Thanks for the changes, it's looking very good. A couple of comments.

src/cli/eofwrap.py Outdated Show resolved Hide resolved
src/cli/eofwrap.py Outdated Show resolved Hide resolved
src/cli/evm_bytes.py Outdated Show resolved Hide resolved
@pdobacz pdobacz force-pushed the ethtests-translate branch from bf73941 to f7084a2 Compare October 24, 2024 09:54
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.

Looks great! Many thanks for the refactor in evm_bytes.py.

@marioevz marioevz merged commit 496c591 into ethereum:main Oct 25, 2024
3 checks passed
@marioevz marioevz deleted the ethtests-translate branch October 25, 2024 17:55
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.

4 participants