Precompile modexp test cases #364

jochem-brouwer · 2023-12-20T14:21:34Z

🗒️ Description

This adds a few test cases for the ModExp precompile introduced in Byzantium.

Tested with: evm version 1.13.4-stable-3f907d6a, solc version 0.8.23+commit.f704f362.Linux.gpp

🔗 Related Issues

Part of these test cases where consensus bugs found in EthereumJS, see the issue: ethereumjs/ethereumjs-monorepo#3168 and the PR: ethereumjs/ethereumjs-monorepo#3169

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

jochem-brouwer · 2023-12-20T14:22:03Z

~~I need some help starting with the Tox validation. Is there a way to auto-fix my files?~~

Fixed!

danceratopz

Hey @jochem-brouwer, thanks for contributing!

Despite draft status, I think it's worth mentioning that a test can't call state_test multiple times within one test function execution (the framework should really test for this and raise an error if this condition is violated - I just created #365 to address this).

Instead, you can parametrize the test using pytest with the test vectors defined in data. Then the test function will get executed multiple times, once for each test vector. See jochem-brouwer#1.

Parametrize test function using pytest.mark.parametrize

…wer/execution-spec-tests into precompile-modexp-test-cases

jochem-brouwer · 2023-12-21T16:50:05Z

@danceratopz Thanks for the follow-up PR 😄

Although I have not added all cases from the EIP test file, I think this is ready for review. If you want, I can also add the remaining tests from the EIP.

jochem-brouwer · 2023-12-21T16:51:13Z

Note: I can not set labels for the PR, and I am assuming that if this gets merged, it will be merged via a squash-and-merge. I have not yet ticked those boxes.

jochem-brouwer · 2023-12-23T20:34:40Z

I have updated and also added the 2 remaining cases in the EIP.

tests/byzantium/precompiles/test_05_modexp.py

jochem-brouwer · 2024-01-04T11:41:52Z

Happy new years everyone 😄

Could someone take a look at this PR? I think it is ready-to-merge, except this spell issue (see my comment here: #364 (comment))

CC @danceratopz @marioevz

danceratopz · 2024-01-04T13:19:43Z

Happy new years everyone 😄

Could someone take a look at this PR? I think it is ready-to-merge, except this spell issue (see my comment here: #364 (comment))

CC @danceratopz @marioevz

Happy New Years @jochem-brouwer! Yup, will do!

danceratopz

Hey @jochem-brouwer sorry for the delay!

This is looking really good, below are a few suggestions.

After you've looked at these comments, I might make a suggestion how to change the test IDs to make them more compact and re-usable in the console. Currently, they're a little bit messy to copy-paste and use them in the console due to the single quotes and spaces:

tests/byzantium/precompiles/test_05_modexp.py::test_modexp[fork_Byzantium-input_['', '', '02']-output_['0x01', '0x01']]
tests/byzantium/precompiles/test_05_modexp.py::test_modexp[fork_Byzantium-input_['', '', '0002']-output_['0x01', '0x0001']]
tests/byzantium/precompiles/test_05_modexp.py::test_modexp[fork_Byzantium-input_['00', '00', '02']-output_['0x01', '0x01']]

This is very pytest specific though, so happy to take care of that, if that's ok.

The other suggestion I'd make would be to move the tests:

From: tests/byzantium/precompiles/test_05_modexp.py
To: tests/byzantium/eip198_modexp_precompile/test_modexp.py

as precompiles were introduced over several forks, for consistency in our repo, I think it's better to organize by the EIP that they were added. We can make discovering and executing precompiles more convenient by adding pytest marks to the tests (e.g., @pytest.mark.precompile_test, see fill --markers, if you you're interested, it's like tagging tests with metadata).

tests/byzantium/precompiles/test_05_modexp.py

docs/CHANGELOG.md

tests/byzantium/precompiles/test_05_modexp.py

Co-authored-by: danceratopz <danceratopz@gmail.com>

jochem-brouwer · 2024-01-10T20:03:54Z

Regarding the test names, do you mean something like this?: 460bf35

(I am resolving the latest comment as we speak 😄 )

jochem-brouwer · 2024-01-10T20:46:50Z

I'm not sure, how do I run evm_bytes_to_python? I can run fill, or order_fixtures, but if I try evm_bytes_to_python then I get command not found. (I have updated my branch to latest main)

jochem-brouwer · 2024-01-10T21:30:42Z

If I rename to fe4c0a8

tests/byzantium/eip198_modexp_precompile/test_modexp.py then I have to add

REFERENCE_SPEC_GIT_PATH = "EIPS/eip-198.md"
REFERENCE_SPEC_VERSION = "9e393a79d9937f579acbdcb234a67869259d5a96"

To the test (which I did). However now the documentation fails to build and I don't know why (just checked, CI passes if I rename it back https://github.com/ethereum/execution-spec-tests/actions/runs/7480915178/job/20361369965?pr=364 ). How should I fix this? It seems it can't find some files

ERROR   -  mkdocstrings: tests.byzantium.eip198_modexp_precompile.test_modexp could not be found

Aborted with a BuildError!
ERROR   -  Error reading page 'tests/byzantium/eip198_modexp_precompile/test_modexp/index.md':
ERROR   -  Could not collect 'tests.byzantium.eip198_modexp_precompile.test_modexp'

But I don't see anything in the other EIP tests which gives me a hunch what the problem could be 🤔

Besides this I think the PR is done :) Have addressed all feedback.

jochem-brouwer · 2024-01-10T21:43:04Z

Never mind, I forgot to rename the __index__.py to the right folder.

@danceratopz ready for re-review 😄

danceratopz

Amazing! This looks really nice! Thanks for all your work on this.

Regarding the test names, do you mean something like this?: 460bf35
Yes that's one approach (and I like it a lot), but I was originally hoping to use auto-generated names, but make them more compact.

In the end I decided to refactor how we write the parameter values using dataclasses in the hopes of making the code more readable. What do you think?

I made two PRs, one with manual ids only (closer to this PR as it currently stands) and one which generates ids from the parameters. We should pick the one we like most and merge that 😺 - let me know what you think.
cc @marioevz @spencer-tb please also say which of the following you like more... I'm really torn 😆 And what do you think about the dataclass usage? I think the overhead makes sense for these tests.

tests/byzantium/eip198_modexp_precompile/test_modexp.py

jochem-brouwer · 2024-01-11T21:02:38Z

I personally like this one better jochem-brouwer#2 since it is more descriptive. (I had thought of creating those manually, but indeed creating those using autogen is better 😄 )

Let me know if you need anything from my side 😄 👍

jochem-brouwer · 2024-01-11T21:15:56Z

Also regarding the new classes: that definitely makes it more readable!

spencer-tb · 2024-01-15T09:47:00Z

Big fan of 2) for the maintainability :D - will be a nice structure to follow for similar tests in the future

refactor(tests): express test parameters as dataclasses (v1 - verbose autogen'd ids with param values)

danceratopz

Thanks again @jochem-brouwer! Nice edition.

Can you just check the comment below? Otherwise, LGTM!

tests/byzantium/eip198_modexp_precompile/test_modexp.py

Co-authored-by: danceratopz <danceratopz@gmail.com>

jochem-brouwer · 2024-01-17T08:21:01Z

@danceratopz I have just manually checked, the EIP has 5 test cases. I manually also checked that the test cases from the EIP match the test cases as written down in this PR. So from my side also: go ahead and merge, thanks a lot 😄 👍

jochem-brouwer added 4 commits December 19, 2023 16:02

add modexp test case

55b5935

fix storage len

40b2192

fix code

d7d3dbe

add cases

1e13d99

make tox happy

dccaec3

jochem-brouwer marked this pull request as draft December 20, 2023 18:13

refactor: use pytest parametrize and TestAddress

c44ea58

danceratopz reviewed Dec 20, 2023

View reviewed changes

jochem-brouwer added 4 commits December 21, 2023 17:45

update changelog

aa3c60f

Merge pull request #1 from danceratopz/modexp-parametrize-test-function

5033153

Parametrize test function using pytest.mark.parametrize

Merge branch 'precompile-modexp-test-cases' of github.com:jochem-brou…

21454cf

…wer/execution-spec-tests into precompile-modexp-test-cases

add evm version

77a6828

jochem-brouwer marked this pull request as ready for review December 21, 2023 16:50

danceratopz added scope:tests Scope: Test cases type:feat type: Feature labels Dec 22, 2023

add remaining EIP changes

40a7586

jochem-brouwer commented Dec 23, 2023

View reviewed changes

tests/byzantium/precompiles/test_05_modexp.py Outdated Show resolved Hide resolved

danceratopz reviewed Jan 10, 2024

View reviewed changes

jochem-brouwer and others added 5 commits January 10, 2024 20:45

apply suggestions

fadd5ae

Merge branch 'main' into precompile-modexp-test-cases

e8c008c

Update tests/byzantium/precompiles/test_05_modexp.py

8ba82f9

Co-authored-by: danceratopz <danceratopz@gmail.com>

apply suggestions

35f1eef

apply test names

460bf35

rename test

fe4c0a8

jochem-brouwer added 4 commits January 10, 2024 22:04

make test more readable

470cd84

add reference files

43fc9f3

test rename back

8b05a47

rename back

ca852ce

rename init.py byzantium tests

da26d1d

refactor(tests): express test parameters as dataclasses

b2da0d7

danceratopz reviewed Jan 11, 2024

View reviewed changes

tests/byzantium/eip198_modexp_precompile/test_modexp.py Outdated Show resolved Hide resolved

danceratopz and others added 4 commits January 16, 2024 10:51

feat(fw): add a base dataclass to auto-generate test ids

8ac3052

feat(tests): use TestParameterGroup to simplify test parameter classes

6af9ecb

docs: update changelog

fb80c48

Merge pull request #2 from danceratopz/refactor/more-readable-params

e0749dd

refactor(tests): express test parameters as dataclasses (v1 - verbose autogen'd ids with param values)

danceratopz approved these changes Jan 16, 2024

View reviewed changes

tests/byzantium/eip198_modexp_precompile/test_modexp.py Outdated Show resolved Hide resolved

Update tests/byzantium/eip198_modexp_precompile/test_modexp.py

98b29a3

Co-authored-by: danceratopz <danceratopz@gmail.com>

danceratopz merged commit 8c7cb87 into ethereum:main Jan 17, 2024
5 checks passed

jochem-brouwer mentioned this pull request Jan 18, 2024

Issue Tracker for Uncovered execution-spec-tests Test Cases ethereumjs/ethereumjs-monorepo#3202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Precompile modexp test cases #364

Precompile modexp test cases #364

jochem-brouwer commented Dec 20, 2023 •

edited

Loading

jochem-brouwer commented Dec 20, 2023 •

edited

Loading

danceratopz left a comment

jochem-brouwer commented Dec 21, 2023

jochem-brouwer commented Dec 21, 2023

jochem-brouwer commented Dec 23, 2023

jochem-brouwer commented Jan 4, 2024

danceratopz commented Jan 4, 2024

danceratopz left a comment •

edited

Loading

jochem-brouwer commented Jan 10, 2024

jochem-brouwer commented Jan 10, 2024

jochem-brouwer commented Jan 10, 2024 •

edited

Loading

jochem-brouwer commented Jan 10, 2024

danceratopz left a comment •

edited

Loading

jochem-brouwer commented Jan 11, 2024

jochem-brouwer commented Jan 11, 2024

spencer-tb commented Jan 15, 2024

danceratopz left a comment

jochem-brouwer commented Jan 17, 2024

Precompile modexp test cases #364

Precompile modexp test cases #364

Conversation

jochem-brouwer commented Dec 20, 2023 • edited Loading

🗒️ Description

🔗 Related Issues

✅ Checklist

jochem-brouwer commented Dec 20, 2023 • edited Loading

danceratopz left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Dec 21, 2023

jochem-brouwer commented Dec 21, 2023

jochem-brouwer commented Dec 23, 2023

jochem-brouwer commented Jan 4, 2024

danceratopz commented Jan 4, 2024

danceratopz left a comment • edited Loading

Choose a reason for hiding this comment

jochem-brouwer commented Jan 10, 2024

jochem-brouwer commented Jan 10, 2024

jochem-brouwer commented Jan 10, 2024 • edited Loading

jochem-brouwer commented Jan 10, 2024

danceratopz left a comment • edited Loading

Choose a reason for hiding this comment

jochem-brouwer commented Jan 11, 2024

jochem-brouwer commented Jan 11, 2024

spencer-tb commented Jan 15, 2024

danceratopz left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Jan 17, 2024

jochem-brouwer commented Dec 20, 2023 •

edited

Loading

jochem-brouwer commented Dec 20, 2023 •

edited

Loading

danceratopz left a comment •

edited

Loading

jochem-brouwer commented Jan 10, 2024 •

edited

Loading

danceratopz left a comment •

edited

Loading