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

Updates to the driver.md GST handler #2682

Merged
merged 13 commits into from
Jan 22, 2025

Conversation

anvacaru
Copy link
Contributor

@anvacaru anvacaru commented Jan 16, 2025

This PR makes a few minor updates to the driver.md and gst_to_kore.py test handlers.

  1. expectException is an optional key for a block in a General State Test used to highlight an invalid block. This field represents the name of the exception which is expected to be thrown when importing this block to the chain.
    Up until now this key was in the discardedKeys set, but GSTs would still fail final assertions. Changes here are added to check if the status code is an ExceptionalStatusCode when expectException is present, discarding the rest of the checks for the current block.
    Other vm implementations follow the same approach:
    py-evm
    rust-evm

  2. Implementation of EIP-3706 which requires the sender of a transaction to be an EOA (externally owned account, without any code deployed).
    For this EIP I added a new rule for loadTx to throw EVMC_FAILURE when the origin of the transaction has deployed code.
    I couldn't find this EIP linked under any hardfork update, however it is present in the Yellowpaper. (under 6. Transaction Execution).

  3. Initial checks for balance underflow, depth exceeded, and nonce exceeded were also omitted from the loadTx rules, which caused a number of tests to fail.

  4. Changed filter_discard_keys to remove discarded keys from the GST file/fixture recursively.

@anvacaru anvacaru changed the title Use expectException to handle intrinsically invalid blocks in GST tests Updates to the driver.md GST handler Jan 17, 2025
@anvacaru anvacaru marked this pull request as ready for review January 17, 2025 17:28
Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

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

LGTM, but I would wait for a second approval.
As we had some conflicts on failing.llvm I should ask if this is expected to really remove so many tests of it?

@anvacaru
Copy link
Contributor Author

Thank you! The conflicts were expected because the previous PR changed failing.llvm.

Most of the tests removed from failing.llvm come from stEIP1559/intrinsic.json and failed because the expectException was not handled.

@automergerpr-permission-manager automergerpr-permission-manager bot merged commit 903e131 into master Jan 22, 2025
12 checks passed
@automergerpr-permission-manager automergerpr-permission-manager bot deleted the expect-exception branch January 22, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants