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(tests) Additional precompile tests for blake2b, identity #1244

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Feb 21, 2025

πŸ—’οΈ Description

Convert remaining Blake2 precompile test cases for CALL and DELEGATECALL

πŸ”— Related Issues

#972 (stPreCompiledContracts / stPreCompiledContracts2)

πŸ—‘οΈ ethereum/tests removals:

βœ… 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.

@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch 2 times, most recently from 19ae03c to b10bd8f Compare February 21, 2025 21:53
Copy link
Contributor Author

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

@marioevz @winsvega I'm working on several more precompile tests and double checking that all cases are covered.

  • Blake2b should be covered with these additions, but one case is causing timeouts (see separate comment below).
  • This PR includes a couple Identity test cases. There are numerous others for CALL and CALLCODE that I will add here.

I want to clarify the other test cases since you mentioned skipping a few from the stPreCompiledContracts2 directory.

@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch 2 times, most recently from 5c29b09 to 7882fc1 Compare February 28, 2025 02:48
@reedsa reedsa changed the title (feat) Additional precompile tests for blake2b, identity feat(tests) Additional precompile tests for blake2b, identity Mar 12, 2025
@winsvega
Copy link
Contributor

add tests to converted file to run check the coverage

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.

add converted-tests .txt records to check run the coverage

@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch 2 times, most recently from 60723e8 to 3be54a3 Compare March 20, 2025 22:50
@reedsa reedsa added the port Related to porting ethereum/tests to EEST label Mar 20, 2025
@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch from e2c0180 to f13de88 Compare March 21, 2025 20:00
@reedsa
Copy link
Contributor Author

reedsa commented Mar 21, 2025

@winsvega @marioevz Coverage added and failing from the two cases that I have commented out due to the extreme number of rounds used. The test hangs and times out so I would guess we can ignore those. Is there a way to explicitly flag those cases so the coverage report passes?

@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch 4 times, most recently from 8972e36 to 1c0bbc3 Compare March 24, 2025 17:14
@reedsa reedsa added the type:feat type: Feature label Mar 24, 2025
Add blake2 coverage for CALL, DELEGATECALL, CALLCODE tests
Update blake2 REFERENCE_SPEC_VERSION to correct value
@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch from abd4708 to 30b1070 Compare March 24, 2025 22:41
… rounds

Timeout fix for CALLCODE blake2 tests
@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch 2 times, most recently from d2e792f to 630687f Compare March 25, 2025 17:41
@marioevz
Copy link
Member

Added a line to converted-ethereum-tests.txt to trigger coverage run again πŸ‘

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.

Hey, some comments that would make use of some pytest features to remove some code duplication. Thanks!

@reedsa reedsa force-pushed the eip152-blake2-delegatecall branch from e975154 to 14d6f53 Compare March 25, 2025 22:02
@reedsa
Copy link
Contributor Author

reedsa commented Mar 25, 2025

Test coverage is failing due to one case being skipped in test_blake2b_large_gas_limit due to timeouts. delegatecall coverage has also changed due to differences in setup, specifically with preallocations of accounts in the original tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port Related to porting ethereum/tests to EEST type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants