feat: migrate reth dependencies from v1.10.2 to v1.11.0 with optimism…

[louisliu2048]

… submodule

Description

Type of Change

• 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)
• Documentation update
• Refactoring (no functional changes)
• Other (please describe):

Code Guidelines

Before submitting your PR, please review the relevant code guidelines in the docs/ folder:

• Building on Reth Guide - Comprehensive guide for building on top of Reth

Specific Guidelines by Component:

• Chainspec & EVM Guide - For changes to chainspec or EVM configuration
• Database Guide - For changes to database tables or storage
• Engine & Consensus Guide - For changes to consensus or engine API
• Extension Guide - For adding extensions or plugins
• Node Builder Guide - For changes to node initialization
• Payload Builder Guide - For changes to block building logic

Checklist

• I have reviewed the relevant code guidelines in the docs/ folder
• My code follows the coding standards of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Testing

Additional Notes

[Vui-Chee]

Notes:

• bin/tools/import.rs: defaulting ImportConfig.fail_on_invalid_block to false changes import strictness; is this intentional for all environments or should it be configurable via CLI?
• Cargo.toml: please confirm all op-alloy crates pulled transitively by reth v1.11.0 are redirected to the local optimism workspace to avoid duplicate type definitions.

[Vui-Chee]

Both notes are not needed:

• fail_on_invalid_block: required field added to match upstream ImportConfig API (not a behavior choice)
• op-alloy patching: comprehensive [patch.crates-io] section already covers all relevant crates

LGTM!

[louisliu2048]

@claude help to review the code

[claude]

Claude finished @louisliu2048's task in 2m 19s —— View job

---

Code Review — feat: migrate reth dependencies from v1.10.2 to v1.11.0 with optimism submodule

• Gather context and understand the PR
• Review Cargo.toml dependency changes
• Review new files (builder_tx.rs, standard/service.rs)
• Review modified builder files
• Review flashblocks and other crate changes
• Review Docker/tooling changes
• Provide final summary

---

This is a well-structured version migration. The approach of splitting reth base deps (from upstream paradigmxyz/reth v1.11.0) from OP-specific deps (via local deps/optimism submodule) is clean and easier to maintain than a single okx/reth fork. Below are findings ordered by severity.

---

Bugs / Issues

1. commit_txs silently commits reverted/halted transactions (crates/builder/src/builders/builder_tx.rs:293)

fn commit_txs(&self, signed_txs: ...) -> Result<(), BuilderTransactionError> {
    for signed_tx in signed_txs {
        let ResultAndState { state, .. } = evm.transact(&signed_tx)...?;
        evm.db_mut().commit(state)   // committed even if result is Revert/Halt
    }
}

ResultAndState is destructured ignoring the result. If a transaction reverts or halts, its state changes (potentially wasted gas, failed storage writes) are still committed to the DB. Compare with add_builder_txs which correctly checks result.is_success() before committing. This method should do the same. Fix this →

2. simulate_call log validation is too permissive (crates/builder/src/builders/builder_tx.rs:332)

let topics: HashSet<B256> =
    logs.into_iter().flat_map(|log| log.topics().to_vec()).collect();
if !expected_logs.iter().all(|expected_topic| topics.contains(expected_topic)) { ... }

All topics from all logs are merged into a single HashSet. This means the check only verifies that each expected topic appears somewhere in any log, not that any specific log emitted a specific event. An adversarial or buggy contract could emit unrelated logs containing the expected topics. Consider matching on (address, topic) pairs or checking per-log, depending on how strict the validation needs to be.

---

Minor Issues / Nits

3. Typo in standard/service.rs line 82

"Failed to bootstrap flashtestations, builderb will not include flashtestations txs"

builderb → builder. Fix this →

4. alloy-primitives pinned to ~1.5 while other alloy crates are ~1.6 (Cargo.toml:171)

The comment explains this is intentional to avoid secp256k1 incompatibility, but it creates a version mismatch within the alloy ecosystem that could cause subtle type incompatibilities. Recommend adding an inline comment on the alloy-primitives entry explaining why it stays on 1.5 while others are 1.6 (the secp256k1 reason is currently only in the block comment above).

5. Test macros if_standard! / if_flashblocks! are effectively hardcoded (crates/builder/src/tests/mod.rs:8)

macro_rules! if_flashblocks { ($($body:tt)*) => { $($body)* }; }   // always expands
macro_rules! if_standard!   { ($($body:tt)*) => {};               }  // always discards

This means the standard-mode assertions in data_availability.rs (checking 8 txs, index 0–7) will never run. If there's a regression in the standard builder's DA behaviour, the test won't catch it. Consider adding a feature flag or cfg attribute (e.g., #[cfg(feature = "flashblocks")]) to make these macros switch based on build configuration, rather than hard-coding one mode.

6. gas_limit in sign_tx uses EIP-150 buffer but estimate_builder_tx_gas does not (builder_tx.rs:260,380)

sign_tx correctly applies gas_used * 64 / 63 to account for EIP-150 call forwarding. However, BuilderTxBase::signed_builder_tx (used internally) sets gas_limit: gas_used without the buffer — a simple self-transfer to Address::ZERO shouldn't need an external call buffer, so this is probably fine, but worth an explanatory comment to avoid future confusion.

---

Positive Observations

• Dependency split is clean: separating upstream paradigmxyz/reth (by stable tag) from OP-specific crates (local submodule) is much easier to reason about than a single fork rev.
• [patch.crates-io] section is thorough and well-documented: the comments clearly explain which transitive deps each patch redirects, making future upgrades straightforward.
• spawn_blocking → sync closure (crates/flashblocks/src/service.rs:460): switching from spawn_blocking(Box::pin(async move {...})) to spawn_blocking(move || {...}) is the correct API usage for CPU-bound blocking work.
• ReceiptBuilderCtx API change (tx → tx_type): passing only the tx type instead of the full transaction is a cleaner abstraction, consistently applied across all call sites.
• TaskManager → Runtime: the test framework migration to Runtime::test_with_handle() and graceful_shutdown_with_timeout properly tracks the upstream reth API change.
• BuilderTransactions trait (builder_tx.rs): good separation between simulation (on a state copy) and actual execution, with the invalid signer set preventing cascading failures from a single bad signer.
• fail_on_invalid_block: false (bin/tools/import.rs): correctly identified by reviewer as a required API field match, not a behavior choice.

---

Summary

The migration is overall solid and the architectural decision to use a git submodule for the optimism monorepo deps is a clear improvement. The main actionable issue is the commit_txs revert bug (item 1) — the rest are minor. The two approved reviews (sieniven, Vui-Chee) have already resolved their concerns.

[claude]

This is a major dependency migration (reth v1.10.2 → v1.11.0) spanning 26 files with significant architectural changes — switching from the okx/reth fork to upstream paradigmxyz/reth plus a local optimism git submodule — that warrants careful human review.

Extended reasoning...

Overview

This PR migrates ~50+ reth dependencies from the okx/reth fork (pinned by git rev) to upstream paradigmxyz/reth v1.11.0, while moving optimism-specific crates to local path dependencies via a new deps/optimism git submodule. It touches 26 files including Cargo.toml (dependency sources + version bumps + new [patch.crates-io] section), Dockerfile, task spawning APIs across multiple crates, receipt builder API changes (tx → tx_type), test framework infrastructure (TaskManager → Runtime, RollupArgs defaults), new trait bounds (Storage = OpStorage), and two new dead-code scaffolding files.

Security risks

No direct security vulnerabilities were identified. The dependency sources shift from a private fork to upstream + local submodule, which is generally positive for staying current with upstream security fixes. The thiserror upgrade from 1.x to 2.x and secp256k1 default-features change are low-risk but worth verifying compatibility. The [patch.crates-io] section redirecting op-alloy crates to local paths needs careful review to ensure no type mismatches at crate boundaries.

Level of scrutiny

This PR requires thorough human review. It is a foundational dependency migration that affects the entire build graph. The switch from a fork to upstream + submodule is an architectural decision with long-term maintenance implications. The API changes (spawn_blocking → spawn_blocking_task, spawn_critical → spawn_critical_task, spawn → spawn_task) span multiple crates and need verification that the new APIs have equivalent semantics. The test infrastructure changes (dropping enable_tx_conditional, TaskManager → Runtime) could mask regressions.

Other factors

The three bugs found are all nits: dead scaffolding code in builders/, a dropped DA test assertion for tx_hashes[8], and enable_tx_conditional defaulting to false in tests. None are blocking, but they indicate the migration may have some rough edges. The PR description is empty (no checkboxes filled, no testing description), which makes it harder to assess the author's confidence level. The Cargo.lock diff (not shown) would be critical to review for unexpected transitive dependency changes.

[claude]

🟡 .reth-dev.toml was not updated to reflect the dependency migration from okx/reth to paradigmxyz/reth v1.11.0. The entire [patch."https://github.com/okx/reth"] section (lines 1-61) is now dead config since no Cargo.toml dependency references that URL anymore, both sections reference removed crates (reth-trie-db, reth-db-common, reth-execution-errors, etc.), and new crates added in this PR (reth-optimism-storage, reth-trie-common, reth-optimism-flashblocks) are missing. Any developer running just build-dev after this PR merges will get broken patch resolution.

Extended reasoning...

What the bug is

The .reth-dev.toml file is a template used by the justfile's build-dev command (and related commands like build-docker-dev, validate-reth-dev) to generate .cargo/config.toml for local development builds. It contains [patch] sections that redirect git dependencies to local filesystem paths via sed placeholder substitution. After this PR migrates all reth dependencies from okx/reth to paradigmxyz/reth v1.11.0, the .reth-dev.toml file was not updated to match.

The specific issues

1. Dead config section: The [patch."https://github.com/okx/reth"] section (lines 1-61) patches a URL that is no longer referenced anywhere in Cargo.toml. All base reth dependencies now point to paradigmxyz/reth, making this entire section useless.

2. References to removed crates: Both patch sections reference crates that no longer exist in reth v1.11.0: reth-db-common, reth-execution-errors, reth-evm-ethereum, reth-node-ethereum, reth-trie-db, and reth-trie-parallel. These would cause Cargo patch resolution errors.

3. Missing new crates: Several crates added to the workspace in this PR are absent from the template: reth-optimism-storage, reth-trie-common, reth-optimism-flashblocks, and others. Without these patches, local dev builds would have version conflicts between the git-sourced and local versions of these crates.

4. OP crates misplaced: OP-specific crates like reth-optimism-chainspec are still listed under the paradigmxyz/reth patch section, but in the PR they now come from local deps/optimism/ path dependencies rather than from the reth git repo.

Step-by-step proof

1. Cargo.toml no longer contains any dependency with git = "https://github.com/okx/reth" — all such entries were replaced with git = "https://github.com/paradigmxyz/reth" or local path = "deps/optimism/..." entries.
2. .reth-dev.toml line 1 still declares [patch."https://github.com/okx/reth"] — this patches a URL that no Cargo.toml dependency uses, so Cargo ignores it entirely.
3. .reth-dev.toml lines 10, 60-61 reference reth-db-common, reth-trie-db, reth-trie-parallel — crates that don't exist in reth v1.11.0. When a developer runs just build-dev /path/to/reth-v1.11.0, the generated .cargo/config.toml would reference non-existent paths.
4. The justfile's validate-reth-dev recipe (line 131) explicitly checks that .reth-dev.toml stays in sync with Cargo.toml, and sync-reth-dev (line 194) is designed to update it. Neither was run as part of this PR.

Impact

This only affects the local development workflow (just build-dev), not production builds which use Cargo.toml directly. However, any developer who uses this workflow after merging this PR will encounter broken builds and will need to manually update the template or run just sync-reth-dev (which itself may need updates since the sync-reth-dev script greps for okx/reth entries that no longer exist).

How to fix

Run just sync-reth-dev /path/to/reth with a v1.11.0 reth checkout to regenerate the file, or manually update .reth-dev.toml to: (1) remove the dead [patch."https://github.com/okx/reth"] section, (2) remove references to deleted crates, (3) add entries for all new crates in the workspace dependencies, and (4) ensure OP-specific crates that now come from deps/optimism/ are handled appropriately.

[claude]

🟡 Unused import use core::fmt::Debug; on line 18. The only reference to Debug in this file is #[derive(Debug)] on line 52, which is a derive macro that fully qualifies the trait path internally and does not require an explicit use import. This will trigger an unused_imports warning under clippy.

Extended reasoning...

What the bug is

The PR adds use core::fmt::Debug; at line 18 of crates/builder/src/flashblocks/context.rs. This import is not used anywhere in the file.

How it manifests

The only reference to Debug in this file is the #[derive(Debug)] attribute on line 52 for the FlashblocksBuilderCtx struct. Derive macros in Rust generate trait implementations using fully qualified paths internally (e.g., core::fmt::Debug), so they do not require the trait to be in scope via a use import. A grep for Debug in the file confirms only two occurrences: the import on line 18 and the derive on line 52.

Why existing code does not prevent it

This import was added alongside the OpTxType import on line 19 as part of the migration refactor. It likely was added speculatively during development (perhaps for a trait bound that was later removed) and was not caught before the PR was submitted. The Rust compiler emits an unused_imports warning for this, and under the project’s clippy configuration (-D warnings), this warning becomes a hard error.

Impact

If CI enforces cargo clippy --all-targets --workspace -- -D warnings (as documented in CLAUDE.md), this unused import will cause a build failure. Even without strict CI enforcement, it is dead code that adds noise.

How to fix

Simply remove line 18:

- use core::fmt::Debug;

Step-by-step proof

1. Line 18: use core::fmt::Debug; — this is the import under question.
2. Line 52: #[derive(Debug)] — this is the only occurrence of Debug in the file.
3. #[derive(Debug)] expands to impl core::fmt::Debug for FlashblocksBuilderCtx { ... } using fully qualified paths. It does not require Debug to be in scope.
4. No function signatures, trait bounds, or other code in this file reference the Debug trait directly.
5. Therefore, the import on line 18 is unused and will trigger a clippy warning.