bluealloy / revm

Ethereum Virtual Machine written in rust that is fast and simple to use
https://bluealloy.github.io/revm/
MIT License
1.49k stars 478 forks source link

Return Error if CKZG or BLS features are disabled, but are called. #1435

Open DaniPopes opened 1 month ago

DaniPopes commented 1 month ago

The precompile address should be specified regardless, but with an implementation that panics at run time so that certain bugs are caught early.

rupam-04 commented 1 month ago

Hi, willing to work on this. Can you share some pointers please?

DaniPopes commented 1 month ago

Sure, in revm-precompile lib.rs, instead of conditionally adding precompiles with cfg, they should always be added but with a fallback implementation that panics at runtime.

For example in cancun https://github.com/bluealloy/revm/blob/30c6362bd4676911fa8f3306b4d8311cee340931/crates/precompile/src/lib.rs#L140-L158

the extend should always be enabled, and kzg_point_evaluation::POINT_EVALUATION should be cfg'd internally, with a panic if the feature is not enabled

rakita commented 1 month ago

Would not panic on those. Seems like bad behaviour

rakita commented 1 month ago

Renamed the issue, we should return the error on this.

rakita commented 1 month ago

We should modify PrecompileResult to contain additional field that would return panic like error. https://github.com/bluealloy/revm/blob/30c6362bd4676911fa8f3306b4d8311cee340931/crates/primitives/src/precompile.rs#L9C29-L9C67

Would add something like:

enum PrecompileResult {
    Ok {
      gas_used: u64,
      output: Bytes,
    },
    Error {
        error_type: PrecompileError,
    },
    FatalError {
        msg: String,
    }
}

Fatal error would stop execution, while Ok and Error would work as previously with Result<>.

rakita commented 1 month ago

ref: https://github.com/bluealloy/revm/issues/1283

rakita commented 1 day ago

Implemented for kzg here: https://github.com/bluealloy/revm/pull/1589 blst pending and require some code refactoring