Skip to content

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.
  • ipsilon/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
kclowes pushed a commit to kclowes/execution-spec-tests that referenced this pull request Oct 20, 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.

5 participants