EVM: error handling

[jochem-brouwer]

Part of #3712

Excerpt from evm/errors.ts:

export enum EvmErrorCode {
  UNSUPPORTED_FEATURE = 'EVM_ERROR_UNSUPPORTED_FEATURE',
  RUNTIME_ERROR = 'EVM_ERROR_RUNTIME_ERROR',
}

type EvmRuntimeErrorType = {
  code: EvmErrorCode.RUNTIME_ERROR
  reason: RuntimeErrorMessage | EOFError
} & (
  | { reason: RuntimeErrorMessage.REVERT; revertBytes: Uint8Array }
  | { reason: Exclude<RuntimeErrorMessage, RuntimeErrorMessage.REVERT> | EOFError }
)

export type EvmErrorType = { code: EvmErrorCode.UNSUPPORTED_FEATURE } | EvmRuntimeErrorType

export class EvmError extends EthereumJSError<EvmErrorType> {
  constructor(type: EvmErrorType, message?: string) {
    super(type, message)
  }
}

This is a Proof-of-Concept (I still need to do some cycles to clean this up though) which shows that we now have two EVM error types: a UNSUPPORTED_FEATURE or a RUNTIME_ERROR. In case of a RUNTIME_ERROR, typescript now demands a reason field in the error (this could for instance be "stack overflow", "stack underflow", etc.

What is nice: for the REVERT reason it demands a revertBytes field, which is actually the reverted bytes in case the EVM runs into a revert (from these bytes error messages can be derived, such as solidity revert strings!).

Errors now look like this:

(I had added the error.code field but I think we should keep it at error.type.code).

The error handler (taken / inspired from lodestar: https://github.com/ChainSafe/lodestar/blob/unstable/packages/utils/src/errors.ts) allows to add custom error classes (see EvmError class in this case, can add extra constructor arguments for environment situation), and also has a message argument which allows for custom message strings.

Note: super WIP. I am not completely satisfied yet with the results, because now EVM has a huge code bloat due to instantiating these error messages. I have to find a way to go back to the "old versions" (trap(ERROR.OUT_OF_GAS)) but still remaining type-safety (for the REVERT error string for instance).

Note, tests will likely fail because of the updated error format.

[Maliksb11]

Okay 👍

[gabrocheleau]

Is this scalable? I'm wondering how this would look if we had multiple error codes with corresponding different formats. Right now we're just having one case for "RuntimeErrorMessage.REVERT" and another excluding "RuntimeErrorMessage.REVERT", but I wonder if that might become too burdensome if we need to exclude more

[jochem-brouwer]

This was some experimenting with the typescript typing I did. It ensures that if reason is RuntimeErrorMessage.REVERT it has the mandatory revertBytes. If is NOT a REVERT then the reason could be any of the RuntimeErrorMessage or an EOFError.

Note: EOFError has not been migrated to the EvmError, likely when I do that this typing (the second line) does not have to be there (will check)

[jochem-brouwer]

This typing (I checked) is just to type the evm errors such that the REVERT error has a revertBytes field. It is straightforward to add new error codes or new error reasons, or the inject specific error which adds context (like the mandatory revertBytes field in the REVERT error)

[gabrocheleau]

Side note, but I think we might want to migrate to the EVM (rather than Evm) capitalization as we've done for some others (Json -> JSON, etc.)

[gabrocheleau]

Part of #3712

Excerpt from evm/errors.ts:

export enum EvmErrorCode {
  UNSUPPORTED_FEATURE = 'EVM_ERROR_UNSUPPORTED_FEATURE',
  RUNTIME_ERROR = 'EVM_ERROR_RUNTIME_ERROR',
}

type EvmRuntimeErrorType = {
  code: EvmErrorCode.RUNTIME_ERROR
  reason: RuntimeErrorMessage | EOFError
} & (
  | { reason: RuntimeErrorMessage.REVERT; revertBytes: Uint8Array }
  | { reason: Exclude<RuntimeErrorMessage, RuntimeErrorMessage.REVERT> | EOFError }
)

export type EvmErrorType = { code: EvmErrorCode.UNSUPPORTED_FEATURE } | EvmRuntimeErrorType

export class EvmError extends EthereumJSError<EvmErrorType> {
  constructor(type: EvmErrorType, message?: string) {
    super(type, message)
  }
}

This is a Proof-of-Concept (I still need to do some cycles to clean this up though) which shows that we now have two EVM error types: a UNSUPPORTED_FEATURE or a RUNTIME_ERROR. In case of a RUNTIME_ERROR, typescript now demands a reason field in the error (this could for instance be "stack overflow", "stack underflow", etc.

What is nice: for the REVERT reason it demands a revertBytes field, which is actually the reverted bytes in case the EVM runs into a revert (from these bytes error messages can be derived, such as solidity revert strings!).

Errors now look like this:

(I had added the error.code field but I think we should keep it at error.type.code).

The error handler (taken / inspired from lodestar: ChainSafe/lodestar@unstable/packages/utils/src/errors.ts) allows to add custom error classes (see EvmError class in this case, can add extra constructor arguments for environment situation), and also has a message argument which allows for custom message strings.

Note: super WIP. I am not completely satisfied yet with the results, because now EVM has a huge code bloat due to instantiating these error messages. I have to find a way to go back to the "old versions" (trap(ERROR.OUT_OF_GAS)) but still remaining type-safety (for the REVERT error string for instance).

Note, tests will likely fail because of the updated error format.

I like the approach, as it has the obvious benefits of:

• Being able to match errors with reliables error codes
• Custom error types with additional info in a consistant format. Useful for both writing errors and consuming them

Some of the things that I'm questioning however:

• Mentioned that in a comment, but adding new error types should be straightforward, and with the current typings I'm wondering if that might become hard to scale.
• You have mentioned that in your initial post, but can we evaluate the additional bloat caused by these? We have been moving away from a lot of the object-oriented things recently (e.g. with tx capabilities), using granular helpers rather than bloated classes, and I wonder if this might be something that increases bundle size, affects performance, etc. EVM is really something we should not compromise too much performance on imo.

Do you have some examples on how Lodestar handles custom error codes in similar contexts (multiple error codes within a single package, with different types for each of them?). I'Ve looked but can only find examples like this: https://github.com/ChainSafe/lodestar/blob/dad9037e7739d5bcbccfe627e715ef40e9ba935b/packages/api/src/utils/server/error.ts which are simpler than what we do.

Nice work!

[jochem-brouwer]

For Lodestar error handling see:

https://github.com/ChainSafe/lodestar/blob/unstable/packages/utils/src/errors.ts

https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/errors/blockError.ts

[jochem-brouwer]

For performance, this should not have any noticeable impact, the errors are just "nicer" now since it gives error codes and error context (as example here in the REVERT error).

I will clean up this PR to reflect how this would work, and if all is fine we can roll this all repo-wide.

[gabrocheleau]

For performance, this should not have any noticeable impact, the errors are just "nicer" now since it gives error codes and error context (as example here in the REVERT error).

I will clean up this PR to reflect how this would work, and if all is fine we can roll this all repo-wide.

Great! Looking at Lodestar's handling I think it looks super clean, for e.g. this type is really readable and it would be easy to add new error types etc. : https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/errors/blockError.ts#L74

Let me know once this is ready for an actual review, excited about rolling this out progressively throughout the monorepo!

[codecov]

Codecov Report

Attention: Patch coverage is 44.44444% with 15 lines in your changes missing coverage. Please review.

Project coverage is 77.91%. Comparing base (bd77a74) to head (85aa5fa).

Additional details and impacted files

Flag	Coverage Δ
block	76.87% <ø> (ø)
blockchain	85.69% <ø> (ø)
client	66.27% <100.00%> (ø)
common	90.72% <ø> (ø)
devp2p	76.21% <ø> (-0.07%)	⬇️
ethash	81.04% <ø> (ø)
evm	?
genesis	99.84% <ø> (ø)
mpt	59.72% <ø> (+0.20%)	⬆️
rlp	69.70% <ø> (ø)
statemanager	70.47% <ø> (ø)
tx	80.96% <ø> (ø)
util	84.92% <42.30%> (-0.63%)	⬇️
vm	?
wallet	83.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.