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

Introduces EVM version Osaka #15830

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Introduces EVM version Osaka #15830

merged 1 commit into from
Feb 7, 2025

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Feb 4, 2025

Depends on #15829 Merged

@r0qs r0qs added the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2025
@nikola-matic nikola-matic force-pushed the bump-evmone-ci branch 2 times, most recently from 3a5447d to 49e2433 Compare February 5, 2025 18:29
@r0qs r0qs force-pushed the add-evm-version-osaka branch from 7473b22 to 1fa5cdf Compare February 6, 2025 01:13
@r0qs r0qs marked this pull request as ready for review February 6, 2025 01:13
Base automatically changed from bump-evmone-ci to develop February 6, 2025 09:07
@nikola-matic nikola-matic force-pushed the add-evm-version-osaka branch from 1fa5cdf to 94b94db Compare February 6, 2025 09:17
@@ -182,6 +182,7 @@ at each version. Backward compatibility is not guaranteed between each version.
- Opcode ``mcopy`` is available in assembly (see `EIP-5656 <https://eips.ethereum.org/EIPS/eip-5656>`_).
- Opcodes ``tstore`` and ``tload`` are available in assembly (see `EIP-1153 <https://eips.ethereum.org/EIPS/eip-1153>`_).
- ``prague`` (**experimental**)
- ``osaka``
Copy link
Member

@clonker clonker Feb 6, 2025

Choose a reason for hiding this comment

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

I guess this should be marked experimental? (ping @nikola-matic)

Copy link
Member

@cameel cameel Feb 6, 2025

Choose a reason for hiding this comment

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

It definitely should.

Also, one thing I noticed recently, that we might want to fix (though maybe in a separate PR) is that we do have an experimental flag in metadata, but we do not set it for experimental EVM versions (only for some other experimental features, including EOF). So bytecode compiled for Prague is not currently marked as experimental.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's marked as experimental in the other PR (the one that moves EOF tests to Osaka) - https://github.com/ethereum/solidity/pull/15828/files#diff-c907e10ad187a2840ede6859989664668693753e8c7d860fdc1b60275e39f053R185

Not sure why it was done there; I also have a sneaking suspicion that this PR will start failing as soon as this PR and the linked one above are rebased, since Kamil said that we've introduced semantic tests for EOF, whereas evmone 13 (latest and now merged) release did not move the EOF implementation to Osaka.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then again, this PR is from 2 days ago - so not sure when the semantic tests PR was merged.

Copy link
Member

Choose a reason for hiding this comment

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

On Monday: #15666.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why wouldn't they be able to run? They are not marked as requiring Prague specifically.

I'm talking about semantic tests that are explicitly marked as EOF and thus requiring Prague (not even sure if we have such tests); if we do have such tests, bumping them to require Osaka, when the libevmone in our test backend does not yet have EOF in Osaka, would make the tests fail, no?

Copy link
Member

Choose a reason for hiding this comment

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

bumping them to require Osaka, when the libevmone in our test backend does not yet have EOF in Osaka, would make the tests fail, no?

Ah, that's what you mean. No, you don't have to worry about it. You can safely assume that it does support EOF in Osaka as long as it allows selecting Osaka at all (and it does since evmone 12 uses evmc 12, which introduced an enum value for it). Osaka by default includes everything from Prague so they'd have to go out of their way to make it not support EOF.

The thing it does not have yet is to disallow EOF features in Prague. But that's not a problem for us.

I'm talking about semantic tests that are explicitly marked as EOF and thus requiring Prague (not even sure if we have such tests);

We do have a few - those added by #15665. They were added specifically in cases where some of the existing tests did not pass on EOF as is due to differences in behavior and we needed a separate EOF variant.

Though note that they don't even require bumping to Osaka. We have a separate setting to mark them as requiring EOF (bytecodeFormat: >=EOFv1) and we only run them on EVM versions that satisfy it.

Well, we do seem to still have a few tests with EVMVersion: >=prague in them, but that has always been redundant and should be removed.

Copy link
Member

Choose a reason for hiding this comment

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

In any case @clonker's comment is still relevant - the comments about it being experimental should be added already here, not in #15830.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I've pushed the requested changes already. Can you take another look, and if it's OK, I'll merge it later today (have to leave now).

Copy link
Member

Choose a reason for hiding this comment

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

BTW, I also added an issue to fix the missing experimental flag: #15835.

@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Feb 6, 2025
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Just to things to reduce diff noise.

@nikola-matic nikola-matic force-pushed the add-evm-version-osaka branch from 94b94db to 9e9f40e Compare February 6, 2025 14:53
@nikola-matic nikola-matic force-pushed the add-evm-version-osaka branch from 9e9f40e to 6b4ee52 Compare February 6, 2025 18:42
@nikola-matic nikola-matic merged commit 9ff6d26 into develop Feb 7, 2025
74 checks passed
@nikola-matic nikola-matic deleted the add-evm-version-osaka branch February 7, 2025 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants