Change LLL opcode generated by "panic" to INVALID

[benjaminion]

EIP-141 ethereum/EIPs#141 has preserved 0xfe as an invalid opcode for aborting EVM execution. The EVM assembler supports this via the INVALID opcode.

The LLL "panic" expression used to generate a jump to an invalid location in order to abort EVM execution. This change brings "panic" into line with EIP-141 by generating the INVALID opcode instead.

[pirapira]

I think this is fine. For users' faults LLL programmers can use REVERT, not PANIC. @axic what do you think?

[benjaminion]

After reviewing #1589, I'll just note that In LLL there isn't really a meaningful distinction between implicit and explicit throws, as the compiler doesn't autogenerate very much.

If the programmer wishes to distinguish between failure modes, this is what I suggest,

1. Change "panic" expression to generate INVALID (0xfe) as per this PR. Inasmuch as we need a "panic" expression at all, it seems best matched with INVALID.

2. The "revert" expression will generate the new REVERT (0xfd) opcode. This should be used for input validation (e.g. sending value to non-payable contracts) as per Change invalid opcode to revert for input validation. #2298. No code change should be required for this as LLL automatically handles existing opcodes. However, it may be useful to introduce a compiler macro analogous to the existing "return" macro for convenience.

3. The "invalid" expression will generate the INVALID (0xfe) opcode explicitly if desired, so "panic" is not really required any more, but may be more meaningful in a code context.

[axic]

I had a similar change in April, but realised it should be just moved to the standard macros section as it is possible to issue the "invalid opcode". This was not the case when (panic) was added (and hence the reason it exists).

(def 'panic (asm INVALID)) should go into CompilerState.cpp if we decide to change panic.

[benjaminion]

(def 'panic (asm INVALID)) should go into CompilerState.cpp if we decide to change panic.

Sure, that's equivalent from the programmer's point of view, and is in line with your goal as stated in #1230:

Edit: it turns out it's not quite the same for the programmer:

• With the current and my first approach code for this looks like: (panic) to generate 0xfe
• With the macro approach it looks like: panic which is potentially breaking and would fail the existing tests for panic.

Move as much as possible from the parser to macros.

No problem with this approach in principle.

[pirapira]

@axic shall we go for a macro even if that would be a breaking change (from (panic) to panic as written above)?

[axic]

I do not fully understand the above. Why is it different? It shouldn't be different.

[benjaminion]

Why is it different? It shouldn't be different.

Consider compiling the following.

(seq (panic))

1. Current implementation gives 600056.
2. With my patch, gives fe.
3. With your macro in CompilerState.cpp and removing PANIC from CodeFragment.cpp, gives Parse error... Symbol not found: panic

To compile using the macro version (3.) the source needs to be expressed differently, as follows.

(seq panic)

This doesn't compile with the current version (1.). Hence "breaking".

I don't know if there is something funky that can be done with the macro to make the syntax consistent with the previous implementation (1.).

In any case, maybe it's time to drop the panic expression altogether. I doubt there's much code out there that uses it.

[pirapira]

@benjaminion if you have the macro implementation already, can you share this? I want to try.

[axic]

The code actually should be (def 'panic () (asm INVALID)) to make it a call not a literal. However the parser fails on that as it cannot handle empty parameter lists.

However, I think it should work without parameter list too in an ideal case.

[axic]

It turns out to be a parser error:

• 2 parameters are considered "definitions" (aka literals)
• 3 parameters are considered as actual macros

[benjaminion]

@pirapira

I've added a second commit that does that implements the macro version instead. (Not sure what the protocol is for doing this... hope it's OK.)

@axic

It turns out to be a parser error:

Ah, OK - I've wondered about that previously but just put it down to "the way things are".

[pirapira]

@axic if you haven't, I start adding support for macros with zero arguments.

[pirapira]

On top of #2375, (panic) should be definable as a macro.

[axic]

Can you please rebase (now that #2375 is merged)?

[benjaminion]

Have updated the macro and rebased.

In summary,

1. the "panic" expression has been moved from the parser to compiler macros. It is defined as an expression with no arguments, so that (panic) is valid syntax. This depends on LLL: macro with zero args #2375.

2. "panic" now generates 0xfe (INVALID) rather than jumping to an undefined location.

[pirapira]

Looks good to me.