Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error Management Improvements / Add error codes #487

Closed
rumkin opened this issue Apr 5, 2019 · 13 comments
Closed

Error Management Improvements / Add error codes #487

rumkin opened this issue Apr 5, 2019 · 13 comments

Comments

@rumkin
Copy link
Contributor

rumkin commented Apr 5, 2019

Error management in long term couldn't rely on stringified error messages. For example Node.js decided to add error codes while ago because they met the issues with this. You can look at lib/internal/error.js to see how they have solved the problem.

In a few words node.js is using an error factory which receives error code, message formatter and base error class. And then use the factory to define list of errors.

I think they overengineered it: they simplify error creation but factory class creation make stack trace less informative. And this could be done using ES6 class inheritance mechanism.

I have some experience with such error handling. And can help to implement it in VM. Just decided to create an issue before making a PR.

Proposal

  1. Add code property to identify error.
  2. Add details property to store error details as dictionary of primitives.
  3. Add format method to produce error message from details object.

Motivation

  1. It's easy to compare error codes using strict equality operator. What makes error handling robust.
  2. Codes could be used as part of URL. In documentation for example.
  3. Details are stored as primitives though they could be used to produce custom output, e.g. highlighting.
  4. Error message could use Intl to produce localized message text.
  5. Error message could be modified for environment purposes.
  6. Shorten error invocation code.
  7. It's network friendly. Error could be easy restored and represented on the client side using from code and details.

Example

Let's take upfront cost error in runTx method:

message = "sender doesn't have enough funds to send tx. The upfront cost is: " + tx.getUpfrontCost().toString() + ' and the sender\'s account only has: ' + new BN(fromAccount.balance).
cb(new Error(message))
return

How it could be modified.

  1. Create lib/errors.js to hold EVM error constructors:
  2. We create custom Error with code for example tx_upfront_cost:
const formatters = {
  tx_upfront_cost({cost, balance}) {
      return `sender doesn't have enough funds to send tx. The upfront cost is: ${cost} and the sender\'s account only has: ${balance}`
  }
}

class EvmError extends Error () {
  constructor(details) {
    super();
    this.message = this.format(details)
    this.details = details
  }

  format(details) {
     return formatters[this.code](details) 
  }
}

class TxUpfrontCost extends EvmError {
  get code() {
     return 'tx_upfront_cost'
  }
}

exports.TxUpfrontCost = TxUpfrontCost

Use it in the runTx code:

const Err = require('./errors')

// .. below in the code

cb(new Err.TxUpfrontCost({
  cost: tx.getUpfrontCost().toString(),
  balance: BN(fromAccount.balance),
})
@holgerd77
Copy link
Member

Hi @rumkin, thanks for this extensive write-up! Error handling can definitely improved within the VM, and your motivational points you outlined are all pretty valid from my perspective. So if you want to do some work here that would be highly appreciated! 😄

@s1na Any additional thoughts or comments?

One thing that came to my mind: I think we should also preserve broader error categorization like STACK_OVERFLOW (or respectively: do some better categorization, but at least provide categories), so that people can handle broader categories. Is this also implied in your concept?

One way here would probably to (also/alternatively) do numerical error codes like (made up the numbers) 301, 302,..., one could also think of adding an extra middle class layer on the inheritance hierarchy with e.g. a StackOverflowError or so class and some getter method which inherits down and can be used to retrieve the respective category. Does this make sense? Thoughts?

Is there actually some kind of standard - e.g. for error codes - in the (some of the) other clients?

@s1na
Copy link
Contributor

s1na commented Apr 5, 2019

Thanks for the proposal, I'm already on-board :)

I'd add that in addition to error classification, we could also improve error handling, in the sense that which errors are expected (e.g. OOG within the context of evm), and which ones are unexpected and should be propagated and possibly cause a crash.

@holgerd77
Copy link
Member

@rumkin additional note: we are currently in the process of a larger refactor - see v4 PR #479 branch - eventually it makes sense to do this work directly towards this - not sure how deep you want to dive in.

@rumkin
Copy link
Contributor Author

rumkin commented Apr 5, 2019

@holgerd77

So if you want to do some work here that would be highly appreciated!

Great! I'm in.

  1. Categories are possible. And intermediate classes are good for this. For example for web services I usually implement 3 categories: Access, BadInput and Runtime. This is enough to cover cases of permissions, input validation and any unexpected behavior. Of cause exact categories will different for EVM, but this is a proven solution. And this allows to receive full advantages from inheritance.

Errors could be event nested like so Err.State.InvalidTrie.

... do numerical error codes ...

  1. Numeric error codes are ok in case of Ethereum. But I'm not a big fan of them. Because you always need to store a mapping table somewhere on a disk or in a head. And text codes are better to remember.

Is there actually some kind of standard - e.g. for error codes

  1. There is no standard. Only common techniques which I guess you already known.
    For numeric there are:
  1. Grouping using some step (like HTTP do using 100 as a step).
  2. Custom offsets.
  3. Incremental counter.

For text codes there are prefixes. Prefix could has some additional separation (fs:some_error) or just be a part of code (fs_some_error).

The main thing is to make it easy to restore error on the other side of network connection. And if there is a numeric codes in use, then there should be some package with translator. Because developers are prone to handle errors in-place and if it's not possible they will not manage it at all.

it makes sense to do this work directly towards this - not sure how deep you want to dive in

  1. Do you have timeframe to estimate if I could join?

@sina

which errors are expected (e.g. OOG within the context of evm), and which ones are unexpected and should be propagated and possibly cause a crash.

I need to get deeper in EVM to be useful in such cases. But I've got some case with expected and unexpected errors in deeply nested tree in test running tool. And solution for me become a wrapper for unexpected errors which is using like a flag that error is already handled.

catch (err) {
 if (err instanceof UnitError || err instanceof RuntimeError) {
   throw err;
 }
 else {
   throw new RuntimeError(err);
 }
}

RuntimeError could be a regular object. To avoid duplicated stack trace capturing cause it's pretty expensive. Maybe it could be useful somehow.

@holgerd77
Copy link
Member

  1. Yes, I think three steps is very much fine and should cover enough cases respectively give fine-grained enough control.

  2. Yes, you are very much right, this extra (mental/technical) mapping for numerical codes is annoying, furthermore if we get all the properties like category based handling with a more readable approach.

  3. With standard I meant: what types of errors are the other client (VM) implementations using in the EVM context (rather than some generic: how to do error handling standards).

Had a short look, some inspirations from other clients (feel free to directly add to this list in this post by editing if permissions available):

Do you think you have still some capacity to work on this or did something change in the meantime (feel free to openly let us know, this write-up is already pretty helpful)? And if so, would you want to do a small getting-started PR, maybe with the basic new structure handling one specific error case where classification is simple?

@NikZak
Copy link

NikZak commented Sep 30, 2024

I have problems with error management with the VM. When I get EvmError I can not debug where it is coming from. Any help here would be valuable. Is there a way to throw console messages? Or is there a way to have at least some debug message where the error stems from (maybe some require failed in solidity code - but where is the message from that require?)

@jochem-brouwer
Copy link
Member

Hi @NikZak, could you post the EvmError you are getting here? You should get a stack trace. Note, that if this error gets thrown such that your script actually runs into this error, this means that internally we have thrown this and this would signal a but within the EVM.

However, I think you are looking into the "solidity error" which is being thrown. We do not throw this by default, and this is also how the EVM works. The EVM will run "calls", and these calls themselves might revert (error). To lookup these errors, you have to hook into the events of EVM:

evm.events.on('afterMessage', (data) => { /** your logic here *// }

The data is of type EVMResult. You can lookup data.execResult.exceptionError (if no error, this is undefined). This will give you an error string. Let me know if you want to pinpoint down the exact error which solidity might have thrown (the "revert reason"), you will need additional steps here to extract it. The exceptionError will only give you the "reason" for an error, for instance revert, but it could also be out of gas, you likely need more environment data.

@NikZak
Copy link

NikZak commented Sep 30, 2024

Thank you very much @jochem-brouwer! But I really need to know the exact error which solidity might have thrown (the "revert reason"). Would be amazing if you could point to steps that help get that

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Sep 30, 2024

Hi @NikZak, what you need to extract the reason here is to hook into the afterMessage and then extract the revert reason in case a message frame ends with a revert reason. I created and tested this for you, this should do the job:

import { equalsBytes, bytesToBigInt, utf8ToBytes, bytesToUtf8, bytesToHex } from '@ethereumjs/util'
import { keccak256 } from 'ethereum-cryptography/keccak'
    
evm.events.on('afterMessage', (e) => {
    if (e.execResult.exceptionError?.error === 'revert') {
        const revertBytes = e.execResult.returnValue
        // The revertBytes are the "raw bytes" which are thrown within the EVM
        // If the contract is compiled by solidity, this is an ABI-encoded string
        // See: https://gist.github.com/pcaversaccio/e1aebf9222424e9c1fbc82b8bf5fea8b
        // To extract the reason:
        // The first 4-bytes are the function selector of Error(string)
        // The keccak256 hash of "Error(string)" is 08c379a0afcc32b1a39302f7cb8073359698411ab5fd6e3edb2c02c0b5fba8aa
        // Take the first 4-bytes to get the function selector: 08c379a0
        // Then, the next 32 bytes is the offset of the string in this memory, which points to the start of the revert string
        // When reading this offset from memory, the first 32 bytes are interpreted as the string length
        // The actual string follows after this
        const fnSelector = keccak256(utf8ToBytes("Error(string)")).slice(0,4)
        if (equalsBytes(fnSelector, revertBytes.slice(0, 4))) {
            const relevantMemory = revertBytes.slice(4)
            const offset = Number(bytesToBigInt(relevantMemory.slice(0, 32)))
            const length = Number(bytesToBigInt(relevantMemory.slice(offset, offset+ 32)))
            const stringStart = offset + 32
            const stringBytes = relevantMemory.slice(stringStart, stringStart + length)
            const revertString = bytesToUtf8(stringBytes)
            console.log(`EVM Revert, reason: ${revertString} (length: ${length}))`)
        } else {
            console.log(`Non-solidity revert, raw revert bytes: ${bytesToHex(revertBytes)}`)
        }
    }
})

@NikZak
Copy link

NikZak commented Sep 30, 2024

Oh, this is perfect! Helps a lot! Indeed informative messages now! Thanks a lot!

@gabrocheleau gabrocheleau removed their assignment Oct 2, 2024
@NikZak
Copy link

NikZak commented Oct 4, 2024

Just for completeness - added panic messages too.

const solidityPanicErrorCodes = {
  0x00: "Generic compiler inserted panics",
  0x01: "Assert with an argument that evaluates to false",
  0x11: "Arithmetic operation results in underflow or overflow",
  0x12: "Divide or modulo by zero",
  0x21: "Convert a value that is too big or negative into an enum type",
  0x22: "Access a storage byte array that is incorrectly encoded",
  0x31: "Call .pop() on an empty array",
  0x32: "Access an array, bytesN or an array slice at an out-of-bounds or negative index",
  0x41: "Allocate too much memory or create an array that is too large",
  0x51: "Call a zero-initialized variable of internal function type",
};

/// The revertBytes are the "raw bytes" which are thrown within the EVM
/// If the contract is compiled by solidity, this is an ABI-encoded string
/// See: https://gist.github.com/pcaversaccio/e1aebf9222424e9c1fbc82b8bf5fea8b
/// To extract the reason:
/// The first 4-bytes are the function selector
/// E.g: the keccak256 hash of "Error(string)" is 08c379a0afcc32b1a39302f7cb8073359698411ab5fd6e3edb2c02c0b5fba8aa
/// Take the first 4-bytes to get the function selector: 08c379a0
/// Then, the next 32 bytes is the offset of the string in this memory, which points to the start of the revert string
/// When reading this offset from memory, the first 32 bytes are interpreted as the string length
/// The actual string follows after this
export function handleErrorMessages(vm: VM) {
  vm.evm.events!.on("afterMessage", (e) => {
    if (e.execResult.exceptionError?.error === "revert") {
      const revertBytes = e.execResult.returnValue;
      const errorFnSelector = keccak256(utf8ToBytes("Error(string)")).slice(
        0,
        4
      );
      const panicFnSelector = keccak256(utf8ToBytes("Panic(uint256)")).slice(
        0,
        4
      );
      if (equalsBytes(errorFnSelector, revertBytes.slice(0, 4))) {
        const relevantMemory = revertBytes.slice(4);
        const offset = Number(bytesToBigInt(relevantMemory.slice(0, 32)));
        const length = Number(
          bytesToBigInt(relevantMemory.slice(offset, offset + 32))
        );
        const stringStart = offset + 32;
        const stringBytes = relevantMemory.slice(
          stringStart,
          stringStart + length
        );
        const revertString = bytesToUtf8(stringBytes);
        console.error(
          `EVM Revert, reason: ${revertString} (length: ${length}))`
        );
      } else if (equalsBytes(panicFnSelector, revertBytes.slice(0, 4))) {
        const errorCode = Number(bytesToBigInt(revertBytes.slice(4)));
        const errorReason =
          solidityPanicErrorCodes[
            errorCode as keyof typeof solidityPanicErrorCodes
          ];
        console.error(
          `Solidity panic, reason: ${errorReason} (code: ${errorCode})`
        );
      } else {
        console.error(
          `Non-solidity revert, raw revert bytes: ${bytesToHex(revertBytes)}`
        );
      }
    }
  });
}

@jochem-brouwer
Copy link
Member

Ah, interesting, I was not aware of Panic(string). Great addition, thanks! I also found this solidity ref: https://docs.soliditylang.org/en/latest/control-structures.html#error-handling-assert-require-revert-and-exceptions

@jochem-brouwer
Copy link
Member

I will close this issue, the central error handling issue is now here: #3712

@NikZak if you need any further assistance feel free to open a new issue/discussion or ask on our discord: https://discord.gg/TNwARpR 😄 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants