Update to reth 11.3#925
Conversation
|
Claude finished @SozinM's task in 3m 1s —— View job Review: Update to reth 11.3Reviewed the non-mechanical changes (the bulk of the diff is trait-bound churn — Main concerns
Minor
Notes
· branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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() |
There was a problem hiding this comment.
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_handlecaches thatHandle. 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 /ProviderFactorypaths 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.
| CfgEnv::new() | ||
| .with_chain_id(chain_spec.chain().id()) | ||
| .with_spec(spec) | ||
| .with_spec_and_mainnet_gas_params(spec) |
There was a problem hiding this comment.
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.
| 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 {} |
There was a problem hiding this comment.
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.
1 participant
📝 Summary
Purely vibecoded, do not merge (yet)
💡 Motivation and Context
✅ I have completed the following steps:
make lintmake test