Skip to content

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

Merged
marioevz merged 2 commits intomainfrom
eip-7623-spec-update
Jan 23, 2025
Merged

fix(tests): EIP-7623 update due to spec change#1071
marioevz merged 2 commits intomainfrom
eip-7623-spec-update

Conversation

@marioevz
Copy link
Copy Markdown
Member

@marioevz marioevz commented Jan 13, 2025 •
edited by danceratopz
Loading

🗒️ 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
Copy Markdown
Member Author

marioevz commented Jan 15, 2025

Rebased after #1068 merge. Ready for review.

@marioevz marioevz mentioned this pull request Jan 20, 2025
Closed
16 tasks
@marioevz marioevz force-pushed the eip-7623-spec-update branch from c66d709 to 42d6dee Compare January 21, 2025 21:37
winsvega
winsvega reviewed Jan 22, 2025
View reviewed changes
@danceratopz
Copy link
Copy Markdown
Member

danceratopz commented Jan 23, 2025

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
Copy Markdown
Member

danceratopz commented Jan 23, 2025

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

@chfast
Copy link
Copy Markdown
Member

chfast commented Jan 23, 2025 •
edited
Loading

@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
Copy Markdown
Member

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

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
Copy Markdown
Member

danceratopz commented Jan 23, 2025

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

danceratopz
danceratopz approved these changes Jan 23, 2025
View reviewed changes
Copy link
Copy Markdown
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 mentioned this pull request Oct 9, 2025
Open
@marioevz marioevz merged commit 4a77afb into main Jan 23, 2025
@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
@marioevz @fselmo
fix(tests): EIP-7623 update due to spec change (ethereum#1071)
32a44b6 
* 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
@marioevz
fix(tests): EIP-7623 update due to spec change (ethereum#1071)
04dc44a 
* 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

Reviewers

@danceratopz danceratopz danceratopz approved these changes

+1 more reviewer

@winsvega winsvega winsvega left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@marioevz @danceratopz @chfast @winsvega