EOF: Implement `ext*calls` #15559

rodiazet · 2024-10-31T15:25:20Z

~~Depends on #15620~~ Merged.
~~Depends on #15626~~ Merged.

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

test/libyul/yulSyntaxTests/eof/extcalls_invalid_in_legacy.yul

cameel

There are a few things that need correcting: memory side-effects, non-existent ext members on address, probably unnecessary check for new opcodes outside of EOF. We also have no analysis checks against passing gas into an external call.

Also, tests are failing.

cameel · 2024-12-05T13:51:35Z

libyul/AsmAnalysis.cpp

+				"The \"{instruction}\" instruction is {kind} VMs.",
+				fmt::arg("instruction", boost::to_lower_copy(instructionInfo(_instr, m_evmVersion).name)),
+				fmt::arg("kind", "not available in legacy bytecode")


Is there any good reason to have an explicit check for specifically these three instructions? We did not do that for the ones we added earlier. Instead we just excluded them from dialect, so that they don't have corresponding builtins and their names still remain usable.

Because these three opcodes are available in yul (EOF) as regular instructions. Other instructions I added are buildin functions or are unavailable in yul. If we don't add the sanity check fails

I mean all added opcodes were till now buildin or are disabled in dialect.

Ah, right. I'm still confusing this bit with the code responsible for making sure instructions are available or not on EOF/legacy. But this only kicks in when we would get an error anyway and just replaces the generic "function not found" with a more specific message explaining why it was not available.

So this is correct, it's the other way around - the dialect builtins should report these specialized messages too if we wanted to be fully consistent.

But while thinking about it I realized we're missing a check elsewhere and a very important one - we're missing the special-casing for the EXT*CALL instructions on legacy. They should be marked as non-reserved in EVMDialect::createReservedIdentifiers(). Without it, this is breaking for legacy Yul.

And it's actually not just this PR, we're missing such exceptions for nearly all the instructions we added so far. We did that only for auxdataloadn, but then we forgot dataloadn, which gets added automatically as all Instructions are.

This thing is just too error-prone and we keep realizing we're missing something there (#15550 (comment)). I think that instead of testing it ad-hoc for every new opcode/builtin, we should create a few comprehensive test cases that cover all of them and just keep them updated with new PRs.

We basically need 4 tests. Each one should cover all EOF-only opcodes and all EOF-only builtins and check that:

The name is not reserved on legacy (can declare variables and functions with that name). Covers createReservedIdentifiers().

The name is reserved on EOF.

The name is not defined on legacy (calling that name will not execute a Yul builtin). Covers createBuiltins().

If it works on EOF, we get a specialized error saying it's only available on EOF (rather than a generic error about calling undefined function). Covers validateInstructions() we have here.

It either works on EOF or is not defined (depending on the case).

This can of course be done in a separate PR not to hold this one up. In this one just make sure that these things work correctly for EXT*CALL instructions.

Addressed. I created these tests covering ext*call in this PR. They should be extended to all added EOF instructions

libyul/AsmAnalysis.cpp

libevmasm/SemanticInformation.cpp

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

test/libyul/yulSyntaxTests/eof/extcalls.yul

cameel · 2024-12-05T16:00:31Z

I think it would be good to enable a few related semantic tests already in this PR. Some external calls and the low-level calls on address. Otherwise we have zero coverage for the codegen changes here and you will have to deal with them all at once when we enable all semantic tests.

cameel · 2024-12-05T16:05:17Z

Also, a question about the EOF spec. It says:

NOTE: The replacement instructions EXT*CALL continue being treated as undefined in legacy code.

Does it simply mean that the opcodes are treated as INVALID on legacy? Isn't this the case for all newly introduced opcodes? I have an impression that there might be more to this than that because otherwise why call out specifically EXT*CALL?

rodiazet · 2024-12-06T13:54:19Z

Test are failing because they depend on other PR with stack height calculation. I can regenerate them but I would rather wait until it's merged.

rodiazet · 2024-12-06T14:32:01Z

I think it would be good to enable a few related semantic tests already in this PR. Some external calls and the low-level calls on address. Otherwise we have zero coverage for the codegen changes here and you will have to deal with them all at once when we enable all semantic tests.

I have already a version with all semantic tests passing. It required three small fixes but in this case I would not bother now. Moreover we need to introduce Osaka to Solidity to run semantic tests with the newest evmone now.

gumb0 · 2024-12-06T14:33:49Z

Also, a question about the EOF spec. It says:

NOTE: The replacement instructions EXT*CALL continue being treated as undefined in legacy code.

Does it simply mean that the opcodes are treated as INVALID on legacy? Isn't this the case for all newly introduced opcodes? I have an impression that there might be more to this than that because otherwise why call out specifically EXT*CALL?

It's the same as with all other newly introduced opcodes.

Original plan was to enable them also for legacy, then it changed, and this explicit note added to point out the change.

libyul/AsmAnalysis.cpp

cameel

The main remaining thing is #15559 (comment), which I missed previously. I.e. extcall opcodes must not be reserved identifiers on legacy + a test confirming it works correctly by defining a function/variable with that name.

The same thing needs to be fixed for other opcodes, but that should be done separately, since that's already broken on develop.

I have already a version with all semantic tests passing. It required three small fixes but in this case I would not bother now. Moreover we need to introduce Osaka to Solidity to run semantic tests with the newest evmone now.

Well, if you prefer we can merge without it, but I'd still recommend enabling at least one just to confirm that late changes in the PR did not break anything.

rodiazet · 2024-12-09T10:56:20Z

The main remaining thing is #15559 (comment), which I missed previously. I.e. extcall opcodes must not be reserved identifiers on legacy + a test confirming it works correctly by defining a function/variable with that name.

The same thing needs to be fixed for other opcodes, but that should be done separately, since that's already broken on develop.

I have already a version with all semantic tests passing. It required three small fixes but in this case I would not bother now. Moreover we need to introduce Osaka to Solidity to run semantic tests with the newest evmone now.

Well, if you prefer we can merge without it, but I'd still recommend enabling at least one just to confirm that late changes in the PR did not break anything.

I enabled one but to be able to run any semantic test we need to first merge this #15626 or #15606

github-actions bot added the external contribution ⭐ label Oct 31, 2024

This comment was marked as resolved.

Sign in to view

rodiazet force-pushed the eof-extcalls branch 2 times, most recently from 4e8c5c1 to 626388d Compare October 31, 2024 15:27

cameel added has dependencies The PR depends on other PRs that must be merged first EOF labels Nov 1, 2024

rodiazet force-pushed the eof-extcalls branch from 626388d to 8ecb35d Compare November 21, 2024 13:13

rodiazet marked this pull request as ready for review November 21, 2024 13:13

shemnon mentioned this pull request Nov 27, 2024

EOF Implementers Call #62 ethereum/pm#1192

Closed

rodiazet force-pushed the eof-extcalls branch 4 times, most recently from b1cc886 to fbc76c7 Compare December 2, 2024 09:55

axic reviewed Dec 2, 2024

View reviewed changes

libsolidity/codegen/ir/IRGeneratorForStatements.cpp Show resolved Hide resolved

rodiazet force-pushed the eof-extcalls branch 3 times, most recently from aa4501a to ea37148 Compare December 3, 2024 14:26

This comment was marked as resolved.

Sign in to view

rodiazet force-pushed the eof-extcalls branch from ea37148 to 216792c Compare December 4, 2024 10:52

cameel removed the has dependencies The PR depends on other PRs that must be merged first label Dec 5, 2024

cameel reviewed Dec 5, 2024

View reviewed changes

test/libyul/yulSyntaxTests/eof/extcalls_invalid_in_legacy.yul Outdated Show resolved Hide resolved

cameel requested changes Dec 5, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

rodiazet force-pushed the eof-extcalls branch 2 times, most recently from 63c2be6 to 45df930 Compare December 6, 2024 15:22

cameel reviewed Dec 6, 2024

View reviewed changes

libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved

cameel mentioned this pull request Dec 6, 2024

eof: Fix legacy staticcall error for EOF #15620

Merged

cameel reviewed Dec 6, 2024

View reviewed changes

rodiazet force-pushed the eof-extcalls branch 2 times, most recently from fe57558 to 7cc0877 Compare December 9, 2024 10:35

rodiazet force-pushed the eof-extcalls branch 2 times, most recently from 5d5931c to 82c21ea Compare December 9, 2024 16:41

cameel approved these changes Dec 9, 2024

View reviewed changes

cameel added has dependencies The PR depends on other PRs that must be merged first and removed has dependencies The PR depends on other PRs that must be merged first labels Dec 9, 2024

rodiazet added 4 commits December 9, 2024 22:34

eof: Introduce new instruction set for ext*call

2ae6807

eof: Use EOF ext*call in yul generator for statements.

f495f67

eof: Add unit tests in yulSyntaxTests and objectCompiler

6520bfd

eof: Enable one semantic test to sanity check.

6263f56

rodiazet force-pushed the eof-extcalls branch from 82c21ea to 6263f56 Compare December 9, 2024 21:34

cameel enabled auto-merge December 9, 2024 21:37

cameel merged commit a62a455 into ethereum:develop Dec 9, 2024
73 checks passed

rodiazet deleted the eof-extcalls branch December 10, 2024 09:47

This was referenced Dec 20, 2024

eof: Update yulSyntaxTests tests for EOF #15660

Open

Some EOF-specific Yul builtin names are reserved even outside of EOF #15672

Open

Refine and Stabilize EOF Support #15310

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOF: Implement `ext*calls` #15559

EOF: Implement `ext*calls` #15559

rodiazet commented Oct 31, 2024 •

edited by cameel

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

cameel left a comment •

edited

Loading

cameel Dec 5, 2024

rodiazet Dec 6, 2024

rodiazet Dec 6, 2024

cameel Dec 6, 2024 •

edited

Loading

cameel Dec 6, 2024 •

edited

Loading

cameel Dec 6, 2024 •

edited

Loading

rodiazet Dec 9, 2024 •

edited

Loading

cameel commented Dec 5, 2024

cameel commented Dec 5, 2024

rodiazet commented Dec 6, 2024 •

edited

Loading

rodiazet commented Dec 6, 2024

gumb0 commented Dec 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

cameel left a comment •

edited

Loading

rodiazet commented Dec 9, 2024 •

edited

Loading

EOF: Implement ext*calls #15559

EOF: Implement ext*calls #15559

Conversation

rodiazet commented Oct 31, 2024 • edited by cameel Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel Dec 5, 2024

Choose a reason for hiding this comment

rodiazet Dec 6, 2024

Choose a reason for hiding this comment

rodiazet Dec 6, 2024

Choose a reason for hiding this comment

cameel Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

cameel Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

cameel Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

rodiazet Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

cameel commented Dec 5, 2024

cameel commented Dec 5, 2024

rodiazet commented Dec 6, 2024 • edited Loading

rodiazet commented Dec 6, 2024

gumb0 commented Dec 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

cameel left a comment • edited Loading

Choose a reason for hiding this comment

rodiazet commented Dec 9, 2024 • edited Loading

EOF: Implement `ext*calls` #15559

EOF: Implement `ext*calls` #15559

rodiazet commented Oct 31, 2024 •

edited by cameel

Loading

cameel left a comment •

edited

Loading

cameel Dec 6, 2024 •

edited

Loading

cameel Dec 6, 2024 •

edited

Loading

cameel Dec 6, 2024 •

edited

Loading

rodiazet Dec 9, 2024 •

edited

Loading

rodiazet commented Dec 6, 2024 •

edited

Loading

cameel left a comment •

edited

Loading

rodiazet commented Dec 9, 2024 •

edited

Loading