Eof/extcall fixes #56

shemnon · 2024-09-13T04:25:33Z

remove unneeded printlns that get in the way of EEST fills
use correct stack heights for value and address in EXTCALL
New extCallGas function that handles min retained and min callee gas
Handle min gas failures via tempCallGas == 0
remove stipend refund for EXT*CALL
Call Stack too deep should return 1
Update aux data handling in EOFCREATE
Add flag to allow EOF from contract creation transactions

Fixes fill of opcode validiiy tests

* use corret stack heights for value and address in EXTCALL * New extCallGas function that handles min retained and min callee gas * Handle min gas failures via tempCallGas == 0 * remove stipend refund for EXT*CALL * Call Stack too deep should return 1

Handle Aux Data in EOF Create w/o validation failures

Allow Create Transactions

Some corner cases for create * Don't consume all gas with an invalid EOF contract * don't allow CREATE/CREATE2 opcodes to create EOF

MariusVanDerWijden · 2024-09-16T07:44:27Z

core/state_transition.go

 		// Special case for EOF, if the initcode or deployed code is
 		// invalid, the tx is considered valid (so update nonce), but
 		// is to be treated as an exceptional abort (so burn all gas).
 		if errors.Is(vmerr, vm.ErrInvalidEOFInitcode) {
-			st.gasRemaining = 0
+			//st.gasRemaining = 0


Are you sure we don't need to burn all gas here?

This looks like a mistaken change. If it should be removed, just remove it, don't just comment it.

But why remove it though?

Step 4 in EIP-7698 - https://eips.ethereum.org/EIPS/eip-7698#specification

If EOF header parsing or full container validation fails, transaction is considered valid and failing. Gas for initcode execution is not consumed, only intrinsic creation transaction costs are charged.

core/vm/evm.go

holiman · 2024-09-16T08:51:01Z

core/vm/eips.go


-	if err == ErrExecutionReverted {
+	if err == ErrExecutionReverted || err == ErrInsufficientBalance || err == ErrDepth {


Better to use errors.Is. But ... do we now have evm instrospection into exactly what type of error that occurs? that sounds crazy

1 if the call has reverted (also can be pushed earlier in a light failure scenario). 2 if the call has failed.

What does failed mean? Doesn't err here always signify that it has reverted?

Generally exit code 1 means something about the VM prevented the call (Insufficient balance, stack to deep, out of gas, other exceptional halts) and exit code 2 means the called code explicitly caused the failure, typically by invoking a REVERT opcode.

If there's a better way to filter this (such as testing for return code 2 and 1 becomes the fall through) let me know, I'm not up to date on all of the geth execution error code names. This also would be a good indicator that we need more reference tests attempting to trigger all the other error cases that could be returned here.

What about precompiles? Our precompiles might return errors that are not caught by this and will return 2, which seems like the wrong approach to me. If this should only happen because of REVERT, than I think we need to explicitly test for REVERT

Generally exit code 1 means something about the VM prevented the call (Insufficient balance, stack to deep, out of gas, other exceptional halts) and exit code 2 means the called code explicitly caused the failure, typically by invoking a REVERT opcode.

Wait isn't it the other way around?

What about precompiles?

I've noticed in the past, that sometimes EIP-authors proposing new precompiles have been used formulations like "throw an error if xx", or "if input is bad, treat as OOG". It's worked out ok, because we haven't differentiated across errors: only "eat all gas or not?".

But now we'll have lots of these "What about [ doesn't exist? precompiles? exited on selfdestruct? precompile arithmetic error? contract violated staticcall? ]"

I think all precompiles would be return code 1 failures, unless it is explicitly attempting to simulate a REVERT operation.

holiman · 2024-09-16T08:54:24Z

core/vm/errors.go

@@ -42,7 +42,7 @@ var (
 	ErrInvalidEOFInitcode       = errors.New("invalid eof initcode")
 	ErrNonceUintOverflow        = errors.New("nonce uint64 overflow")
 	ErrInvalidNumberOfOutputs   = errors.New("invalid number of outputs")
-	ErrInvalidNonReturningFlag  = errors.New("Invalid non-returning flag, bad RETF")
+	ErrInvalidNonReturningFlag  = errors.New("invalid non-returning flag, bad RETF")


Note to marius: this is already fixed in my eof validate

MariusVanDerWijden · 2024-09-16T09:21:52Z

core/vm/validate.go

@@ -155,9 +155,6 @@ func validateCode(code []byte, section int, container *Container, jt *JumpTable,
 			if ct := container.ContainerSections[arg]; len(ct.Data) != ct.DataSize {
 				return nil, fmt.Errorf("%w: container %d, have %d, claimed %d, pos %d", ErrEOFCreateWithTruncatedSection, arg, len(ct.Data), ct.DataSize, i)
 			}
-			if _, ok := visitedSubcontainers[arg]; ok {


Hmm it looks like there is no test in the harness that tests that eofcreating the same subcontainer twice is allowed. I think we should add one (if that is the behavior we want)

I took it out because it was causing a test to fail, but I would say it was causing the failure by side-effect not deliberately and by intent. I will make an explicit test to use EOFCREATE and RETURNCONTRACT of the same subcontainer at multiple points in the code.

This test is re-using the same subcontainer three times, designed to detect address collisions. So it's tested, just not deliberately as a duplicate reference test.

I'm not so sure. When @MariusVanDerWijden took this PR, and remade it against the eof-validation branch, here: https://github.com/holiman/go-ethereum/pull/56/files , one test was affected, but only insofar as going from one type of error into another.

And those test vectors are taken from the execution-spec reference tests.

I added a testvector to execution spec tests that triggers it ethereum/execution-spec-tests#809

The collision test didn't have an explicit eof_test, but was a state_test/blockchaiin_test, and that test failed in the EEST fill command. So it was tested by side effect in the state tests, just not deliberately in the eof_tests, making the error more difficult to find.

Marius's new test makes it explicit in eof_tests in an isolated fashion.

holiman · 2024-09-17T07:00:32Z

core/state_transition.go

 		// Special case for EOF, if the initcode or deployed code is
 		// invalid, the tx is considered valid (so update nonce), but
 		// is to be treated as an exceptional abort (so burn all gas).
 		if errors.Is(vmerr, vm.ErrInvalidEOFInitcode) {
-			st.gasRemaining = 0


Comment still says "is to be treated as an exceptional abort (so burn all gas).". We need to update that (note: I don't really mean to pile more work on @shemnon , just noting it).

holiman · 2024-09-17T07:20:14Z

core/vm/eips.go


-	if err == ErrExecutionReverted {
+	if errors.Is(err, ErrExecutionReverted) || errors.Is(err, ErrInsufficientBalance) || errors.Is(err, ErrDepth) {


I suspect that the error conditions here, and in general having error-introspection, is going to be very problematic going forward.
We've traditionally been pretty hand-wavy about errors, everything is more or less "out of gas error", the exception being revert which doesn't eat all gas. But now all implementations will all of a sudden have to take great care about specific individual errors.

I think that is a mistake. But not really related to this specific PR

Based on the tests there are only two cases return code 2 is used: RETURNDATACOPY failed on a size violation or an EXTSTATICCALL violated the static data. evmone code isn't terribly revelatory.

core/state_transition.go

Co-authored-by: Marius van der Wijden <[email protected]>

MariusVanDerWijden

SGTM, Merging this in now, so we can have a working devnet version.
I'm still a bit hesitant (as is Martin) about the error checks, but we can work on that in a follow up, or once we upstream it to master

@shemnon

…w tests, last fix Changes include: - unexporting errors, removing error-to-code mapping - new test vectors from execution-spec-tests v [email protected] - remaining fix from @shemnon in MariusVanDerWijden#56

@shemnon

…sts, last fix Changes include: - unexporting errors, removing error-to-code mapping - new test vectors from execution-spec-tests v [email protected] - remaining fix from @shemnon in MariusVanDerWijden#56 - core/vm: address review-comments - simplify code - cmd/evm: move eofdump/eofparse into `evm` binary - Also makes eofdump read from stdin if hex is not provided, and makes the output of eofdump a bit more eye-friendly. - core/vm: refactor check in eof control-flow validation - core/vm: refactor some control flow checks for readability

shemnon added 5 commits September 11, 2024 17:30

remove println on bad opcode

8d68a86

Fixes fill of opcode validiiy tests

EXTCALL test fixes

4a4d3b0

* use corret stack heights for value and address in EXTCALL * New extCallGas function that handles min retained and min callee gas * Handle min gas failures via tempCallGas == 0 * remove stipend refund for EXT*CALL * Call Stack too deep should return 1

EOFCREATE validation

389edbd

Handle Aux Data in EOF Create w/o validation failures

Create Transaction

4a93fe9

Allow Create Transactions

formatting

810397f

shemnon force-pushed the eof/extcall-fixes branch from 957f4af to 810397f Compare September 14, 2024 03:40

create transaction

c7efbf4

Some corner cases for create * Don't consume all gas with an invalid EOF contract * don't allow CREATE/CREATE2 opcodes to create EOF

MariusVanDerWijden reviewed Sep 16, 2024

View reviewed changes

core/vm/evm.go Outdated Show resolved Hide resolved

holiman reviewed Sep 16, 2024

View reviewed changes

MariusVanDerWijden reviewed Sep 16, 2024

View reviewed changes

MariusVanDerWijden mentioned this pull request Sep 16, 2024

new(tests): EOF - EIP-7620: EOFCREATE referencing the same subcontainer twice ethereum/execution-spec-tests#809

Merged

8 tasks

cleanup from review comments

12bcb49

holiman reviewed Sep 17, 2024

View reviewed changes

MariusVanDerWijden reviewed Sep 17, 2024

View reviewed changes

core/state_transition.go Outdated Show resolved Hide resolved

shemnon and others added 3 commits September 17, 2024 15:44

Fix dangling data in container for initcode

4392a1c

Update core/state_transition.go

995310c

Co-authored-by: Marius van der Wijden <[email protected]>

don't do beacon chain updates in state tests

740ad8c

MariusVanDerWijden approved these changes Sep 25, 2024

View reviewed changes

MariusVanDerWijden merged commit 2413e46 into MariusVanDerWijden:mega-eof Sep 25, 2024

holiman added a commit to holiman/go-ethereum that referenced this pull request Sep 26, 2024

core/vm: remaining fix from @shemnon in MariusVanDerWijden#56

9465e6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eof/extcall fixes #56

Eof/extcall fixes #56

shemnon commented Sep 13, 2024 •

edited

Loading

MariusVanDerWijden Sep 16, 2024

holiman Sep 16, 2024

shemnon Sep 16, 2024

holiman Sep 16, 2024

holiman Sep 16, 2024

shemnon Sep 16, 2024

shemnon Sep 16, 2024

MariusVanDerWijden Sep 17, 2024

MariusVanDerWijden Sep 17, 2024

holiman Sep 17, 2024 •

edited

Loading

shemnon Sep 17, 2024

holiman Sep 16, 2024

MariusVanDerWijden Sep 16, 2024

shemnon Sep 16, 2024

shemnon Sep 16, 2024

holiman Sep 17, 2024

MariusVanDerWijden Sep 17, 2024

shemnon Sep 17, 2024

holiman Sep 17, 2024

holiman Sep 17, 2024

shemnon Sep 17, 2024 •

edited

Loading

MariusVanDerWijden left a comment


		if err == ErrExecutionReverted {
		if err == ErrExecutionReverted \|\| err == ErrInsufficientBalance \|\| err == ErrDepth {


		if err == ErrExecutionReverted {
		if errors.Is(err, ErrExecutionReverted) \|\| errors.Is(err, ErrInsufficientBalance) \|\| errors.Is(err, ErrDepth) {

Eof/extcall fixes #56

Eof/extcall fixes #56

Conversation

shemnon commented Sep 13, 2024 • 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

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

Choose a reason for hiding this comment

holiman Sep 17, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

MariusVanDerWijden left a comment

Choose a reason for hiding this comment

shemnon commented Sep 13, 2024 •

edited

Loading

holiman Sep 17, 2024 •

edited

Loading

shemnon Sep 17, 2024 •

edited

Loading