search

paradigmxyz / reth

Modular, contributor-friendly and blazing-fast implementation of the Ethereum protocol, in Rust
https://reth.rs/
Apache License 2.0
4.01k stars 1.23k forks source link

Feature audit: `optimism` #11116

Open onbjerg opened 2 months ago

onbjerg commented 2 months ago

Describe the feature

A lot of optimism crates have an optimism feature that enables optimism features in common crates. The optimism specific crates should just have these enabled by default, and not have them gated by an optimism feature.

For example, reth-evm-optimism has this feature which is pointless, it should just enable optimism in all the subcrates by default:

https://github.com/paradigmxyz/reth/blob/2684594ec163ec8553b633356960a9711fd2b688/crates/optimism/evm/Cargo.toml#L44-L50

We should go over all op-reth specific crates and remove the optimism feature, and instead enable the optimism feature on common subcrates by default in these

Additional context

No response

0xkrane commented 2 months ago

can take this

just to understand, there are two parts:

onbjerg commented 2 months ago

I mean for example in reth-evm-optimism we depend on reth-primitives. That crate has a feature called optimism which is only activated when the feature is also enabled in reth-evm-optimism. Instead, we should remove the optimism feature from reth-evm-optimism, and just enable optimism on reth-primitives directly. Does that make sense?

0xkrane commented 2 months ago

thanks! yeah that makes sense. its what i thought you meant but just wasn't sure what common meant there. Will work on this.

emhane commented 2 months ago

I mean for example in reth-evm-optimism we depend on reth-primitives. That crate has a feature called optimism which is only activated when the feature is also enabled in reth-evm-optimism. Instead, we should remove the optimism feature from reth-evm-optimism, and just enable optimism on reth-primitives directly. Does that make sense?

I don't think it's possible to do this for all op crates, because of the remaining optimism features in non-op-reth crates, but we can try complete this issue for some optimism crates

onbjerg commented 2 months ago

What crates would this not be possible for?

emhane commented 1 month ago

for example this optimism-feature related bug appears, wasn't straight forward to debug so didn't allocate more time to it previously when I tried

2024-09-24T04:10:34.667888Z ERROR reth_tasks: Critical task `payload builder service` panicked: `This should not be called in optimism mode. Use `optimism_receipts_root_slow` instead.`

you can see this error in failing test in https://github.com/paradigmxyz/reth/pull/11136

onbjerg commented 1 month ago

If all the optimism features in the common crates are enabled, and all the optimism features are removed from the optimism specific crates, as this issue suggests, then this should be a non issue:)