Skip to content

chore(edr_op): remove the unsupported OpHardfork::OSAKA variant - #1603

Merged
anaPerezGhiglia merged 15 commits into
mainfrom
chore/remove-op-hardfork-osaka
Aug 20, 2026
Merged

chore(edr_op): remove the unsupported OpHardfork::OSAKA variant#1603
anaPerezGhiglia merged 15 commits into
mainfrom
chore/remove-op-hardfork-osaka

Conversation

@anaPerezGhiglia

Copy link
Copy Markdown
Contributor

#1601 was meant to be behavior-preserving with respect to the currently pinned revm/op-revm versions, so OpHardfork mirrored every OpSpecId variant, including OSAKA. EDR currently does not support Osaka-equivalent execution in OP.

op-revm replaced the Osaka variant with Karst (the actual OP hardfork) in 20.0.0, published to crates.io through ethereum-optimism/optimism#20068.

This PR removes the variant, since we don't support Karst in OP. It is dormant, unreachable from the napi API (which stops at Isthmus) and never constructed (activations end at Jovian), so the removal is behavior-preserving. Whenever Karst is supported, the variant should be incorporated again under the right name.

@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 7244794

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@anaPerezGhiglia anaPerezGhiglia changed the title chore: remove OpHardfork::Osaka since is not an actual OP stack hardfork chore(edr_op): remove the unsupported OpHardfork::OSAKA variant Aug 6, 2026
@anaPerezGhiglia
anaPerezGhiglia requested a review from Copilot August 6, 2026 18:23
@anaPerezGhiglia anaPerezGhiglia added the no changeset needed This PR doesn't require a changeset label Aug 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Removes the OSAKA hardfork variant from the local OpHardfork model and related conversions, aligning supported hardforks with current OP needs and updating tests accordingly.

Changes:

  • Removed OSAKA from OpHardfork, including string identifiers and op_revm::OpSpecId conversion.
  • Updated FromStr / &'static str conversions to no longer accept/emit "Osaka".
  • Adjusted test variant list and updated revm parity module documentation.
Suppressed comments (1)

crates/edr_op/src/hardfork.rs:124

  • Removing support for parsing \"Osaka\" is an externally-observable behavior change: inputs that previously parsed will now return UnknownHardfork. If this string can appear in persisted configs/fixtures, consider keeping \"Osaka\" as a deprecated alias (either mapping to a chosen replacement or returning a more actionable error) or documenting/migrating callers accordingly.
impl FromStr for OpHardfork {
    type Err = UnknownHardfork;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s {
            name::BEDROCK => Ok(Self::BEDROCK),
            name::REGOLITH => Ok(Self::REGOLITH),
            name::CANYON => Ok(Self::CANYON),
            name::ECOTONE => Ok(Self::ECOTONE),
            name::FJORD => Ok(Self::FJORD),
            name::GRANITE => Ok(Self::GRANITE),
            name::HOLOCENE => Ok(Self::HOLOCENE),
            name::ISTHMUS => Ok(Self::ISTHMUS),
            name::JOVIAN => Ok(Self::JOVIAN),
            name::INTEROP => Ok(Self::INTEROP),
            _ => Err(UnknownHardfork),
        }
    }
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +257 to +259
/// guarantee that the owned enum matches op-revm for every variant it
/// defines during the transition; DELETE THIS MODULE when an op-revm
/// upgrade removes variants from `OpSpecId`.
Comment thread crates/edr_op/src/hardfork.rs Outdated
Comment on lines 109 to 123
name::OSAKA => Ok(Self::OSAKA),
_ => Err(UnknownHardfork),
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary

@anaPerezGhiglia
anaPerezGhiglia marked this pull request as ready for review August 6, 2026 18:44
@anaPerezGhiglia
anaPerezGhiglia requested a review from Wodann August 6, 2026 18:44
Wodann and others added 12 commits August 12, 2026 23:49
…ponding associated type names

Two sites got narrower bounds:

chain/spec/evm/src/lib.rs:27 — ContextForChainSpec only needs the EVM-level type, so it now projects through EvmHardforkChainSpec.
edr_napi_core/src/solidity/config.rs:224-225 — the combined bound decomposed into EvmHardforkChainSpec<EvmHardfork: HardforkTr> + ProtocolHardforkChainSpec<ProtocolHardfork: FromStr<Err = UnknownHardfork>>.
…spec

`PartialHeader::new` required `HardforkT: ProtocolHardfork` solely to reach
`ProtocolParams::bomb_delay` for the Ethash fallback, which pushed L1
proof-of-work policy onto every chain: `OpHardfork::bomb_delay` was an
`unreachable!()` and `OpChainSpec::MIN_ETHASH_DIFFICULTY` a meaningless 0.

Chains now supply the computation as
`ProviderChainSpec::default_block_difficulty`, carried down to the header
layer as a `fn` pointer on `BlockConfig`. A pointer rather than a closure
parameter because the reservation path resolves lazily behind `RwLock`s,
arbitrarily far from `reserve_blocks`, with only the cloned `BlockConfig`
crossing that boundary - and both it and `Reservation` derive `Clone`/`Debug`.
No trait bound had to be widened to make this reachable.

The Ethash formula moves to `edr_chain_l1::difficulty`, its only consumer.
Its `assert!(spec_id >= BYZANTIUM)` and the `unreachable!()` in `bomb_delay`
are both replaced by `PreMergeL1Hardfork`, whose `TryFrom<L1Hardfork>`
failure *is* the post-merge branch, leaving `bomb_delay` total over an
exhaustive match.

`MIN_ETHASH_DIFFICULTY` and `BlockConfig::min_ethash_difficulty` are gone,
and `ProtocolParams` keeps only `miner_reward`. `PartialHeader::new` and the
storage/blockchain layers that forward to it now require just
`Copy + Into<EvmSpecId> + PartialOrd`.
`BlockBuilder::finalize_block` took a `rewards` vector that every caller
built identically, out of values the builder already owned: the amount
from the blockchain's hardfork, and the recipient read straight off
`block_builder.header().beneficiary`. The amount was threaded down from
`edr_provider` as a `u128` so the builder could be told a number it
could compute itself.

`EthBlockBuilder` used to be both the shared Ethereum block-building
machinery and L1's `BlockBuilder` implementation, while OP wrapped it.
That dual role is what made the reward awkward to place, since the
reward is L1-only but the engine is instantiated with `OpHardfork` too.
It now loses its `BlockBuilder` impl and keeps only the engine role: the
reward loop becomes `apply_rewards`, and `finalize` is reward-free. A new
`L1BlockBuilder` wraps it and implements `BlockBuilder`, pinned to L1's
hardfork type so it can pay the reward; `OpBlockBuilder` simply doesn't.
Behavior is unchanged for OP, whose reward was already zero and skipped.

With the reward L1-local, `ProtocolParams` has no members left. Deleting
it reduces `ProtocolHardfork` to `Copy + Into<EvmSpecId> + PartialOrd`,
which is purely derivable, and removes the `miner_reward` stub OP only
carried to answer `None`.
`ProtocolHardfork::to_evm_spec_id` was a named `Into<EvmSpecId>` conversion.
Remove it: call sites that can infer the target become a bare `.into()`; the
rest annotate `let evm_spec_id: EvmSpecId = ...`, because protocol hardforks
carry two `Into` bounds — `Into<Self::EvmHardfork>` and `Into<EvmSpecId>` —
which coincide for L1 but not for a chain with its own EVM hardfork type.
That ambiguity is now documented on the associated type.

`ProtocolHardfork` itself stays, reduced to a blanket-implemented alias for
`Copy + Into<EvmSpecId> + PartialOrd`: the hardfork-generic block and
blockchain crates have no chain spec in scope to carry these bounds through
an associated type, so the alias names the bundle once instead of repeating
it at thirteen sites.
@anaPerezGhiglia
anaPerezGhiglia temporarily deployed to github-action-benchmark August 19, 2026 22:24 — with GitHub Actions Inactive
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.99%. Comparing base (0611870) to head (7244794).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1603      +/-   ##
==========================================
- Coverage   80.00%   79.99%   -0.01%     
==========================================
  Files         453      453              
  Lines       79293    79292       -1     
  Branches    79293    79292       -1     
==========================================
- Hits        63440    63432       -8     
- Misses      13673    13677       +4     
- Partials     2180     2183       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anaPerezGhiglia
anaPerezGhiglia temporarily deployed to github-action-benchmark August 19, 2026 22:32 — with GitHub Actions Inactive
@anaPerezGhiglia
anaPerezGhiglia temporarily deployed to github-action-benchmark August 19, 2026 22:32 — with GitHub Actions Inactive
Base automatically changed from refactor/edr-owned-hardfork-types to main August 20, 2026 13:50
@anaPerezGhiglia
anaPerezGhiglia temporarily deployed to github-action-benchmark August 20, 2026 13:58 — with GitHub Actions Inactive
@anaPerezGhiglia
anaPerezGhiglia added this pull request to the merge queue Aug 20, 2026
Merged via the queue into main with commit d2064e1 Aug 20, 2026
36 checks passed
@anaPerezGhiglia
anaPerezGhiglia deleted the chore/remove-op-hardfork-osaka branch August 20, 2026 14:13
@anaPerezGhiglia
anaPerezGhiglia had a problem deploying to github-action-benchmark August 20, 2026 14:26 — with GitHub Actions Failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changeset needed This PR doesn't require a changeset

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants