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

feat(fixtures,specs,tests): Transaction tests, EIP-7702 convert invalid tx tests #933

Merged
merged 21 commits into from
Dec 5, 2024

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Oct 31, 2024

🗒️ Description

Transaction Test Format

Introduce the Transaction test type to EEST.

TODOs:

  • Correctly calculate the intrinsic gas of the transactions when valid
  • Documentation
  • Migrate some tests from ethereum/tests

At the moment the spec only generates the transaction fixture type, and automatically generating blockchain and engine blockchain tests could be added in the future.

Fixture format type in _info

A new info key fixture_format is added to the _info dictionary in the generated fixtures.

This is done in order to better discriminate between fixture formats, since the addition of the transaction test format was causing issues in checkfixtures command.

Convert EIP-77022 Invalid Transaction Tests

This PR also converts all tests in tests/prague/eip7702_set_code_tx/test_set_code_txs.py to a transaction test, in order to not produce any errors during the parsing of the state tests.

I'm open to leave this out to a different PR to keep the commit granularity.

Fix Transaction(to=Address(0)).to == None

Fixes a bug where a transaction with a destination address meant to be the zero address, ends up as a contract creating transaction instead.

🔗 Related Issues

None

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

@marioevz marioevz added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) labels Oct 31, 2024
@marioevz marioevz marked this pull request as draft October 31, 2024 17:26
@winsvega
Copy link
Contributor

winsvega commented Oct 31, 2024

Look at transaction test filler
It has no pre state.

We used to have t9n in geth to ask the client for transaction test validity

Now we have python implementation of transaction
So we can generate tests no problem.

Basically its like difficulty test. Lets say we test data field.
So we put all other tx fields to be of valid value.

Then we iterate by forks by tx types.
And we iterate data by vector of tests.

Data is empty - tx valid
Data is 00 - valid
Data is 32 random bytes - valid
Data is max_tx_data sise - valid
Max size+1 - invalid
Data size is uint64 +/- 1
Data size is uint128 +/-1
Data size is uint256 +/-1

The exception here is.
Intrinsic tx validation. So tx with gaslimit 0 is a valid structure. But it will be rejected as invalid because we validate intrinsic tx gaslimit rule

In this tests we don't care about pre or post state and out of funds exceptions or out of gas exceptions

Same for gaslimit field.
So I don't see how you want to run it on a bc because you will have to set block gaslimit and account with enough funds.

While transaction structure remains valid.

Also for int fields we test rlp encryption.
Field is 80 field is 6000 field is prefixed by 00(invalid)

This test you can also treat as transaction deserialization tests from rlp. Because we can put an extra field in rlp or remove one. Or mess rlp headers.

@marioevz marioevz force-pushed the transaction-tests branch 2 times, most recently from d1028ea to 61e8fc9 Compare November 6, 2024 16:53
@danceratopz danceratopz added the fork:prague Prague hardfork label Dec 2, 2024
@marioevz marioevz marked this pull request as ready for review December 3, 2024 22:07
@marioevz marioevz requested a review from danceratopz December 3, 2024 22:07
@marioevz
Copy link
Member Author

marioevz commented Dec 3, 2024

Look at transaction test filler It has no pre state.

Regarding this, I added the pre with the execute mode in mind:

We can test sending these faulty transactions to the eth_sendRawTransaction endpoint, but if the account that is recovered from the signature has no funds, we could get a false positive even if a buggy client processes the faulty transaction. Therefore we can use pre.fund_eoa to ensure the origin account always has funds in it.

Copy link
Contributor

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

This will read existing .json files as well?

also can be a good idea to export fields in readable format in the generated fixture json, not just tx bytes.

the purpose of this test is to test rlp decode of txbytes. where txbytes can be ANYTHING
also the test pass if tx gasLimit * gasPrice < 2^256, so a transaction with gasLimit * gasPrice == 2^255 is a valid one. (read from rlp works)
so the pre defenition with this check will not work.

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.

Really nice! Already looks really good, happy to get this merged and iterate further in subsequent PRs.

docs/consuming_tests/index.md Outdated Show resolved Hide resolved
docs/writing_tests/types_of_tests.md Show resolved Hide resolved
src/ethereum_test_fixtures/file.py Outdated Show resolved Hide resolved
src/ethereum_test_fixtures/file.py Outdated Show resolved Hide resolved
src/ethereum_test_specs/transaction.py Outdated Show resolved Hide resolved
@marioevz marioevz changed the title feat(fixtures/specs): Transaction tests feat(fixtures,specs,tests): Transaction tests, EIP-7702 convert invalid tx tests Dec 5, 2024
@marioevz marioevz merged commit 4aa6bc0 into main Dec 5, 2024
5 checks passed
@marioevz marioevz deleted the transaction-tests branch December 5, 2024 18:05
@marioevz marioevz mentioned this pull request Dec 9, 2024
8 tasks
codeofcarson pushed a commit to codeofcarson/execution-spec-tests that referenced this pull request Jan 24, 2025
…id tx tests (ethereum#933)

* new(fixtures): TransactionFixture format

* new(specs): TransactionTest format

* fix(forks): rename intrinsic cost calc parameter name

* fix(tests): rename intrinsic cost calc parameter name

* fix(specs): use `fork` intrinsic gas calculator

* fix(fixtures): Add `TransactionFixture` to file

* fix(fixtures): transaction type optional fields

* feat(fixtures): Add fixture format to `_info` for easier parsing

* fix(cli/check_fixtures): Allow a single fixture check

* fix(types): Address(0) == ""

* fix(fixtures,specs): fixes

* feat(types): Allow nonce list in auth tuple, for testing purposes

* fix(tests): EIP-7702: Convert invalid tx tests

* docs: update, changelog

* fix(docs): tox

* fix(tests): EIP-7702: Move invalid tx tests to its own file

* new(docs): Add Transaction Test to `consuming_tests`

* fix(fixtures): fix `fixture_type_discriminator`

* Update docs/consuming_tests/index.md

Co-authored-by: danceratopz <[email protected]>

* Apply suggestions from code review (fixture_type_discriminator)

Co-authored-by: danceratopz <[email protected]>

* nit

---------

Co-authored-by: danceratopz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fork:prague Prague hardfork scope:fw Scope: Framework (evm|tools|forks|pytest) scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants