Reorder status codes by chfast · Pull Request #23 · ipsilon/evmc

chfast · 2018-04-18T14:08:55Z

Fixes #21.

ehildenb

Looks good, I think the new order makes more sense.

axic · 2018-04-18T17:34:59Z

include/evmc.h

+     *
+     * An example is RETURNDATACOPY reading past the available buffer.
+     */
    EVMC_INVALID_MEMORY_ACCESS = 10,


I think this should come before static mode.

I think a logical order would be:

invalid/unknown instructions

invalid number of arguments for instructions

invalid arguments (bad jump, invalid memory access)

invalid state (static mode violation)

No comment on this nice ordered list? 😉

I did it this way...

I don't think so :)

EVMC_INVALID_MEMORY_ACCESS = 9, EVMC_CALL_DEPTH_EXCEDED = 10, EVMC_STATIC_MODE_VIOLATION = 11,

What am I missing?

This part:

invalid/unknown instructions

invalid number of arguments for instructions

invalid arguments (bad jump)

The order now is argument count, bad jump, invalid/unknown, etc.

Reordered again then.

axic · 2018-04-18T17:36:10Z

include/evmc.h

+ * Successful execution is represented by ::EVMC_SUCCESS having value 0.
+ * Positive values represent failures defined by VM specifications with generic
+ * ::EVMC_FAILURE code of value 1.
+ * Negative values represent VM internal errors and might be not recoverable.


Should mention the special REJECTED here I think.

axic · 2018-04-18T17:36:25Z

include/evmc.h

+     * For example, the Client tries running a code in the EVM 1.5. If the
+     * code is not supported there, the execution falls back to the EVM 1.0.
+     */
+    EVMC_REJECTED = -2,


What is the reason for swapping these two?

The generic first, the specific after.

I a bit torn on this, since REJECTED is a special one and I am not sure it is an internal error. It is certainly recoverable.

It's internal in sense it should not be passed back to the caller.
Yes, it's recoverable if the client supports it and has a backup VM. I will extend the description of evmc_status_code to mention this as suggested.

axic · 2018-04-19T09:39:57Z

include/evmc.h

+ * (see e.g. ::EVMC_REJECTED), otherwise internal errors are not recoverable.
+ * The generic representant of errors is ::EVMC_INTERNAL_ERROR but
+ * an EVM implementation MAY return negative status codes that are not defined
+ * in the EVMC documentation.


Should probably leave a comment here along the lines of "should you need more error codes, please create an issue on the evmc repo to discuss" to facilitate discussion.

ehildenb · 2018-04-19T15:41:51Z

I might be good to start a convention along the lines of:

0*: normal statuses (EVMC_SUCCESS, EVMC_REVERT, etc ...)
1*: exceptional statuses (oog, oom, etc...)
2*: validity statuses (bad jump structure, etc ...)
3*: error modes (internal error, etc ...)

this way we don't have to re-organize the entire error-code space every time we add codes. Perhaps we make them hexadecimal numbers to allow for at least 16 codes in each category.

Notice that the KEVM formalization of this: https://github.com/kframework/evm-semantics/blob/typed-exceptions/network.md
does not include numbers, it simply bins them by sort. So I have a sort ExceptionalStatusCode for execeptions, and EndStatusCode for ones that end execution, then just "the rest". Also note that I have EVMC_BALANCE_UNDERFLOW and EVMC_ACCOUNT_ALREADY_EXISTS as status codes which are not present here (and are used in KEVM).

more discussion

chfast · 2018-04-19T20:12:57Z

I don't mind categories, but I'm capable of doing good clusterization myself. If that will follow external reference I'm ok if having this here.

Personally, I believe it will just look nice here, with not benefits for implementations.

I'd like to keep internal errors with negative values. This way you can specify that status_code >= 0 must be passed to the caller and status_code < 0 must not be passed to the caller.

I would also avoid hex, because the number would like different in this header (e.g. 0x11) and different in logs (e.g. Failure code 17) so you cannot quickly look them up here.
If we need more space than 10, use 100.

What do you think @axic?

axic · 2018-04-20T21:33:54Z

I would also avoid hex, because the number would like different in this header (e.g. 0x11) and different in logs (e.g. Failure code 17) so you cannot quickly look them up here.

This seems to be a reasonable idea.

I wouldn't bother too much about this, I think 0.2.0 will come soon after all these different implementers experience the first problems and we can fix it there.

chfast · 2018-04-20T22:01:09Z

I wouldn't bother too much about this, I think 0.2.0 will come soon after all these different implementers experience the first problems and we can fix it there.

I'm a bit inclined not to overdesign it now.

axic · 2018-04-20T22:30:49Z

include/evmc/evmc.h

+    EVMC_INVALID_MEMORY_ACCESS = 9,
+
+    /** Call depth has exceeded the limit (if any) */
+    EVMC_CALL_DEPTH_EXCEDED = 10,


This still has the typo (exceded vs. exceeded).

Oops, I only fixed the comment.

timxor · 2018-04-22T19:29:06Z

include/evmc/evmc.h

+
+    /**
+     * The execution has attempted to put more items on the EVM stack
+     * than the specified limit.


Was the EVM stack limit 1024? If it is not going to change in the future, then it may be of use to include that number here (in the comments) 😁 @chfast

Both limits for stack and call depth are 1024 and have never changed.

It may make sense including in the description but not in the error name. We can consider it when a hardfork is introduced changing those :)

I will leave it for another time...

timxor · 2018-04-22T19:37:24Z

include/evmc/evmc.h

+     */
+    EVMC_INVALID_MEMORY_ACCESS = 9,
+
+    /** Call depth has exceeded the limit (if any) */


Was the call depth limit, also 1024?

timxor

English looks good to me!

timxor · 2018-04-22T19:45:47Z

include/evmc/evmc.h

+     * For example, the Client tries running a code in the EVM 1.5. If the
+     * code is not supported there, the execution falls back to the EVM 1.0.
+     */
+    EVMC_REJECTED = -2,


In my opinion (for what it is worth) a more descriptive name would be EVMC_INCOMPATIBILE_VERSION or EVMC_INVALID_VERSION

The provided example is not the only case we are using it even now. You can configure EVMJIT to jit only popular codes (measured by hit count). It will return EVMC_REJECTED if the hit count for requested code is below the threshold.

chfast requested review from axic and ehildenb and removed request for axic April 18, 2018 14:08

ehildenb previously approved these changes Apr 18, 2018

View reviewed changes

axic reviewed Apr 18, 2018

View reviewed changes

ehildenb mentioned this pull request Apr 18, 2018

include/evmc: add status code EVMC_CALL_DEPTH_EXCEDED #24

Merged

chfast force-pushed the status-codes-reorder branch from f9ecca2 to 3817ac8 Compare April 19, 2018 07:37

axic reviewed Apr 19, 2018

View reviewed changes

chfast force-pushed the status-codes-reorder branch 3 times, most recently from 750b35b to 69ba561 Compare April 19, 2018 13:16

chfast mentioned this pull request Apr 19, 2018

evmc.h: spelling error exceded => exceeded #27

Closed

axic reviewed Apr 20, 2018

View reviewed changes

chfast force-pushed the status-codes-reorder branch from 69ba561 to a66a89e Compare April 20, 2018 22:38

Reorder status codes

9938b3b

chfast force-pushed the status-codes-reorder branch from a66a89e to 9938b3b Compare April 21, 2018 22:14

timxor reviewed Apr 22, 2018

View reviewed changes

timxor approved these changes Apr 22, 2018

View reviewed changes

axic approved these changes Apr 23, 2018

View reviewed changes

chfast merged commit 766d4ab into master Apr 23, 2018

chfast deleted the status-codes-reorder branch April 23, 2018 12:09

Conversation

chfast commented Apr 18, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehildenb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehildenb commented Apr 19, 2018

Uh oh!

chfast commented Apr 19, 2018

Uh oh!

axic commented Apr 20, 2018

Uh oh!

chfast commented Apr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timxor Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timxor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chfast commented Apr 18, 2018 •

edited by axic

Loading

timxor Apr 22, 2018 •

edited

Loading