Skip to content

feat(tests): Identity precompile test conversions #1344

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Mar 24, 2025

🗒️ Description

Identity Precompile test conversions.

🔗 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 identity-precompile branch 2 times, most recently from 3724613 to f7798a5 Compare March 24, 2025 17:15
@reedsa reedsa added type:feat type: Feature port Related to porting ethereum/tests to EEST labels Mar 24, 2025
@reedsa reedsa force-pushed the identity-precompile branch 2 times, most recently from d76ba24 to f7857da Compare March 26, 2025 01:26
@reedsa
Copy link
Contributor Author

reedsa commented Mar 26, 2025

@marioevz @winsvega There are a couple test cases failing, seeing differences in the expectations from the original tests. I have a couple questions about these cases, as follows:

  • The original test for identity_1_nonzero, expects the call to succeed and for the stored value to be 0. However this implementation results in the call failing for some reason.
  • The original test for identity_5 does not include the call result in the expected output. In this case, it looks like the call should revert but expects the stored value to be set as 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF. When running the converted test case I do not see the value being stored. Should the expected result just be 0xDEADBEEF based on this implementation, or is there a better way to capture the original test? Does this happen because the call reverts and the contract code does not complete?

@winsvega
Copy link
Contributor

Have you checked the vmtrace of original tests?

You can run retesteth test.json --vmtrace

You can select vector --singlenet Cancun -d index

@reedsa
Copy link
Contributor Author

reedsa commented Mar 26, 2025

Have you checked the vmtrace of original tests?

You can run retesteth test.json --vmtrace

You can select vector --singlenet Cancun -d index

Yeah the trace for the CALL in identity_5 shows "error" out of gas, but the specified memory is still allocated with that value. I'm wondering if since the call reverts from the error, if these new tests do not end up allocating that memory.

@reedsa reedsa self-assigned this Mar 28, 2025
@reedsa reedsa requested review from marioevz and winsvega March 28, 2025 22:02
@winsvega
Copy link
Contributor

winsvega commented Mar 28, 2025

If a subcall fails with OOG, the upper level call continues, thus stroing all the values, but the failed call returns 0.

Sorry I have not time to review, but I can give hints.

@winsvega
Copy link
Contributor

Ah look

https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/stPreCompiledContracts2/CallIdentity_1_nonzeroValueFiller.json

In original test the pre storage is empty (zero)

Thus if the call fails we don't see it as the storage was not set to deadbeef
Thats why it is good to always set the storage to nonzero value if we expect to verify return code of 0.

@winsvega
Copy link
Contributor

About identity5, if you see the original test makes little sense, it can be errors.

See if the test logic make sense, try to enhance it with more scenarios using pytest.

If you see that original test does not do anything useful, see what was the intent, maybe it can be fixed.

@winsvega
Copy link
Contributor

Fill tests shows much more mismatch in the expectations. Need to check the logic, something is off.

@reedsa reedsa force-pushed the identity-precompile branch 3 times, most recently from e3228c8 to 12035a8 Compare April 4, 2025 20:50
@reedsa
Copy link
Contributor Author

reedsa commented Apr 4, 2025

I realized I needed a different gas_limit for a couple of cases. Those now reside in a separate test.

@reedsa reedsa force-pushed the identity-precompile branch 4 times, most recently from 356e53b to 3b103ad Compare April 4, 2025 23:07
@reedsa reedsa force-pushed the identity-precompile branch from 3b103ad to 1ef4d38 Compare April 4, 2025 23:33
@reedsa
Copy link
Contributor Author

reedsa commented Apr 4, 2025

I was expecting the identity precompile tests to be valid from Frontier but I see failures when I try to fill these. Is there something extra needed to cover these?

The coverage failures look exactly as they did for the blake2 precompile tests. Adding that description to the thread below. #1244 (comment)

Add returndatasize test cases
Add tests for CALL
Add tests for large inputs/outputs
Add CALLCODE tests
@reedsa reedsa force-pushed the identity-precompile branch from 1ef4d38 to fe91892 Compare April 4, 2025 23:41
@reedsa
Copy link
Contributor Author

reedsa commented Apr 7, 2025

Manual Coverage Analysis

/evmone/evmc/include/evmc - helpers.h ✅

     302 LBC           0 :     case EVMC_PRAGUE:
     303               0 :         return "Prague";

Lines lost due to coverage script not filling Prague for python tests, and since latest release of ethereum/tests contains prague, this is expected.

/evmone/test/state - mpt.cpp ✅

      95 LBC           0 :     static MPTNode ext(const Path& path, std::unique_ptr<MPTNode> child) noexcept
      96                 :     {
      97               0 :         assert(child->m_kind == Kind::branch);
      98               0 :         MPTNode node{Kind::ext, path};
      99               0 :         node.m_children[0] = std::move(child);
     100               0 :         return node;

...

     244 LBC           0 :     case Kind::ext:
     245                 :     {
     246               0 :         encoded = rlp::encode(m_path.encode(m_kind)) + encode_child(*m_children[0]);
     247               0 :         break;

Lost due how test accounts (in pre-alloc) are structured in EEST vs ethereum/tests, which is irrelevant.

@reedsa reedsa requested a review from spencer-tb April 7, 2025 18:52
@spencer-tb spencer-tb added the scope:tests Scope: Changes EL client test cases in `./tests` label Apr 14, 2025
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 scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants