feat(tests) Precompile Checks #1120

reedsa · 2025-01-23T22:25:28Z

🗒️ Description

Add tests to check each precompile at specific addresses.

Tests from: https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/stPreCompiledContracts/idPrecompsFiller.yml

Removed in:
ethereum/tests#1444

🔗 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: 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

Gave this one a quick look and I wrote one comment.

We also have this similar test over here: https://github.com/ethereum/execution-spec-tests/blob/main/tests/frontier/precompiles/test_precompile_absence.py

marioevz · 2025-01-24T16:47:08Z

tests/precompiles/test_precompiles.py

+    ["address", "exists"],
+    [
+        ("01", True),
+        ("02", True),
+        ("03", True),
+        ("04", True),
+        ("05", True),
+        ("06", True),
+        ("07", True),
+        ("08", True),
+        ("09", True),
+        ("0A", True),
+        ("0B", False),


This list would only be valid for Cancun, but we could use this marker @pytest.mark.parametrize_by_fork to dynamically generate this list by fork.

I would also reduce the amount of cases to the first exists=False fork.

yes. but what if we will have gaps? like new precompiles defined 0x20, 0x21, 0x22 but not at 0x1f, 0x1e

reedsa · 2025-01-24T21:01:33Z

Gave this one a quick look and I wrote one comment.

We also have this similar test over here: https://github.com/ethereum/execution-spec-tests/blob/main/tests/frontier/precompiles/test_precompile_absence.py

Those tests are very similar. Do you think it's still worth migrating these tests?

tests/frontier/precompiles/test_precompiles.py

reedsa · 2025-01-30T20:34:43Z

@winsvega @marioevz now I have the data (precompile address) being sent into the transaction. It looks like all tests are returning 0 however. Is there something I may have missed with the logic in the contract? In the original test, the gas cost difference is found based on a condition that the precompile gas is greater than the gas for the call at the address 0x10000.

tests/frontier/precompiles/test_precompiles.py

winsvega

added comments

reedsa · 2025-02-03T21:32:16Z

@marioevz @winsvega Latest revisions are creating parameterized tests for each fork with valid precompiles as well as a single invalid precompile. I noticed the gas cost is slightly higher than what the original tests were using. Instead of the gas being compared with 0x10 in the original test, it now checks that the cost is less than 0x1A4.

This however fails with Byzantium and Constantinople for precompile 5 and I'm not sure why.

winsvega · 2025-02-03T22:08:21Z

I think the tes was not designed for that forks. Check in ethereum/legacytests if it has a revision for other forks.

But yes, need to understand what's going on

winsvega · 2025-02-06T09:06:37Z

tests/frontier/precompiles/test_precompiles.py

+    """
+    supported_precompiles = fork.precompiles()
+
+    for address in range(1, len(supported_precompiles) + 2):


why +2
and what if precompiles are ranges [1..to 11], [100 to 112]

I can extract this into a variable to describe it a little better. The range is set to include the first address where a precompile does not exist. That will cover what @marioevz mentioned in #1120 (comment)

To support disjointed ranges, perhaps this should be changed to some maximum and ignore most addresses that do not have a supported precompile address. This would effectively be the inverse of the "precompile absence" tests:

execution-spec-tests/tests/frontier/precompiles/test_precompile_absence.py

Line 40 in 093d119

for address in range(1, UPPER_BOUND + 1):

winsvega · 2025-02-06T09:19:06Z

tests/frontier/precompiles/test_precompiles.py

+    gas_10000 = 0x20
+
+    account = pre.deploy_contract(
+        Op.MSTORE(gas_test, Op.GAS)


@marioevz it works Op.GAS as well as Op.GAS() ?

I've seen this in a few places like

execution-spec-tests/tests/osaka/eip7692_eof_v1/gas_test.py

Line 81 in 093d119

Op.GAS

tests/frontier/precompiles/test_precompiles.py

.vscode/launch.json

tests/frontier/precompiles/test_precompiles.py

winsvega

when implementing test for the first time, we, actually take our understanding of vm for granted, but newcomers must use vmtrace. and it always good to use vmtrace to see what is actually happening. (yes by now we have little compiler in our head)
I remember the first year I was using vmtrace all the time, it is essential to get an idea of what is going on and how your code is executed.

saying that we might want to use/develop better tools for vmtrace visualizing and debugging. and manuals.

@marioevz @danceratopz

Contract compares gas cost for each precompile. Only checks a single unsupported precompile address.

winsvega · 2025-02-11T09:05:19Z

tests/frontier/precompiles/test_precompiles.py

+    env = Environment()
+
+    account = pre.deploy_contract(
+        Op.MSTORE(0x00, Op.GAS)


yes now 0x00 can become a variable like

slot_precompile_call_gas = 0x00
slot_empty_address_call_gas = 0x20

it is not a contradiction to previous comment ) this values are reused a lot )

to your liking

I like the descriptive names, good idea!

winsvega · 2025-02-11T09:07:53Z

tests/frontier/precompiles/test_precompiles.py

+        + Op.CALL(address=address)
+        + Op.MSTORE(0x00, Op.SUB(Op.MLOAD(0x00), Op.GAS))
+        + Op.MSTORE(0x20, Op.GAS)
+        + Op.CALL(address=0x10000)


usually we use input 0, length 32 output 32 length 32 or so. thats why ori marked this args as 0xff to not confuse the memory. because call can touch our memory if we are not careful.

Thinking this should be explicit so that defaults are overridden, I probably don't need variables for these though. Is this what you are saying?

Suggested change

+ Op.CALL(address=0x10000)

+ Op.CALL(

address=address,

value=0,

args_offset=0,

args_size=32,

output_offset=32,

output_size=32,

)

winsvega · 2025-02-11T09:13:40Z

tests/frontier/precompiles/test_precompiles.py

+
+    # A high gas cost will result from calling a precompile
+    # Expect 0x00 when a precompile exists at the address, 0x01 otherwise
+    post = {account: Account(storage={0: "0x00" if precompile_exists else "0x01"})}


here I don't clearly understand why abs(empty_call_gas - precompile_call_gas) < 0x1a4 means this logic.
if you can rewrite it more clean like
if precompile_exists then precompile_call_gas is x else y if it is possible.

I'm not clear on this, since the actual gas value varies by precompile call. I'm not sure what logic could be set outside of the contract code. Could you explain a bit more on this?

winsvega · 2025-02-11T09:17:40Z

tests/frontier/precompiles/test_precompiles.py

+        elif num_unsupported > 0:
+            # Check unsupported precompiles up to NUM_UNSUPPORTED_PRECOMPILES
+            yield (hex(address), False)
+            num_unsupported -= 1


what if precompiles are [1-15], [20-25] then 17,18,19 won't be checked for not being a precompile ?
its not the case, just analyzing. I see the original test verified first 0xff addresses. so if a fork will define precompiles with gaps could be an issue here.
I think now we have separate range of special addresses starting from 0x100 too.

Since there are other tests for no precompile in test_precompile_absence, having those checks here as well would be redundant. I think that's why @marioevz said we should just check the first address that a precompile does not exist. I could change the upper bound to 0xff if need be, but I think the fork.precompiles for the forks being checked in this test are mostly just 0x1 - 0x9, and for cancun it includes 0xa as well.

I think now we have separate range of special addresses starting from 0x100 too.

Should the fork that supports this be included here? I don't see those specific ranges in the fork classes.

winsvega · 2025-02-11T09:26:53Z

coverage of the precompiles does not work as its a third party lib in evmone not included in coverage
but still add a link into converted-ethereum-tests.txt

winsvega

I think now it covers the original test. a little optimised with undefined precompiles check.

just a few notes if we can now make it even better

reedsa force-pushed the id-precompiles branch from 4ade831 to ccb6574 Compare January 23, 2025 22:27

marioevz reviewed Jan 24, 2025

View reviewed changes

reedsa force-pushed the id-precompiles branch from ccb6574 to 473e19f Compare January 24, 2025 21:00

reedsa commented Jan 24, 2025

View reviewed changes

tests/frontier/precompiles/test_precompiles.py Outdated Show resolved Hide resolved

reedsa changed the title ~~(feat) Precompile Checks~~ feat(tests) Precompile Checks Jan 28, 2025

reedsa force-pushed the id-precompiles branch 3 times, most recently from dfbf4c6 to f78c534 Compare January 30, 2025 20:30

winsvega reviewed Feb 3, 2025

View reviewed changes

tests/frontier/precompiles/test_precompiles.py Outdated Show resolved Hide resolved

winsvega reviewed Feb 3, 2025

View reviewed changes

tests/frontier/precompiles/test_precompiles.py Outdated Show resolved Hide resolved

winsvega reviewed Feb 3, 2025

View reviewed changes

reedsa force-pushed the id-precompiles branch 3 times, most recently from d2cab4a to e43edba Compare February 3, 2025 21:27

reedsa force-pushed the id-precompiles branch 2 times, most recently from 9372ea0 to cc74bbb Compare February 4, 2025 15:19

reedsa mentioned this pull request Feb 5, 2025

Remove migrated ID Precompile tests ethereum/tests#1444

Open

reedsa marked this pull request as ready for review February 5, 2025 16:41

reedsa requested review from marioevz, winsvega and danceratopz February 5, 2025 16:41

winsvega reviewed Feb 6, 2025

View reviewed changes

tests/frontier/precompiles/test_precompiles.py Outdated Show resolved Hide resolved

winsvega reviewed Feb 6, 2025

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

winsvega reviewed Feb 6, 2025

View reviewed changes

tests/frontier/precompiles/test_precompiles.py Outdated Show resolved Hide resolved

winsvega requested changes Feb 6, 2025

View reviewed changes

Check each fork for supported precompiles.

474d1c1

Contract compares gas cost for each precompile. Only checks a single unsupported precompile address.

reedsa force-pushed the id-precompiles branch from cc74bbb to 474d1c1 Compare February 10, 2025 20:35

winsvega reviewed Feb 11, 2025

View reviewed changes

winsvega self-requested a review February 11, 2025 09:27

winsvega approved these changes Feb 11, 2025

View reviewed changes

Include idPrecomps in converted-ethereum-tests.txt

1cad697

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tests) Precompile Checks #1120

feat(tests) Precompile Checks #1120

reedsa commented Jan 23, 2025 •

edited

Loading

marioevz left a comment

marioevz Jan 24, 2025

winsvega Feb 11, 2025

reedsa commented Jan 24, 2025

reedsa commented Jan 30, 2025

winsvega left a comment

reedsa commented Feb 3, 2025

winsvega commented Feb 3, 2025

winsvega Feb 6, 2025

reedsa Feb 10, 2025

winsvega Feb 6, 2025

reedsa Feb 10, 2025

winsvega left a comment

winsvega Feb 11, 2025 •

edited

Loading

reedsa Feb 11, 2025

winsvega Feb 11, 2025

reedsa Feb 11, 2025

winsvega Feb 11, 2025 •

edited

Loading

reedsa Feb 11, 2025

winsvega Feb 11, 2025 •

edited

Loading

reedsa Feb 11, 2025

winsvega commented Feb 11, 2025 •

edited

Loading

winsvega left a comment

-        + Op.CALL(address=0x10000)
+        + Op.CALL(
+            address=address,
+            value=0,
+            args_offset=0,
+            args_size=32,
+            output_offset=32,
+            output_size=32,
+        )

feat(tests) Precompile Checks #1120

Are you sure you want to change the base?

feat(tests) Precompile Checks #1120

Conversation

reedsa commented Jan 23, 2025 • edited Loading

🗒️ Description

🔗 Related Issues

✅ Checklist

marioevz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedsa commented Jan 24, 2025

reedsa commented Jan 30, 2025

winsvega left a comment

Choose a reason for hiding this comment

reedsa commented Feb 3, 2025

winsvega commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega left a comment

Choose a reason for hiding this comment

winsvega Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega commented Feb 11, 2025 • edited Loading

winsvega left a comment

Choose a reason for hiding this comment

reedsa commented Jan 23, 2025 •

edited

Loading

winsvega Feb 11, 2025 •

edited

Loading

winsvega Feb 11, 2025 •

edited

Loading

winsvega Feb 11, 2025 •

edited

Loading

winsvega commented Feb 11, 2025 •

edited

Loading