core/vm: Make INVALID a defined opcode #24017

gumb0 · 2021-11-29T13:28:53Z

This doesn't change anything except error message, ~~but will be required for EOF code calidation (EIP-3670)~~ It doesn't solve the needs of EIP-3670 anymore (because undefined and INVALID are not distinguished in jump table), but does improve the traces.

Before:

> build/bin/evm --code "0xfe" --json --debug run
{"pc":0,"op":254,"gas":"0x2540be400","gasCost":"0x0","memory":"0x","memSize":0,"stack":[],"returnData":"0x","depth":1,"refund":0,"opName":"opcode 0xfe not defined","error":"invalid opcode: opcode 0xfe not defined"}
{"output":"","gasUsed":"0x2540be400","time":132537,"error":"invalid opcode: opcode 0xfe not defined"}
#### LOGS ####

After:

> build/bin/evm --code "0xfe" --json --debug run
{"pc":0,"op":254,"gas":"0x2540be400","gasCost":"0x0","memory":"0x","memSize":0,"stack":[],"returnData":"0x","depth":1,"refund":0,"opName":"INVALID","error":""}
{"output":"","gasUsed":"0x2540be400","time":119327,"error":"invalid opcode: INVALID"}
#### LOGS ####

axic · 2021-11-29T15:57:03Z

core/vm/errors.go

@@ -36,6 +36,7 @@ var (
 	ErrGasUintOverflow          = errors.New("gas uint64 overflow")
 	ErrInvalidCode              = errors.New("invalid code: must not begin with 0xef")
 	ErrNonceUintOverflow        = errors.New("nonce uint64 overflow")
+	ErrInvalidOpcode            = errors.New("exection reached INVALID (0xfe) opcode")


Suggested change

ErrInvalidOpcode = errors.New("exection reached INVALID (0xfe) opcode")

ErrInvalidOpcode = errors.New("execution reached INVALID (0xfe) opcode")

Could also just say:

Suggested change

ErrInvalidOpcode = errors.New("exection reached INVALID (0xfe) opcode")

ErrInvalidOpcode = errors.New("aborted with INVALID (0xfe) opcode")

Lastly with EIP-1803 I hope we can rename INVALID to ABORT to make it more clear, but that should be a separate PR I suppose?

holiman

Overall, I'm not quite convinced about the need for doing this. Can you elaborate on that?

holiman · 2021-11-29T20:52:03Z

core/vm/errors.go

@@ -36,6 +36,7 @@ var (
 	ErrGasUintOverflow          = errors.New("gas uint64 overflow")
 	ErrInvalidCode              = errors.New("invalid code: must not begin with 0xef")
 	ErrNonceUintOverflow        = errors.New("nonce uint64 overflow")
+	ErrInvalidOpcode            = errors.New("aborted with INVALID (0xfe) opcode")


We already have an ErrInvalidOpCode, as a type, defined further down in this file. Having a type with the same name as a global var is not a good idea.

IMO let's not define a new specific error for this specific opcode -- the idea with 0xFE is that it will always remain undefined. There are others currently undefined, and IMO this opcode should be treated exactly like those, by the EVM.

So I think we can skip this var here. And the opInvaid could just do

return nil, &ErrInvalidOpCode{opcode: OpCode(0xfe)}

gumb0 · 2021-11-30T10:30:59Z

Overall, I'm not quite convinced about the need for doing this. Can you elaborate on that?

With EOF we propose (in EIP-3670) to forbid deploying undefined opcodes (which will allow introducing new opcodes later without bumping EOF version and also simplifies EVM implementation, where it needs only EOF version (without additional revision) to know if particular byte value is defined)

0xfe should still be allowed to deploy as a guaranteed way to fail execution, so this is where it differs from all other bytes that are not defined.

Also a minor point: having an enum value for INVALID helps to clarify EIP-3540 rules in code, see this discussion #22958 (comment)

gumb0 · 2021-11-30T10:39:19Z

0xfe should still be allowed to deploy as a guaranteed way to fail execution, so this is where it differs from all other bytes that are not defined.

And I have EIP-3670 implementation where I use jumpTable[opcode] == nil to find if the opcode is defined, that's what required this PR.

holiman · 2021-11-30T11:09:06Z

core/vm/jump_table.go

+		INVALID: {
+			execute:     opInvalid,
+			constantGas: 0,
+			minStack:    minStack(0, 0),
+			maxStack:    maxStack(0, 0),
+		},


Though -- in the context of what you're implementing in that other PR... Is it really necessary to define it as an acutal op in the jumptable, as opposed to just define the OpCode but leave it empty in the jumptable?

I think it's better to have it in the jumptable, I see the question of whether 0xfe is a defined opcode as a property of particular fork rules (currently it's defined in all forks, but theoretically 0xfe might mean something else in some other jump table)

Without putting it into jumptable I can only think of having it as a special case in code validation, like jumpTable[opcode] == nil && opcode != 0xfe will mean it's not defined.

axic · 2021-12-09T13:47:30Z

As discussed with @gumb0, opInvalid may be required for the EIP-3670-style code validation, but lets tackle that once @gumb0 opens a draft PR for that. This change (defining the instruction) is useful for logging purposes as well as for consumers of the instruction set for (dis)assembly purposes (such as goevmlab). As a reference, most assemblers (including Solidity) out there support invalid as an opcode.

holiman

LGTM

* core/vm: Define 0xfe opcode as INVALID * core/vm: Remove opInvalid as opUndefined handles it Co-authored-by: Alex Beregszaszi <[email protected]>

core/vm: Make INVALID a defined opcode (ethereum#24017) * core/vm: Define 0xfe opcode as INVALID * core/vm: Remove opInvalid as opUndefined handles it Co-authored-by: Alex Beregszaszi <[email protected]> Co-authored-by: Andrei Maiboroda <[email protected]> Co-authored-by: Alex Beregszaszi <[email protected]>

Cherry-pick ethereum/go-ethereum#24017. * core/vm: Define 0xfe opcode as INVALID * core/vm: Remove opInvalid as opUndefined handles it Co-authored-by: Alex Beregszaszi <[email protected]> Co-authored-by: Andrei Maiboroda <[email protected]> Co-authored-by: Alex Beregszaszi <[email protected]>

* core/vm: Define 0xfe opcode as INVALID * core/vm: Remove opInvalid as opUndefined handles it Co-authored-by: Alex Beregszaszi <[email protected]>

axic reviewed Nov 29, 2021

View reviewed changes

gumb0 force-pushed the invalid-opcode branch 3 times, most recently from 2676eda to e2aefed Compare November 29, 2021 18:42

holiman reviewed Nov 29, 2021

View reviewed changes

gumb0 force-pushed the invalid-opcode branch from e2aefed to 3180e4f Compare November 30, 2021 10:59

holiman reviewed Nov 30, 2021

View reviewed changes

gumb0 force-pushed the invalid-opcode branch from 3180e4f to f8997b5 Compare November 30, 2021 11:15

chfast self-requested a review November 30, 2021 13:12

axic mentioned this pull request Nov 30, 2021

core/vm: implement EIP-3540: EVM Object Format (EOF) v1 #22958

Closed

axic force-pushed the invalid-opcode branch from f8997b5 to 5ae27e9 Compare December 3, 2021 12:49

gumb0 and others added 2 commits December 9, 2021 12:24

core/vm: Define 0xfe opcode as INVALID

fea576c

core/vm: Remove opInvalid as opUndefined handles it

ed76905

axic force-pushed the invalid-opcode branch from 5ae27e9 to ed76905 Compare December 9, 2021 12:42

gumb0 marked this pull request as ready for review December 9, 2021 13:43

gumb0 requested review from gballet, karalabe, MariusVanDerWijden and rjl493456442 as code owners December 9, 2021 13:43

holiman approved these changes Dec 17, 2021

View reviewed changes

holiman added this to the 1.10.14 milestone Dec 17, 2021

holiman merged commit 3e47e38 into ethereum:master Dec 17, 2021

holiman deleted the invalid-opcode branch December 17, 2021 12:44

This was referenced Sep 23, 2022

Metadium to master METADIUM/go-metadium#24

Closed

Metadium to master METADIUM/go-metadium#25

Merged

yperbasis mentioned this pull request Dec 30, 2022

core/vm: Make INVALID a defined opcode erigontech/erigon#6477

Merged

gzliudan mentioned this pull request Feb 27, 2024

preparation for solidity v0.8.23 upgrade XinFinOrg/XDPoSChain#452

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/vm: Make INVALID a defined opcode #24017

core/vm: Make INVALID a defined opcode #24017

gumb0 commented Nov 29, 2021 •

edited

Loading

axic Nov 29, 2021

axic Nov 29, 2021

axic Nov 29, 2021

holiman left a comment

holiman Nov 29, 2021

gumb0 commented Nov 30, 2021

gumb0 commented Nov 30, 2021

holiman Nov 30, 2021

gumb0 Nov 30, 2021

axic commented Dec 9, 2021

holiman left a comment

	ErrInvalidOpcode = errors.New("exection reached INVALID (0xfe) opcode")
	ErrInvalidOpcode = errors.New("execution reached INVALID (0xfe) opcode")

	ErrInvalidOpcode = errors.New("exection reached INVALID (0xfe) opcode")
	ErrInvalidOpcode = errors.New("aborted with INVALID (0xfe) opcode")

core/vm: Make INVALID a defined opcode #24017

core/vm: Make INVALID a defined opcode #24017

Conversation

gumb0 commented Nov 29, 2021 • edited Loading

axic Nov 29, 2021

Choose a reason for hiding this comment

axic Nov 29, 2021

Choose a reason for hiding this comment

axic Nov 29, 2021

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

holiman Nov 29, 2021

Choose a reason for hiding this comment

gumb0 commented Nov 30, 2021

gumb0 commented Nov 30, 2021

holiman Nov 30, 2021

Choose a reason for hiding this comment

gumb0 Nov 30, 2021

Choose a reason for hiding this comment

axic commented Dec 9, 2021

holiman left a comment

Choose a reason for hiding this comment

gumb0 commented Nov 29, 2021 •

edited

Loading