Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Commit

Permalink
Change ExecError related functions to be associated-fns (#1450)
Browse files Browse the repository at this point in the history
### Description

This PR attempts to associate the function `get_step_reported_error` to
`ExecError`.

### Issue Link

Resolves #1181 

### Type of change

- [x ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

### Contents

### Rationale
We wanted to move the function `get_step_reported_error` to an `imp
ExecError`, I also moved the part related to `OogError` to its own
associated function `From<OpCodeId> for OogError`. I changed an if/else
block into a match expecting to handle more errors for CREATE/2.

Personally I would update the function `get_step_reported_error` to take
as input an `GethExecStep` and in that case directly implement it as a
`From<GethExecStep> for ExecError` but the return type should be changed
to `Option<ExecError>` since the step may not have an error (step.error
is optional).

### How Has This Been Tested?
No tests were added as no new functionalities were added.
  • Loading branch information
Raphael authored Jun 8, 2023
1 parent 06cce9f commit 9620822
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 42 deletions.
9 changes: 3 additions & 6 deletions bus-mapping/src/circuit_input_builder/input_state_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ use super::{
TransactionContext,
};
use crate::{
error::{
get_step_reported_error, DepthError, ExecError, InsufficientBalanceError,
NonceUintOverflowError,
},
error::{DepthError, ExecError, InsufficientBalanceError, NonceUintOverflowError},
exec_trace::OperationRef,
operation::{
AccountField, AccountOp, CallContextField, CallContextOp, MemoryOp, Op, OpEnum, Operation,
Expand Down Expand Up @@ -1155,8 +1152,8 @@ impl<'a> CircuitInputStateRef<'a> {
step: &GethExecStep,
next_step: Option<&GethExecStep>,
) -> Result<Option<ExecError>, Error> {
if let Some(error) = &step.error {
return Ok(Some(get_step_reported_error(&step.op, error)));
if let Ok(error) = ExecError::try_from(step) {
return Ok(Some(error));
}

if matches!(step.op, OpcodeId::INVALID(_)) {
Expand Down
88 changes: 52 additions & 36 deletions bus-mapping/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,39 @@ pub enum OogError {
SelfDestruct,
}

// Given OpCodeId, returns correponding OogError.
impl From<&OpcodeId> for OogError {
fn from(op: &OpcodeId) -> Self {
match op {
OpcodeId::MLOAD | OpcodeId::MSTORE | OpcodeId::MSTORE8 => {
OogError::StaticMemoryExpansion
}
OpcodeId::CREATE | OpcodeId::RETURN | OpcodeId::REVERT => {
OogError::DynamicMemoryExpansion
}
OpcodeId::CALLDATACOPY
| OpcodeId::CODECOPY
| OpcodeId::EXTCODECOPY
| OpcodeId::RETURNDATACOPY => OogError::MemoryCopy,
OpcodeId::BALANCE | OpcodeId::EXTCODESIZE | OpcodeId::EXTCODEHASH => {
OogError::AccountAccess
}
OpcodeId::LOG0 | OpcodeId::LOG1 | OpcodeId::LOG2 | OpcodeId::LOG3 | OpcodeId::LOG4 => {
OogError::Log
}
OpcodeId::EXP => OogError::Exp,
OpcodeId::SHA3 => OogError::Sha3,
OpcodeId::CALL | OpcodeId::CALLCODE | OpcodeId::DELEGATECALL | OpcodeId::STATICCALL => {
OogError::Call
}
OpcodeId::SLOAD | OpcodeId::SSTORE => OogError::SloadSstore,
OpcodeId::CREATE2 => OogError::Create2,
OpcodeId::SELFDESTRUCT => OogError::SelfDestruct,
_ => OogError::Constant,
}
}
}

/// Insufficient balance errors by opcode/state.
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum InsufficientBalanceError {
Expand Down Expand Up @@ -164,43 +197,26 @@ pub enum ExecError {
NonceUintOverflow(NonceUintOverflowError),
}

// TODO: Move to impl block.
pub(crate) fn get_step_reported_error(op: &OpcodeId, error: &str) -> ExecError {
if error == GETH_ERR_OUT_OF_GAS || error == GETH_ERR_GAS_UINT_OVERFLOW {
// NOTE: We report a GasUintOverflow error as an OutOfGas error
let oog_err = match op {
OpcodeId::MLOAD | OpcodeId::MSTORE | OpcodeId::MSTORE8 => {
OogError::StaticMemoryExpansion
}
OpcodeId::CREATE | OpcodeId::RETURN | OpcodeId::REVERT => {
OogError::DynamicMemoryExpansion
}
OpcodeId::CALLDATACOPY
| OpcodeId::CODECOPY
| OpcodeId::EXTCODECOPY
| OpcodeId::RETURNDATACOPY => OogError::MemoryCopy,
OpcodeId::BALANCE | OpcodeId::EXTCODESIZE | OpcodeId::EXTCODEHASH => {
OogError::AccountAccess
// Returns a GethExecStep's error if present, else return the empty error.
impl TryFrom<&GethExecStep> for ExecError {
type Error = ();

fn try_from(step: &GethExecStep) -> Result<Self, Self::Error> {
Ok(match step.error.as_ref().ok_or(())?.as_str() {
GETH_ERR_OUT_OF_GAS | GETH_ERR_GAS_UINT_OVERFLOW => {
// NOTE: We report a GasUintOverflow error as an OutOfGas error
let oog_err = OogError::from(&step.op);
ExecError::OutOfGas(oog_err)
}
OpcodeId::LOG0 | OpcodeId::LOG1 | OpcodeId::LOG2 | OpcodeId::LOG3 | OpcodeId::LOG4 => {
OogError::Log
error => {
if error.starts_with(GETH_ERR_STACK_OVERFLOW) {
ExecError::StackOverflow
} else if error.starts_with(GETH_ERR_STACK_UNDERFLOW) {
ExecError::StackUnderflow
} else {
panic!("Unknown GethExecStep.error: {}", error);
}
}
OpcodeId::EXP => OogError::Exp,
OpcodeId::SHA3 => OogError::Sha3,
OpcodeId::CALL | OpcodeId::CALLCODE | OpcodeId::DELEGATECALL | OpcodeId::STATICCALL => {
OogError::Call
}
OpcodeId::SLOAD | OpcodeId::SSTORE => OogError::SloadSstore,
OpcodeId::CREATE2 => OogError::Create2,
OpcodeId::SELFDESTRUCT => OogError::SelfDestruct,
_ => OogError::Constant,
};
ExecError::OutOfGas(oog_err)
} else if error.starts_with(GETH_ERR_STACK_OVERFLOW) {
ExecError::StackOverflow
} else if error.starts_with(GETH_ERR_STACK_UNDERFLOW) {
ExecError::StackUnderflow
} else {
panic!("Unknown GethExecStep.error: {}", error);
})
}
}

0 comments on commit 9620822

Please sign in to comment.