Return more sane value that replaces Return

[rakita]

Return is a enum and it is internal structure, exposing it outside of revm creates not that great interface.

There are four states that can be returns:

• Success: Transaction executed and it was success
• Failure: Transaction executed but it failed: Check this: https://eips.ethereum.org/EIPS/eip-658
• Error: Transaction not executed. Not enough balance in account to cover max gas consumes.
• ExternalError: Transaction not executed, external database error.

[rakita]

ExternalError should return type that is specified in Database::Error

[Wodann]

I can have a look at this, if you'd like.

Is your idea to add a generic to Return?

If so, would that generic need to implement serde::Serialize and serde::Deserialize for the with-serde feature flag?

[rakita]

That would be great!

I am okay with Return being internal one byte control enum, but it is not great as an external type. New enum needs to be made and maybe even replace the ExecutionResult type or make it better

[Wodann]

Gotcha. I'll create a proof of concept and then we can continue discussing from there 🙂

[Wodann]

I started working on it when I realised that this will be a breaking change, as we'll be replacing the ExecutionResult which is exposed in the ABI and as return value of a lot of APIs. Is that an issue for you?

[rakita]

It should be a breaking change, plan is to publish v3.0.0 version as there are a lot of breaking changes on main

[Wodann]

I'll prioritise this then!

[rakita]

You dont need to rush it, I am taking it slowly, the timeline is around the middle to end of January.

[Wodann]

Before delving too deep, I wanted to discuss the design, as it seems like the changes might be quite invasive to allow errors to bubble up all the way from the EVM implementation.

I was thinking of using the following error types, inspired by the Python reference implementation of the execution spec [1, 2]:

/// Indicates that the EVM has experienced an exceptional halt. This causes execution to
/// immediately end with all gas being consumed.
#[derive(Debug, thiserror::Error)]
pub enum ExceptionalHalt {
    #[error("The new contract code starts with 0xEF")]
    InvalidContractPrefix,
    /// Occurs when the destination of a jump operation doesn't meet any of the following criteria:
    ///
    /// - The jump destination is less than the length of the code.
    /// - The jump destination should have the `JUMPDEST` opcode (0x5B).
    /// - The jump destination shouldn't be part of the data corresponding to `PUSH-N` opcodes.
    #[error("Invalid jump destination encountered")]
    InvalidJumpDest,
    #[error("Invalid opcode is encountered")]
    InvalidOpcode,
    #[error("Invalid parameters are passed")]
    InvalidParameter,
    #[error("Reading data beyond the boundaries of the buffer")]
    OutOfBoundsRead,
    #[error("The operation costs more than the amount of gas left in the frame")]
    OutOfGas,
    #[error("The message depth is greater than `1024`")]
    StackDepthLimit,
    #[error("A push is executed on a stack at max capacity")]
    StackOverflow,
    #[error("A pop is executed on an empty stack")]
    StackUnderflow,
    #[error("Modifying the state while operating inside of a STATICCALL context")]
    WriteInStaticContext,
}

/// All Ethereum errors that might occur by the specification during normal operation.
#[derive(Debug, thiserror::Error)]
pub enum EthereumError {
    #[error(transparent)]
    ExceptionalHalt(#[from] ExceptionalHalt),
    #[error("The block being processed is found to be invalid")]
    InvalidBlock,
    #[error("RLP decoding failed")]
    InvalidRlpDecoding,
    #[error("RLP encoding failed")]
    InvalidRlpEncoding,
    /// Raised by the `REVERT` opcode.
    ///
    /// Unlike other EVM exceptions this does not result in the consumption of all gas.
    #[error("Raised by the `REVERT` opcode")]
    Revert,
}

#[derive(Debug, thiserror::Error)]
pub enum EvmError<DE: Debug> {
    #[error(transparent)]
    Database(#[from] DatabaseError<DE>),
    #[error(transparent)]
    EthereumError(#[from] EthereumError),
}

#[derive(Debug, thiserror::Error)]
pub enum DatabaseError<E: Debug> {
    #[error("The database returned an error")]
    Failure(E),
    #[error("The database is missing code for a contract")]
    MissingCode(B160),
}

Things to note:

• The DatabaseError will need to be propagated.
• I'm using thiserror to create idiomatic library-style Rust errors. This is the de facto standard in the ecosystem.

Open question(s):

1. Do the error types look good?
2. How do we bubble up error types? I'm inclined to say we should start using the Result<(), EvmError<DE>> type everywhere and handle the errors at the right location. That is:
   a. an ExceptionalHalt should consume all gas and then exit;
   b. a DatabaseError should not consume gas and be returned as an Err for the developer using revm to be notified of a fault in their implementation
   c. I'm wondering whether we can catch the InvalidBlock (and invalid transaction for that matter) errors separately from the execution errors, when the user of revm sets the env.block and env.tx using member functions?
   d. Revert should just return normally.
   e. What happens for InvalidRlpDecoding and InvalidRlpEncoding?

[rakita]

I like ExceptionalHalt but those are not a Error per say as even if they are triggered transaction is still considered valid but not successful, it is more like a status of execution. Those are Failure statuses.

EthereumError does not make sense to be put inside evm as every client does rlp in a different place.

DatabaseError: i like that MissingCode

From the first post Success and Failure are statuses of execution, both of them mean that transaction is valid, it is good for debugging and distinction is needed for Receipt status field.
While Error and ExternalError represent invalid transaction or some critical error from the database (These are PrevrandaoNotSet, GasPriceLessThenBasefee, LackOfFundForGasLimit etc.) those means that transaction is not executed. And if that transactions is inside the block, block will be considered invalid while with status, that would not be a case.

To be honest I am not sure what is the best way to pack all of that. Crazy idea, but wdyt about double Result as in Result<Result<_,ExceptionHalt>,DatabaseAndCriticalError>>?

btw I am doing some internal refactoring of revm, so I would like this change to be front facing, as in mostly in evm_impl: https://github.com/bluealloy/revm/blob/main/crates/revm/src/evm_impl.rs

[fvictorio]

Crazy idea, but wdyt about double Result as in Result<Result<_,ExceptionHalt>,DatabaseAndCriticalError>>?

My two cents: I can't give an informed opinion on the Rust side of this (ergonomics, etc.), but conceptually this makes a lot of sense to me.

[Wodann]

I like ExceptionalHalt but those are not a Error per say as even if they are triggered transaction is still considered valid but not successful, it is more like a status of execution. Those are Failure statuses.

They are an error at a lower-level. The way that the EVM handles it is by catching that error and consuming all gas. After that it's no longer an error. The Result type is supposed to be used that way. Return an Err; and now the caller is supposed to do one of two things:

1. Opt-out early (e.g. with ?) and pass the error along to the caller
2. Handle the error appropriately, with no need to pass it on to the caller

After some more implementation work, I landed on this design:

pub enum Eval {
    Continue = 0x00,
    Stop = 0x01,
    Return = 0x02,
    SelfDestruct = 0x03,
    /// Raised by the `REVERT` opcode.
    ///
    /// Unlike other EVM exceptions this does not result in the consumption of all gas.
    Revert = 0x20,
}

pub enum EthereumError {
    #[error(transparent)]
    ExceptionalHalt(#[from] ExceptionalHalt),
    #[error("The block being processed is found to be invalid")]
    InvalidBlock,
    #[error("RLP decoding failed")]
    InvalidRlpDecoding,
    #[error("RLP encoding failed")]
    InvalidRlpEncoding,
}

pub enum Reason {
    Success(Eval),
    Failure(EthereumError),
}

The Reason would replace the Return, signalling success or failure.

btw I am doing some internal refactoring of revm, so I would like this change to be front facing, as in mostly in evm_impl: https://github.com/bluealloy/revm/blob/main/crates/revm/src/evm_impl.rs

When do you think you'll finish - as there'll likely be conflicts? I can try and split up my work. What I've done so far was a lot of investigation, which will be valuable regardless, but the changeset is quite big: https://github.com/Wodann/revm/tree/improvement/return

One thing I can definitely do that's front-facing is add a transaction validation step before we're running the EVM. That should ensure that we can't get NonceOverflow and OutOfFund errors at least

My hope here would be that I can reduce the design to:

pub enum Reason {
    Success(Eval),
    Failure(ExceptionalHalt),
}

EthereumError does not make sense to be put inside evm as every client does rlp in a different place.

I'm merely basing it on the official Eth execution spec: https://github.com/ethereum/execution-specs/blob/24de2192e02bba11e61746aa9dee04d4e7a8b62e/src/ethereum/exceptions.py#L16

Is RLP encoding & decoding infallible in revm?

[fvictorio]

The thing with this:

pub enum Eval {
    Continue = 0x00,
    Stop = 0x01,
    Return = 0x02,
    SelfDestruct = 0x03,
    /// Raised by the `REVERT` opcode.
    ///
    /// Unlike other EVM exceptions this does not result in the consumption of all gas.
    Revert = 0x20,
}

is that the type doesn't encode if the transaction reverted or not, which I feel could be useful (otherwise you have to check the value to see if it's in a "revert" range). The nested Result does encode that information, as far as I understand.

[Wodann]

is that the type doesn't encode if the transaction reverted or not

That's what the Eval::Revert is for, no?

Or do you mean, whether we experienced an ExceptionalHalt and aborted - claiming all gas as cost? Because that is encoded in the enum Reason. If it's a Failure, it reverted when the ExceptionalHalt occurred. If it's one of the Success reasons, we completed the entire transaction.

[rakita]

Hey, really sorry that I am not that responsive, I am on vacation and have a lot of family stuff that I am doing.

pub enum Reason {
    Success(Eval),
    Failure(ExceptionalHalt),
}

This is useful, we can go with this!

Is RLP encoding & decoding infallible in revm? There is not Header,Tx RLP decoding inside any evm.

is that the type doesn't encode if the transaction reverted or not, which I feel could be useful (otherwise you have to check the value to see if it's in a "revert" range). The nested Result does encode that information, as far as I understand.

This revert is more like opcode Revert, and other reverts are Exceptions in execution.

@Wodann most changes that I expect to happen would be located in this function. I see this as localized change and not full replacement of Return:

revm/crates/revm/src/evm_impl.rs

Line 44 in 3a13c9c

fn transact(&mut self) -> (ExecutionResult, State) {

[fvictorio]

@Wodann Maybe I'm not understanding your proposal correctly, but my point is that after sending a transaction to be executed these are the things I would like to know about the result:

1. Was the transaction executed? If not, why?
2. If the transaction was executed, did it revert? If so, why? (It can be a revert, an invalid opcode, an OOG)
3. f the transaction executed successfully, how did it finished? (a stop opcode, a self destruct, etc.)

It seems to me that in that design, doing 1 is easy but doing 2 and 3 not so much, because you have to know which enum values correspond to "succeeded" and which to "reverted". This is similar to what happens now with the Return enum.

[Wodann]

I feel like the second case is covered by Reason::Failure (see above code). Can you validate, @fvictorio ?

[Wodann]

really sorry that I am not that responsive, I am on vacation and have a lot of family stuff that I am doing.

No worries at all. I had enough info to submit a first PR.

If that's good, I can proceed with the Reason enum as well

[rakita]

Cherry-picked ideas from here and came to this: https://github.com/bluealloy/revm/blob/primitives/crates/primitives/src/result.rs#L58-L109

It is only used at the end of transact function as an external variable: https://github.com/bluealloy/revm/blob/primitives/crates/revm/src/evm_impl.rs#L220-L237

Return was renamed to InstructionResult and has mostly unchanged: https://github.com/bluealloy/revm/blob/primitives/crates/interpreter/src/instruction_result.rs

[Wodann]

Overall, looks good to me. I just have one question:

Are there no logs in the case of an ExcecutionResult::Halt - up until the point of the halt?

[rakita]

Overall, looks good to me. I just have one question:

Are there no logs in the case of an ExcecutionResult::Halt - up until the point of the halt?

Should be okay, any Halt is reverting the state, and with revert logs are deleted. Returned Halt (As in ExecutionResult) is only for the call of zero depth (first call), for other depth that Halt would be consumed by the parent scope.

[Wodann]

For deployed contracts that makes most sense, yes. I thought it might be useful for in-development contracts for a developer to be able to observe the logs up until the Halt occurred, though.

[rakita]

For deployed contracts that makes most sense, yes. I thought it might be useful for in-development contracts for a developer to be able to observe the logs up until the Halt occurred, though.

This would override the default mechanism that evm has, reverting everything is what every call is doing,

[rakita]

merged here: #334

[Wodann]

@rakita would you be opposed to convert these types:

pub enum DatabaseComponentError<S: State, BH: BlockHash> {
    State(S::Error),
    BlockHash(BH::Error),
}

pub enum DatabaseComponentRefError<S: StateRef, BH: BlockHashRef> {
    State(S::Error),
    BlockHash(BH::Error),
}

to not use the State[Ref] and BlockHashRef traits but directly receive error types, like such:

pub enum DatabaseComponentError<SE, BHE> {
    State(SE),
    BlockHash(BHE),
}

The reason for this request is that an implementation of State and BlockHash can have complex lifetime definitions, making it harder to use. I also don't think the traits are actually used other than to extract the error types, which could happen at the call site:

fn foo() -> DatabaseComponentError<S::Error, BH::Error>

The example that I'm running into where this poses an issue is the usage: DatabaseComponentError<&'s AsyncState<StateError>, &'b AsyncBlockchain<BlockHashError>>, where the lifetimes don't actually matter for the lifetime of the error and unnecessarily impose constraints on the borrow checker. If I could write this as DatabaseComponentError<StateError, BlockHashError> that would not be an issue.

I see that you already applied this pattern to EVMError<DB>, so it'd be in line with that design. An added benefit is you will no longer need both a DatabaseComponentError and DatabaseComponentRefError

[rakita]

Make sense, didn't think about it and did first thing that came to my mind. Go for it!

[Wodann]

I'll submit a PR shortly