deps: downgrade reth to paradigmxyz/reth v1.10.0#46
Open
chengwenxi wants to merge 5 commits intomainfrom
Open
Conversation
….0.0 workaround Downgrade from morph-l2/reth@8057627 (v1.10.2-based, revm-state 9.0.0) to paradigmxyz/reth@b25f32a (v1.10.0, revm-state 8.1.1) to avoid the EIP-2200 gas regression introduced by revm-state 9.0.0's mark_warm_with_transaction_id resetting original_value on cold→warm slot transitions. Dependency changes: - reth: morph-l2/reth@8057627 → paradigmxyz/reth@b25f32a (v1.10.0) - revm: 34.0.0 → 33.1.0 (revm-state 9.0.0 → 8.1.1) - alloy: 1.6.3 → 1.4.3 - alloy-evm: 0.26.4 → 0.25.1 - alloy-primitives/alloy-sol-types: 1.5.6 → 1.5.0 - alloy-rlp: 0.3.13 → 0.3.10 - alloy-chains: 0.2.30 → 0.2.5 - alloy-hardforks: 0.4.7 → 0.4.5 PR #39 workaround fully removed: - Remove sload_morph custom SLOAD instruction override from MorphEvm - Remove fee_slot_original_values field from MorphTxEnv - Remove fee slot saving/restore logic from validate_and_deduct_token_fee Other adaptations for v1.10.0 API: - with_spec_and_mainnet_gas_params → with_spec + disable_eip7623 field - Remove tx_count_hint from EthBlockExecutionCtx construction - BlockExecutionOutput → ExecutionOutcome in payload builder - Remove receipts() from BlockExecutor impl (not in v1.10.0 trait) - Remove ReceiptRootBloom, StateRootValidator, ChangesetCache (not in paradigmxyz/reth) - Add [patch.crates-io] for vergen 9.0.6 to fix vergen/vergen-git2 lib incompatibility
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates dependency pins; removes pre-computed receipts handling; drops custom SLOAD override and fee-slot preservation; simplifies validator/geth RPC and state-root wiring; switches payload execution result to ExecutionOutcome; adjusts several handler/EVM config signatures and removed fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/revm/src/handler.rs (1)
254-264:⚠️ Potential issue | 🟡 MinorFix formatting issue flagged by CI.
The pipeline reports a
cargo fmtfailure at line 256 in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/handler.rs` around lines 254 - 264, CI flagged a formatting error in the validate_initial_tx_gas function; run rustfmt (cargo fmt) and reformat the block around fn validate_initial_tx_gas, ensuring proper indentation and spacing for the function signature and the lines referencing evm.ctx_ref().tx(), evm.ctx_ref().cfg().spec().into(), and the InitialAndFloorGas type so the file passes cargo fmt.crates/node/src/validator.rs (1)
118-125:⚠️ Potential issue | 🟠 MajorDon't leave
geth_rpc_urlas a silent no-op.After removing the extra validator hook here, nothing in the remaining validation path consumes
geth_rpc_urlanymore.with_geth_rpc_url()still accepts the setting and still logs that state-root cross-validation is enabled, so operators can think a safety check is active when it is not. Either remove that knob/log now or wire it into the new validator path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/validator.rs` around lines 118 - 125, The code currently constructs BasicEngineValidator via BasicEngineValidator::new(...) but drops any effect of geth_rpc_url/with_geth_rpc_url; either remove the configuration/logging or rewire it into the new validator path. Fix by updating the builder/BasicEngineValidator::new signature or its construction here to accept the geth_rpc_url (or a boolean flag) and thread that into the validator implementation so the state-root cross-validation code is actually invoked, or alternatively remove the with_geth_rpc_url() call and its "cross-validation enabled" log so it is not misleading; refer to BasicEngineValidator::new, with_geth_rpc_url, geth_rpc_url and invalid_block_hook when locating and updating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 183-186: The patch in Cargo.toml under [patch.crates-io] currently
uses an absolute local path for vergen (vergen = { path =
"/tmp/vergen-9.0.6-local" }) which breaks CI; replace that absolute path with a
repository-reachable source such as a repo-relative vendored path (e.g., path =
"vendor/vergen-9.0.6"), a git URL with the 9.0.6 tag (e.g., vergen = { git =
"...", tag = "v9.0.6" }), or remove the patch and pin the crate to the exact
version (vergen = "=9.0.6") so that the [patch.crates-io] entry for vergen is
consistently resolvable in CI and fresh clones.
In `@crates/node/src/validator.rs`:
- Around line 97-103: Format the Rust source to satisfy rustfmt: run `cargo fmt`
(or `rustfmt`) on this file to fix the spacing/indentation in the impl/generic
block that references PVB::Validator and the type alias EngineValidator (where
BasicEngineValidator<Node::Provider, Node::Evm, PVB::Validator> is defined), as
well as the later hunk near the code around line ~283; ensure the generics and
trait bounds (reth_node_api::PayloadValidator, <Node::Types as
NodeTypes>::Payload, Block = reth_node_api::BlockTy<Node::Types>) are correctly
wrapped/indented so `cargo fmt --check` passes.
In `@crates/revm/src/evm.rs`:
- Around line 5-17: The import block at the top (the revm::{ ... } use) is
misformatted; run rustfmt/cargo fmt to auto-fix formatting and adjust the nested
module lists so each item is on its own line or grouped per rustfmt style (e.g.,
split revm::{ Context, Inspector, context::{ CfgEnv, ContextError, Evm,
FrameStack }, handler::{ EthFrame, EvmTr, FrameInitOrResult, FrameTr,
ItemOrResult, instructions::EthInstructions }, inspector::InspectorEvmTr,
interpreter::{ Host, Instruction, InstructionContext, gas::BLOCKHASH,
interpreter::EthInterpreter, interpreter_types::StackTr },
primitives::BLOCK_HASH_HISTORY } ). Ensure the top-level use revm::... statement
is formatted consistently with rustfmt so CI passes.
---
Outside diff comments:
In `@crates/node/src/validator.rs`:
- Around line 118-125: The code currently constructs BasicEngineValidator via
BasicEngineValidator::new(...) but drops any effect of
geth_rpc_url/with_geth_rpc_url; either remove the configuration/logging or
rewire it into the new validator path. Fix by updating the
builder/BasicEngineValidator::new signature or its construction here to accept
the geth_rpc_url (or a boolean flag) and thread that into the validator
implementation so the state-root cross-validation code is actually invoked, or
alternatively remove the with_geth_rpc_url() call and its "cross-validation
enabled" log so it is not misleading; refer to BasicEngineValidator::new,
with_geth_rpc_url, geth_rpc_url and invalid_block_hook when locating and
updating the code.
In `@crates/revm/src/handler.rs`:
- Around line 254-264: CI flagged a formatting error in the
validate_initial_tx_gas function; run rustfmt (cargo fmt) and reformat the block
around fn validate_initial_tx_gas, ensuring proper indentation and spacing for
the function signature and the lines referencing evm.ctx_ref().tx(),
evm.ctx_ref().cfg().spec().into(), and the InitialAndFloorGas type so the file
passes cargo fmt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57ac878c-06e6-4222-8f36-564d38185dca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/consensus/src/validation.rscrates/evm/src/block/mod.rscrates/evm/src/config.rscrates/node/src/validator.rscrates/payload/builder/src/builder.rscrates/payload/builder/src/error.rscrates/revm/src/evm.rscrates/revm/src/handler.rscrates/revm/src/tx.rs
💤 Files with no reviewable changes (2)
- crates/evm/src/block/mod.rs
- crates/revm/src/tx.rs
Replace local path patch (/tmp/vergen-9.0.6-local) with git rev from rustyhorde/vergen. Also patch vergen-lib to ensure vergen and vergen-git2 share the same vergen-lib instance, avoiding trait incompatibility.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
122-133: Restore the missingdefault-features = falseflags on the downgraded alloy crates.Cargo enables a dependency's default features unless you explicitly turn them off, and the target
rethmanifest keepsdefault-features = falseonalloy-evm,alloy-genesis,alloy-rpc-types-eth,alloy-serde,alloy-signer, andalloy-signer-local. Omitting those flags here widens the resolved feature set relative to the upstream stack this downgrade is trying to follow.♻️ Suggested manifest change
-alloy-evm = "0.25.1" -alloy-genesis = "1.4.3" +alloy-evm = { version = "0.25.1", default-features = false } +alloy-genesis = { version = "1.4.3", default-features = false } @@ -alloy-rpc-types-eth = { version = "1.4.3" } -alloy-serde = "1.4.3" -alloy-signer = "1.4.3" -alloy-signer-local = "1.4.3" +alloy-rpc-types-eth = { version = "1.4.3", default-features = false } +alloy-serde = { version = "1.4.3", default-features = false } +alloy-signer = { version = "1.4.3", default-features = false } +alloy-signer-local = { version = "1.4.3", default-features = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 122 - 133, Restore the missing default-features = false flags for the downgraded alloy crates in Cargo.toml: add default-features = false to the alloy-evm, alloy-genesis, alloy-rpc-types-eth, alloy-serde, alloy-signer, and alloy-signer-local dependency entries so their feature set matches the target reth manifest; update each dependency line (e.g., the alloy-evm, alloy-genesis, alloy-rpc-types-eth, alloy-serde, alloy-signer, alloy-signer-local entries) to include default-features = false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Around line 122-133: Restore the missing default-features = false flags for
the downgraded alloy crates in Cargo.toml: add default-features = false to the
alloy-evm, alloy-genesis, alloy-rpc-types-eth, alloy-serde, alloy-signer, and
alloy-signer-local dependency entries so their feature set matches the target
reth manifest; update each dependency line (e.g., the alloy-evm, alloy-genesis,
alloy-rpc-types-eth, alloy-serde, alloy-signer, alloy-signer-local entries) to
include default-features = false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abf5977b-e4c1-4561-ada1-07e83431fe82
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/node/src/validator.rs (1)
162-165:⚠️ Potential issue | 🟠 MajorRemove or explicitly deprecate this dead
geth_rpc_urlpath.After the state-root validation hooks were removed, this method only stores
geth_rpc_urlin a dead field; nothing in this file reads it afterward. That makes the setting a silent no-op, and%urlat Line 164 can leak embedded RPC credentials/API keys into logs. Please either remove this surface now, or keep it as a deprecated no-op that warns without logging the raw URL.Minimal mitigation
- pub fn with_geth_rpc_url(mut self, url: String) -> Self { - tracing::info!(target: "morph::validator", %url, "Enabled state root cross-validation via geth diskRoot RPC"); - self.geth_rpc_url = Some(url); + pub fn with_geth_rpc_url(self, url: String) -> Self { + let _ = url; + tracing::warn!( + target: "morph::validator", + "geth_rpc_url is ignored because state-root cross-validation is not active" + ); self }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/validator.rs` around lines 162 - 165, The with_geth_rpc_url method is a dead surface that logs the raw URL; change it into an explicit deprecated no-op by adding a #[deprecated(...)] attribute to with_geth_rpc_url, remove the `%url` interpolation and the sensitive logging, and replace tracing::info!(...) with a non-sensitive tracing::warn!(target: "morph::validator", "with_geth_rpc_url is deprecated and no-op; supplied RPC URL will not be used"); you may keep or clear the geth_rpc_url field (field name: geth_rpc_url) but ensure no raw URL is ever logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/node/src/validator.rs`:
- Around line 162-165: The with_geth_rpc_url method is a dead surface that logs
the raw URL; change it into an explicit deprecated no-op by adding a
#[deprecated(...)] attribute to with_geth_rpc_url, remove the `%url`
interpolation and the sensitive logging, and replace tracing::info!(...) with a
non-sensitive tracing::warn!(target: "morph::validator", "with_geth_rpc_url is
deprecated and no-op; supplied RPC URL will not be used"); you may keep or clear
the geth_rpc_url field (field name: geth_rpc_url) but ensure no raw URL is ever
logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1450a6b-673c-41ff-be89-7715dc0b367d
📒 Files selected for processing (4)
crates/evm/src/block/mod.rscrates/node/src/validator.rscrates/revm/src/evm.rscrates/revm/src/handler.rs
💤 Files with no reviewable changes (1)
- crates/evm/src/block/mod.rs
The geth RPC cross-validation feature (morph_diskRoot) relied on StateRootValidator which was removed in the reth v1.10.0 downgrade. The geth_rpc_url field, CLI arg, and builder method were left behind as dead code that logged misleading "cross-validation enabled" messages. Remove the entire geth_rpc_url plumbing: - MorphEngineValidatorBuilder: remove field and with_geth_rpc_url() - MorphEngineValidator: remove field and with_geth_rpc_url() - MorphAddOns: remove with_geth_rpc_url() constructor - MorphArgs: remove --morph.geth-rpc-url CLI arg
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/node/src/validator.rs (1)
41-43: Drop the unusedchain_specfromMorphEngineValidator.
chain_specis no longer read anywhere in this file, and the new#[allow(dead_code)]just hides that. Removing the field and makingMorphEngineValidator::new()parameterless would also letbuild()stop fetchingctx.node.provider().chain_spec()purely to discard it.Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/validator.rs` around lines 41 - 43, The MorphEngineValidator struct still holds an unused chain_spec field and MorphEngineValidator::new(ctx.node.provider().chain_spec()) forces build() to fetch and discard chain_spec from AddOnsContext; remove the chain_spec field from MorphEngineValidator, change MorphEngineValidator::new() to take no parameters, and update the build method to simply call MorphEngineValidator::new() (removing the ctx.node.provider().chain_spec() call); also update any other constructors or call sites that pass chain_spec (e.g., the other occurrence noted around lines 119-120) to the new parameterless constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/node/src/validator.rs`:
- Around line 41-43: The MorphEngineValidator struct still holds an unused
chain_spec field and MorphEngineValidator::new(ctx.node.provider().chain_spec())
forces build() to fetch and discard chain_spec from AddOnsContext; remove the
chain_spec field from MorphEngineValidator, change MorphEngineValidator::new()
to take no parameters, and update the build method to simply call
MorphEngineValidator::new() (removing the ctx.node.provider().chain_spec()
call); also update any other constructors or call sites that pass chain_spec
(e.g., the other occurrence noted around lines 119-120) to the new parameterless
constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6fe4da7-c9a5-47f5-bd15-99da0e46f2f6
📒 Files selected for processing (4)
crates/node/src/add_ons.rscrates/node/src/args.rscrates/node/src/node.rscrates/node/src/validator.rs
💤 Files with no reviewable changes (1)
- crates/node/src/args.rs
Replace paradigmxyz/reth@b25f32a with morph-l2/reth@1dd7227 (branch panos/v1.10.0-state-root-hook).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
morph-l2/reth@8057627(v1.10.2-based, revm-state 9.0.0) toparadigmxyz/reth@b25f32a(v1.10.0, revm-state 8.1.1) to avoid the EIP-2200 gas regression caused by revm-state 9.0.0'smark_warm_with_transaction_idresettingoriginal_valueon cold→warm slot transitionssload_morphcustom SLOAD,fee_slot_original_valuesfield, and fee slot saving/restore logicExecutionOutcomeinstead ofBlockExecutionOutput, removeStateRootValidator/ChangesetCache/ReceiptRootBloom(morph-l2/reth-only additions)Dependency changes
Test plan
cargo build --workspacecompiles clean (0 errors, 0 warnings)cargo test --workspace— all 249 tests passrevm-stateversion inCargo.lockconfirmed as 8.1.1Summary by CodeRabbit