Remove starknet mutex when fetching config#871
Conversation
WalkthroughThe PR centralizes configuration access by adding Arc-backed config and server_config fields to Api and refactoring server components to read configs through Api instead of holding their own copies or locking Starknet. JsonRpcHandler drops its public config fields and updates its constructor to accept only Api. Call sites in main.rs and various API endpoint modules are updated accordingly. A method in StarknetState is made non-mutating by changing &mut self to &self. A minor path simplification replaces std::sync::Arc with Arc in a calldata construction. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/starknet-devnet-server/src/api/write_endpoints.rs (1)
126-135: Avoid holding dumpable_events lock during file I/OClone events, drop the lock, then write. Prevents blocking other RPCs while dumping.
Apply:
- let dumpable_events = self.api.dumpable_events.lock().await; + let dumpable_events = { self.api.dumpable_events.lock().await.clone() }; if !path.is_empty() { - dump_events(&dumpable_events, &path) + dump_events(&dumpable_events, &path) .map_err(|err| ApiError::DumpError { msg: err.to_string() })?; return Ok(DevnetResponse::DevnetDump(None).into()); } - Ok(DevnetResponse::DevnetDump(Some(dumpable_events.clone())).into()) + Ok(DevnetResponse::DevnetDump(Some(dumpable_events)).into())crates/starknet-devnet/src/main.rs (2)
294-296: Shadowing mutability is unnecessarylet starknet_config = starknet_config; is a no-op aside from removing mutability. Safe to keep; can be dropped for brevity.
404-407: Don’t hold dumpable_events lock during file I/O; signal handlers separation intactClone events, drop the lock, then dump. Keeps shutdown responsive. Separation from the block-interval SIGINT handler remains as intended. Based on learnings
- if let (Some(DumpOn::Exit), Some(dump_path)) = (api.config.dump_on, &api.config.dump_path) { - let events = api.dumpable_events.lock().await; - dump_events(&events, dump_path).expect("Failed to dump."); + if let (Some(DumpOn::Exit), Some(dump_path)) = (api.config.dump_on, &api.config.dump_path) { + let events = { api.dumpable_events.lock().await.clone() }; + dump_events(&events, dump_path).expect("Failed to dump."); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/starknet-devnet-core/src/starknet/mod.rs(1 hunks)crates/starknet-devnet-core/src/state/mod.rs(1 hunks)crates/starknet-devnet-server/src/api/endpoints.rs(1 hunks)crates/starknet-devnet-server/src/api/endpoints_ws.rs(1 hunks)crates/starknet-devnet-server/src/api/json_rpc_handler.rs(6 hunks)crates/starknet-devnet-server/src/api/mod.rs(2 hunks)crates/starknet-devnet-server/src/api/write_endpoints.rs(3 hunks)crates/starknet-devnet/src/main.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T12:57:27.220Z
Learnt from: FabijanC
PR: 0xSpaceShard/starknet-devnet#849
File: crates/starknet-devnet/src/main.rs:345-346
Timestamp: 2025-09-18T12:57:27.220Z
Learning: In crates/starknet-devnet/src/main.rs, there are intentionally two separate signal handlers: one in `create_block_interval()` for terminating the block generation subprocess, and one in `shutdown_signal()` for handling dump-on-exit functionality. These serve distinct purposes and should not be consolidated.
Applied to files:
crates/starknet-devnet/src/main.rs
🔇 Additional comments (15)
crates/starknet-devnet-core/src/starknet/mod.rs (1)
721-721: LGTM!Good simplification using the existing Arc import instead of the fully qualified path. This improves code consistency with other Arc usages in the file.
crates/starknet-devnet-core/src/state/mod.rs (1)
196-201: LGTM! Good refactor to remove unnecessary mutability.Changing from
&mut selfto&selfis correct since the method only reads state viais_contract_deployed. This enables callers to use shared references instead of exclusive locks, which aligns with the PR's goal of reducing mutex contention.crates/starknet-devnet-server/src/api/endpoints.rs (1)
703-704: LGTM! This is the key optimization.Replacing
self.api.starknet.lock().await.configwith(*self.api.config)eliminates mutex contention for config reads. This directly supports the PR objective of reducing mutex locks when fetching configuration.crates/starknet-devnet-server/src/api/endpoints_ws.rs (1)
147-149: LGTM! Consistent with the config access optimization.Accessing
fork_configdirectly viaself.api.configinstead of through a lockedStarknetinstance follows the same optimization pattern as in endpoints.rs, reducing mutex contention.crates/starknet-devnet-server/src/api/mod.rs (1)
34-50: Approve config centralization in Api.All
Api::newcall sites include theserver_configparameter.crates/starknet-devnet-server/src/api/write_endpoints.rs (2)
114-125: Good: config read without locking StarknetAccessing dump_on/dump_path via self.api.config removes unnecessary Starknet lock. Matches PR goal.
139-145: LGTM: load uses Api.configUsing self.api.config.dump_on aligns with centralizing config and avoids Starknet lock.
crates/starknet-devnet-server/src/api/json_rpc_handler.rs (6)
7-7: Import OKDumpOn import matches new usage.
27-27: Import OKrpc_core import supports Id::Null usage.
71-76: Correct: use config.uses_pre_confirmed_block()Pre-confirmed block gating now reads from Api.config; no Starknet lock needed.
151-161: Constructor simplified to Api-onlyJsonRpcHandler::new(api) and origin_caller init via api.config look good; fewer moving parts.
537-544: Method restriction via Api.server_configSwitch to self.api.server_config.restricted_methods is consistent with centralization.
581-601: Dump logic centralized and correct
- Block mode writes immediately to
dump_path; Request/Exit modes buffer todumpable_events.DumpOnderivesCopy, so matching by value is safe.crates/starknet-devnet/src/main.rs (2)
317-319: LGTM: Api/JsonRpcHandler wiringApi::new(starknet, server_config) and JsonRpcHandler::new(api.clone()) align with new API surface.
333-333: serve_http_json_rpc signature verified
serve_http_json_rpc takes &ServerConfig; passing &api.server_config avoids unnecessary clones.
Usage related changes
Development related changes
Closes #869.
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
Refactor
Performance
Chores
Reliability