Skip to content

Update ethereum-tests to v15.0#3871

Merged
jochem-brouwer merged 9 commits into
masterfrom
tests-v15
Feb 21, 2025
Merged

Update ethereum-tests to v15.0#3871
jochem-brouwer merged 9 commits into
masterfrom
tests-v15

Conversation

@jochem-brouwer

@jochem-brouwer jochem-brouwer commented Feb 14, 2025

Copy link
Copy Markdown
Member

@jochem-brouwer jochem-brouwer added PR state: needs review type: tests package: monorepo type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. labels Feb 14, 2025
@codecov

codecov Bot commented Feb 14, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.87%. Comparing base (58ac923) to head (73cd8dc).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 76.87% <ø> (ø)
blockchain 85.69% <ø> (ø)
client 66.28% <ø> (ø)
common 90.72% <ø> (ø)
devp2p 76.27% <ø> (ø)
ethash 81.04% <ø> (ø)
evm 71.04% <ø> (+1.38%) ⬆️
genesis 99.84% <ø> (ø)
mpt 59.72% <ø> (-0.24%) ⬇️
rlp 69.70% <ø> (ø)
statemanager 70.47% <ø> (ø)
tx 80.96% <ø> (ø)
util 85.54% <ø> (ø)
vm 57.81% <ø> (ø)
wallet 83.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ScottyPoi

ScottyPoi commented Feb 17, 2025

Copy link
Copy Markdown
Contributor

There is no longer a test file at .../EOFTests/EIP4200/
However there is a test file that we currently don't access at .../EOFTests/ori/

For whatever reason, I can't find where in the commit history EIP4200 was deleted. Wondering if it was replaced by this ori test? Or if that is something else entirely?

@jochem-brouwer

Copy link
Copy Markdown
Member Author

Hi, thanks for taking over :)

To find the deletion:

cd ./packages/ethereum-tests (ensure you are at v15.0)

git log --diff-filter=D --summary -- ./EOFTests/EIP4200/validInvalid.json

commit ae4791077e8fcf716136e70fe8392f1a1f1495fb
Author: Paweł Bylica <pawel@ethereum.org>
Date:   Wed Dec 18 21:51:27 2024 +0100

    remove migrated EOF RJUMP validation tests

 delete mode 100644 EOFTests/EIP4200/validInvalid.json

ethereum/tests@ae47910

@jochem-brouwer

Copy link
Copy Markdown
Member Author

I'm fairly sure the idea is that all ethereum-tests migrate to execution-spec-tests. For the deletion PR see ethereum/tests#1421

@jochem-brouwer

Copy link
Copy Markdown
Member Author

The tests for specific EIP-4200 should be removed, but the test runner should run all header validation tests in the ./EOFTests folder.

@ScottyPoi

Copy link
Copy Markdown
Contributor

To find the deletion:

cd ./packages/ethereum-tests (ensure you are at v15.0)

git log --diff-filter=D --summary -- ./EOFTests/EIP4200/validInvalid.json

Ah, sweet thanks!

@jochem-brouwer jochem-brouwer marked this pull request as draft February 17, 2025 23:35
@jochem-brouwer jochem-brouwer marked this pull request as ready for review February 17, 2025 23:41
@jochem-brouwer

Copy link
Copy Markdown
Member Author

EVM now runs all container tests in ethereum-tests/EOFTests on CI now. Ready for review & merge 😄 👍

Thanks @ScottyPoi 😄

Comment thread packages/evm/src/eof/errors.ts Outdated
Code0Outputs = 'first code section should have 0x80 (terminating section) outputs',
MaxStackHeight = `expected maxStackHeight`,
MaxStackHeightLimit = `stack height limit of 1024 exceeded: `,
MaxStackHeightLimit = `EOF_MaxStackHeightExceeded`,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wait, is this necessary?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This the exception as it's written in the test. Do we have another way of validating that the correct error was thrown?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah right I see. We don't actually validate these errors (that these match the expected errors). For these specific tests we could maybe do so though, but in general the specification does not specify in which order certain errors have to be checked. So for instance a spec could say:

The EVM should check A and B.

For implementation purposes (like readability or optimizations) we could decide to check B before A, and this could thus yield a different error.

Also note that these errors will be thrown by the EVM. If we want to test those errors we should setup an error map which maps the expected error (so for instance EOF_StackUnderflow) to the error string we throw (for instance: stack underflow).

I just saw you added examples, we should likely investigate these (although I think that we throw the correct error on the correct place, but maybe the error string we throw is a bit misleading/faulty)?

Would be great to add :) Will leave this PR open.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, we could do the mapping of EOF_ErrorName to our error messages and leave our errors as they are

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not so simple actually -- see more examples below

@ScottyPoi ScottyPoi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the EOF validation tests, we don't actually compare the _exception value from the test files to the error message, we're just confirming that an error is thrown.

If we do compare the _exception value, we get a bunch of errors. Mostly this is just semantic, but a few of them seem suspicious, like maybe we are throwing the wrong error.

@ScottyPoi

Copy link
Copy Markdown
Contributor

For example -- we give the same error on these two tests, but the expected errors are different according to the test files

Screenshot from 2025-02-17 17-22-57
Screenshot from 2025-02-17 17-22-51

// Rename this test dir to the location of EOF header tests
// To test, use `npx vitest run ./scripts/eof-header-validation.spec.ts
const testDir = path.resolve('../ethereum-tests/EOFStuff/fixtures/eof_tests')
const testDir = path.resolve('../ethereum-tests/EOFTests')

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah right this file should be disabled for browser tests

@ScottyPoi

Copy link
Copy Markdown
Contributor

Here's another example -- where the EOF_Error is more specific than the error that we throw:

Screenshot from 2025-02-17 17-41-28
Screenshot from 2025-02-17 17-41-19

@ScottyPoi

Copy link
Copy Markdown
Contributor

Then, here, the other way around. We give two different errors for the same expected exception:

Screenshot from 2025-02-17 17-48-39
Screenshot from 2025-02-17 17-41-19

@ScottyPoi ScottyPoi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Will address Error message testing in future PR

@jochem-brouwer jochem-brouwer merged commit cfc5309 into master Feb 21, 2025
@acolytec3 acolytec3 deleted the tests-v15 branch February 21, 2025 00:55
@ScottyPoi ScottyPoi mentioned this pull request Feb 21, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: monorepo PR state: needs review type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. type: tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants