Update to reth 11.3 - #925

Draft

SozinM wants to merge 1 commit into

develop from

msozin/reth11.3

Draft

Update to reth 11.3 #925
SozinM wants to merge 1 commit into
develop from
msozin/reth11.3

Conversation

@SozinM

@SozinM SozinM commented Jun 10, 2026

Copy link

Copy Markdown

Contributor

📝 Summary

Purely vibecoded, do not merge (yet)

💡 Motivation and Context

✅ I have completed the following steps:

Run make lint
Run make test
Added tests (if applicable)

@SozinM


 11.3 vibes

9ad069f

@claude

claude Bot commented Jun 10, 2026 •

edited

Loading

Copy link

Copy Markdown

Contributor

Claude finished @SozinM's task in 3m 1s —— View job

Review: Update to reth 11.3

Reviewed the non-mechanical changes (the bulk of the diff is trait-bound churn — TrieReader → ChangeSetReader/StorageChangeSetReader/StorageSettingsCache, error-type threading, and the new RocksDB/runtime plumbing). Focused on the parts that carry real behavior change. Findings posted inline; summary below.

Main concerns

unsafe impl Sync for SyncStateProvider (provider/mod.rs:46) — This is the load-bearing change of the PR. The doc justification is thorough, but the invariant ("only factory-produced providers are wrapped") is enforced only by convention: new/new_arc are pub and take any StateProviderBox. Suggest tightening visibility to pub(crate) and adding a compile-time/regression assertion so a future reth bump that adds non-thread-safe DB-tx internals fails loudly rather than becoming silent UB. (inline)
reth_task_runtime memoizes a tokio Handle (utils/mod.rs:38) — The OnceLock freezes the first call's decision process-wide. Across multiple #[tokio::test]s the cached handle can outlive its runtime → panic on task spawn; and a first call made off-runtime pins a standalone runtime forever. Fine in production (single long-lived runtime) but fragile for tests touching root-hash/ProviderFactory. (inline)
with_spec_and_mainnet_gas_params (utils/mod.rs:166) — Applied unconditionally in default_cfg_env, which serves all supported chains. Please confirm mainnet gas params are correct for any non-mainnet target, or derive them from chain_spec. (inline)

Minor

IpcStateProvider::storage_by_hashed_key → unimplemented!() (ipc_state_provider.rs:295) — consistent with the file's existing pattern for unsupported trie methods, so acceptable, but it's a panic, not an error. If the IPC path can ever reach reth's hashed-key lookup it will abort the process; returning a ProviderError would be safer. The Arc-wrapping of the per-block caches so cheap clones share state is correct.
Error flattening in order_commit.rs (unwrap_evm_db_error, EvmDatabaseError<ProviderError> → ProviderError) is handled cleanly; the Bal variant being unexpected and preserved as ProviderError::other is reasonable given rbuilder sets no BAL.
Duplicated RocksDB-path derivation in base_config.rs:595 and config.rs:784 (identical match block). Worth extracting to a shared helper.
provider_factory_reopen.rs: opening RocksDB read-only to coexist with a running reth node is the right call and well-commented.

Notes

This is the author's own "do not merge yet / vibecoded" PR, so I did not attempt to build or run make lint/make test (the checkboxes in the description are unchecked). I'd recommend verifying the test suite specifically around concern chore(deps): bump alloy-serde from a1c9f8a to 3cbefdd #2 before merge.

_{· branch msozin/reth11.3}

claude[bot]

claude Bot reviewed

Jun 10, 2026

View reviewed changes

crates/rbuilder/src/utils/mod.rs

Comment on lines +38 to +48

pub fn reth_task_runtime() -> reth::tasks::Runtime {

use reth::tasks::{Runtime, RuntimeBuilder, RuntimeConfig};

static RUNTIME: std::sync::OnceLock<Runtime> = std::sync::OnceLock::new();

RUNTIME

.get_or_init(|| {

let config = match tokio::runtime::Handle::try_current() {

Ok(handle) => RuntimeConfig::with_existing_handle(handle),

Err(_) => RuntimeConfig::default(),

};

RuntimeBuilder::new(config)

.build()

@claude claude Bot Jun 10, 2026

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.

The OnceLock freezes the runtime decision on the first call for the whole process lifetime. Two consequences worth checking:

Captured handle outlives its runtime. If the first caller is inside a tokio runtime, with_existing_handle caches that Handle. In the test suite each #[tokio::test] spins up and tears down its own runtime, so the first test to call this freezes its handle and every later test reuses a handle whose runtime has been dropped — reth task spawns on it will panic. Production is fine (one long-lived runtime), but tests that exercise root-hash / ProviderFactory paths across multiple #[tokio::test]s can flake.
First-call-outside-tokio wins. If the first call happens off a tokio thread, you get a standalone runtime that is then used even for callers that are on the main runtime.

Consider not memoizing the handle path (only memoize a standalone runtime), or documenting that this must first be called from the long-lived runtime.

claude[bot]

claude Bot reviewed

Jun 10, 2026

View reviewed changes

crates/rbuilder/src/utils/mod.rs

CfgEnv::new()

.with_chain_id(chain_spec.chain().id())

.with_spec(spec)

.with_spec_and_mainnet_gas_params(spec)

@claude claude Bot Jun 10, 2026

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.

with_spec_and_mainnet_gas_params applies mainnet gas parameters unconditionally, but default_cfg_env is used for all chains rbuilder builds for (the chain_spec is threaded in just above for the chain id). If any supported chain has different gas params, this would silently use mainnet values. Please confirm this is correct for non-mainnet targets, or derive the params from chain_spec.

claude[bot]

claude Bot reviewed

Jun 10, 2026

View reviewed changes

crates/rbuilder/src/provider/mod.rs

Comment on lines 21 to +46

use revm::database::BundleState;

pub mod ipc_state_provider;

pub mod reth_prov;

pub mod state_provider_factory_from_provider_factory;

/// A state provider shareable across builder threads.

pub type StateProviderArc = Arc<dyn StateProvider + Send + Sync>;

/// A [`StateProvider`] wrapper asserting that the wrapped provider is thread-safe.

///

/// reth 1.10 removed the `Sync` supertrait from [`StateProvider`]/`DbTx`

/// (paradigmxyz/reth#20516) so DB transactions can later grow single-threaded cursor

/// caches. As of reth v1.11.3 no such caches exist: MDBX transactions are still

/// internally synchronized (`TransactionPtr` keeps its lock and its `unsafe impl Sync`),

/// and the in-memory overlay / static-file / IPC providers hold only thread-safe data,

/// so sharing one provider across builder threads is as sound as it was before the

/// bound change.

///

/// SAFETY invariant: every [`StateProviderBox`] wrapped here must come from one of the

/// factories in this module (reth DB, reth node, IPC). Re-audit on every reth bump: if

/// DB transactions gain non-thread-safe internals this assertion becomes unsound.

pub struct SyncStateProvider(StateProviderBox);

// SAFETY: see type-level docs.

unsafe impl Sync for SyncStateProvider {}

@claude claude Bot Jun 10, 2026

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.

The unsafe impl Sync soundness rests entirely on "every wrapped provider comes from one of the factories in this module," but new/new_arc are pub and accept any StateProviderBox — nothing in the type system enforces the invariant, and call sites like sim.rs wrap whatever StateProviderFactory::history_by_block_hash returns (including the IPC provider). The doc is good; consider additionally (a) restricting visibility to pub(crate) so external/unaudited providers can't be wrapped, and (b) adding a regression note/test asserting the concrete provider types in use are actually Send + Sync on their own, so a future reth bump that adds non-thread-safe internals fails loudly rather than silently becoming UB.

Labels

None yet

1 participant

@SozinM

Navigation Menu

Uh oh!

Update to reth 11.3 - #925

Update to reth 11.3 #925
SozinM wants to merge 1 commit into
develop from
msozin/reth11.3

Conversation

@SozinM SozinM commented Jun 10, 2026

📝 Summary

💡 Motivation and Context

✅ I have completed the following steps:

Uh oh!

claude Bot commented Jun 10, 2026 •

edited

Loading

Uh oh!

Review: Update to reth 11.3

Main concerns

Minor

Notes

Uh oh!

@claude claude Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

@claude claude Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

@claude claude Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

@SozinM SozinM commented Jun 10, 2026

📝 Summary

💡 Motivation and Context

✅ I have completed the following steps:

Uh oh!

claude Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: Update to reth 11.3

Main concerns

Minor

Notes

Uh oh!

@claude claude Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

@claude claude Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

@claude claude Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude Bot commented Jun 10, 2026 •

edited

Loading