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.

Sign up for GitHub

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

Jump to bottom

Test EIP-7069 Address Space Extension implications #522

Merged
merged 18 commits into from
Jun 6, 2024
Merged

Test EIP-7069 Address Space Extension implications #522

merged 18 commits into from
Jun 6, 2024

Conversation

shemnon
Copy link
Collaborator

@shemnon shemnon commented Apr 23, 2024 •
edited

🗒️ Description

Validate ASE behaviors on legacy and EOF opcodes for ASE, when targeting
EOAs, Contracts, and empty accounts. Opcodes are EXTCALL/CALL,
EXTDELEGATECALL/DELEGATECALL, EXTSTATICCALL/STATICCALL, and CALLCODE.

🔗 Related Issues

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

@winsvega winsvega self-requested a review April 24, 2024 13:20
@shemnon
Validate EIP-7676 Prepare for Address Space Extension
b005113 
Validate ASE behaviors on legacy and EOF opcodes for ASE, when targeting
EOAs, Contracts, and empty accounts.  Opcodes are BALANCE, EXTCALL/CALL,
EXTDELEGATECALL/DELEGATECALL,  EXTSTATICCALL/STATICCALL, and CALLCODE.

Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
Reviewer requested changes
52be716 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
formatting
f5dd04a 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
speling
b88130d 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon shemnon marked this pull request as ready for review May 9, 2024 19:32
@shemnon
comment out balance from ASE checks
2966ef5 
EIP-7676 moved the critical EXTCALL sections into the main spec.
EOF does not depend on a new BALANCE op, so don't test it.

Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
Merge branch 'main' of github.com:ethereum/execution-spec-tests into …
eb0badf 
…eof/ase
@shemnon
move to new subfolder
6c3a028 
Signed-off-by: Danno Ferrin <danno@numisight.com>
marioevz
marioevz reviewed May 23, 2024
View reviewed changes
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Just a comment about the pattern used to write these tests.

tests/prague/eip7692_eof_v1/eip7676_address_space_extension/test_ase_opcodes.py Outdated Show resolved Hide resolved
@shemnon
Recast as EIP-7069 test
7882ef0 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon shemnon changed the title Validate EIP-7676 Prepare for Address Space Extension Test EIP-7069 Address Space Extension implications May 23, 2024
@shemnon
use itertools.count instead of hard coded address
988996b 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
Copy link
Collaborator Author

shemnon commented May 23, 2024

Since 7979 is out of scope for EOFv1 I migrated this into a 7069 test that only tests the ASE aspects.

marioevz
marioevz reviewed May 28, 2024
View reviewed changes
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

It looks really good overall, but one change I would probably do is:

Instead of doing all types of calls in a single shot, maybe we could simply parameterize for each call opcode.

Reason being that, running the tests like this makes it difficult to single out the failing ones to a single opcode with an incorrect behavior, but if it was parameterized, it would be very easy to do this, even if it comes at the cost of having to run more tests.

tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
@shemnon
Review changes
4ecae63 
* Apply specific edits
* Parameterize by opcode as well

Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
Merge branch 'main' of github.com:ethereum/execution-spec-tests into …
a979246 
…eof/ase
@shemnon
tox fixes
97ed1e1 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
Copy link
Collaborator Author

shemnon commented May 28, 2024

Refactored so it's parameterized on opcode. However the logic to determine the "correct" output is still in place. The other alternative is 7 slightly different versions of the test and parameters, dropping most if the conditional building.

marioevz
marioevz reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Some comments, thanks!

tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
@marioevz
Copy link
Member

marioevz commented Jun 3, 2024

One last comment I forgot in the review (sorry), I think variables with the addresses could be renamed like this:
address_caller -> address_entry_point
address_target -> address_caller

Because, currently, there is target_address but it does not match what is being placed in the pre-alloc IMO.

@shemnon
Merge branch 'main' of github.com:ethereum/execution-spec-tests into …
2a7b7d5 
…eof/ase
@shemnon
remove RETURNDATALOAD from ASE tests
4754460 
Remove RETURNDATALOAD from ASE tests, to be moved to a different test

Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
reviewer comments
338ff16 
Signed-off-by: Danno Ferrin <danno@numisight.com>
marioevz
marioevz reviewed Jun 5, 2024
View reviewed changes
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Some more comments after another review pass.

tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip7069_extcall/test_address_space_extension.py Outdated Show resolved Hide resolved
@shemnon shemnon mentioned this pull request Jun 6, 2024
Open
7 tasks
@shemnon
merge main
53953cc 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
Reviewer comments
95e5bc0 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@shemnon
extreneous changes
8698b1f 
Signed-off-by: Danno Ferrin <danno@numisight.com>
@marioevz marioevz dismissed winsvega’s stale review June 6, 2024 23:15

All changes have been addressed.

marioevz
marioevz approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM.

@marioevz marioevz merged commit 53fc6d8 into ethereum:main Jun 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdobacz pdobacz pdobacz left review comments

@chfast chfast chfast requested changes

@marioevz marioevz marioevz approved these changes

@winsvega winsvega winsvega left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shemnon @marioevz @chfast @winsvega @pdobacz