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

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

Open
DaniPopes opened this issue May 17, 2024 · 7 comments
Open

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

DaniPopes opened this issue May 17, 2024 · 7 comments
Labels
bug Something isn't working dependencies Pull requests that update a dependency file good first issue Good for newcomers

Comments

@DaniPopes
Copy link
Collaborator

DaniPopes commented May 17, 2024

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

@DaniPopes DaniPopes added bug Something isn't working good first issue Good for newcomers dependencies Pull requests that update a dependency file labels May 18, 2024
@rupam-04
Copy link

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

@DaniPopes
Copy link
Collaborator Author

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

pub fn cancun() -> &'static Self {
static INSTANCE: OnceBox<Precompiles> = OnceBox::new();
INSTANCE.get_or_init(|| {
let precompiles = Self::berlin().clone();
// Don't include KZG point evaluation precompile in no_std builds.
#[cfg(feature = "c-kzg")]
let precompiles = {
let mut precompiles = precompiles;
precompiles.extend([
// EIP-4844: Shard Blob Transactions
kzg_point_evaluation::POINT_EVALUATION,
]);
precompiles
};
Box::new(precompiles)
})
}

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
Copy link
Member

rakita commented May 23, 2024

Would not panic on those. Seems like bad behaviour

@rakita rakita changed the title Panic if CKZG or BLS features are disabled, but relevant spec is enabled Return Error if CKZG or BLS features are disabled, but are called. May 23, 2024
@rakita
Copy link
Member

rakita commented May 23, 2024

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

@rakita
Copy link
Member

rakita commented May 23, 2024

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
Copy link
Member

rakita commented May 23, 2024

ref: #1283

@rakita
Copy link
Member

rakita commented Jul 8, 2024

Implemented for kzg here: #1589
blst pending and require some code refactoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants