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

fix(tests): EIP-7623 update due to spec change #1071

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jan 13, 2025

🗒️ Description

This PR updates EIP-7623 tests in accordance with #9227.

tests/prague/eip7623_increase_calldata_cost/test_refunds.py

This file introduces tests that verify all types of refunds:

  • Storage Reset.
  • Authorization with Authority already in State.

Including tests that use both refunds at the same time.

Test scenarios include execution gas usage when:

  • Execution gas minus refund is greater than the data floor.
  • Execution gas minus refund equal to the data floor.
  • Execution gas minus refund is less than the data floor.

tests/prague/eip7623_increase_calldata_cost/test_execution_gas.py

Refunds are removed from this file altogether.

🔗 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:refactor Type: Refactor type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Jan 13, 2025
@marioevz marioevz force-pushed the eip-7623-spec-update branch from 2587fbb to c66d709 Compare January 15, 2025 18:26
@marioevz marioevz marked this pull request as ready for review January 15, 2025 18:27
@marioevz
Copy link
Member Author

Rebased after #1068 merge. Ready for review.

@marioevz marioevz force-pushed the eip-7623-spec-update branch from c66d709 to 42d6dee Compare January 21, 2025 21:37
@danceratopz
Copy link
Member

Hey @chfast, could you please help us find the discrepancy in our implementations?

I've filled these tests locally using EELS ethereum/execution-specs@ccb249c, from devnets/prague/5.

  • ethereum/go-ethereum@f24d6f6 from s1na/go-ethereum/tree/prague-devnet-5 passes all state and blockchain tests.
  • ethereum/evmone@6b7305e passes all state tests, but fails
    4 blockchain tests:
    [  FAILED  ] 4 tests, listed below:
    [  FAILED  ] prague/eip7623_increase_calldata_cost/execution_gas.full_gas_consumption
    [  FAILED  ] prague/eip7623_increase_calldata_cost/execution_gas.gas_consumption_below_data_floor
    [  FAILED  ] prague/eip7623_increase_calldata_cost/refunds.gas_refunds_from_data_floor
    [  FAILED  ] prague/eip7623_increase_calldata_cost/transaction_validity.transaction_validity_type_4
    
    Test IDs retrieved from the log:
    tests/prague/eip7623_increase_calldata_cost/test_execution_gas.py::TestGasConsumption::test_full_gas_consumption[fork_Prague-blockchain_test-exact_gas-type_4]
    tests/prague/eip7623_increase_calldata_cost/test_execution_gas.py::TestGasConsumption::test_full_gas_consumption[fork_Prague-blockchain_test-extra_gas-type_4]
    tests/prague/eip7623_increase_calldata_cost/test_transaction_validity.py::test_transaction_validity_type_4[fork_Prague-blockchain_test-type_4-single_authorization-single_access_list_single_storage_key-extra_gas-floor_gas_greater_than_intrinsic_gas]
    tests/prague/eip7623_increase_calldata_cost/test_transaction_validity.py::test_transaction_validity_type_4[fork_Prague-blockchain_test-type_4-single_authorization-single_access_list_single_storage_key-extra_gas-floor_gas_less_than_or_equal_to_intrinsic_gas]
    

fixtures-eip7623.zip

@danceratopz
Copy link
Member

@chfast Ah, could it be due to lack of type 4 support? All the fails use a type-4 transaction.

@chfast
Copy link
Member

chfast commented Jan 23, 2025

@chfast Ah, could it be due to lack of type 4 support? All the fails use a type-4 transaction.

Yes. You can use eip-7702 branch.

@danceratopz
Copy link
Member

@chfast Ah, could it be due to lack of type 4 support? All the fails use a type-4 transaction.

Yes. You can use eip-7702 branch.

Yup, thanks that works! (f2d71302).

All tests pass:

build/bin/evmone-blockchaintest fixtures-eip7623/blockchain_tests
build/bin/evmone-statetest fixtures-eip7623/state_tests/

@danceratopz
Copy link
Member

Nethermind NethermindEth/nethermind@52b679a from pectra-devnet-5 also passes all state and blockchain tests (via nethtest) 🥳

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.

Looks good to me! Although, as @winsvega remarked, these are pretty tough tests to follow.

@marioevz marioevz force-pushed the eip-7623-spec-update branch from 42d6dee to 762c8ee Compare January 23, 2025 15:11
@marioevz marioevz merged commit 4a77afb into main Jan 23, 2025
21 checks passed
@marioevz marioevz deleted the eip-7623-spec-update branch January 23, 2025 15:24
fselmo pushed a commit to fselmo/execution-spec-tests that referenced this pull request Jan 24, 2025
* fix(tests): EIP-7623 update due to spec change

* Add comments
codeofcarson pushed a commit to codeofcarson/execution-spec-tests that referenced this pull request Jan 24, 2025
* fix(tests): EIP-7623 update due to spec change

* Add comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:tests Scope: Changes EL client test cases in `./tests` type:refactor Type: Refactor type:test Type: Add/refactor fw unit tests; no fw or el client test case changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants