Skip to content

Commit c8779ab

Browse files
prestwichclaude
andcommitted
refactor: address PR #75 review — spec_id fix, move serve.rs, load RPC config
Fix EVM spec_id for RPC calls: all EVM execution paths (eth_call, eth_estimateGas, eth_createAccessList, debug tracing, bundle simulation) now derive the correct SpecId from chain config + block header instead of defaulting to PRAGUE. Move serve.rs from signet-node to signet-rpc: transport infrastructure (HTTP/WS/IPC) is now reth-free, using tokio::spawn and Handle::current() instead of reth's TaskExecutor. This removes axum, tower-http, interprocess, and ajj dependencies from signet-node. Load StorageRpcConfig from host RpcServerArgs: rpc_gas_cap, max_tracing_requests, and gas price oracle settings now flow through from reth's CLI args instead of always using defaults. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent c4ad4e1 commit c8779ab

14 files changed

Lines changed: 264 additions & 137 deletions

File tree

FUTURE_WORK.md

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# Future Work: signet-rpc Architectural Improvements
2+
3+
Deferred from PR #75 review. These should be follow-up PRs after the initial fixes land.
4+
5+
---
6+
7+
## Structured JSON-RPC Error Codes
8+
9+
**Problem**: Most endpoints return `Result<T, String>`, losing structured error info. All errors become `-32603 Internal error`. Clients can't programmatically distinguish error types.
10+
11+
**Approach**:
12+
- Add error code constants to `eth/error.rs`: `-32000` (server error), `-32001` (resource not found), `3` (execution reverted)
13+
- Add `Into<ErrorPayload>` impl on `EthError` that maps variants to codes
14+
- Construct `ErrorPayload` directly with custom codes (all fields are `pub` in ajj 0.6)
15+
- Progressively migrate endpoints from `Result<T, String>` to `ResponsePayload<T, EthError>`
16+
- Start with EVM execution endpoints (call/estimate/accessList), then query endpoints
17+
18+
**Files**: `crates/rpc/src/eth/error.rs`, `crates/rpc/src/eth/endpoints.rs`, `crates/rpc/src/debug/error.rs`, `crates/rpc/src/signet/error.rs`
19+
20+
---
21+
22+
## Reorg Tracking in Filter/Subscription Managers
23+
24+
**Problem**: No mechanism to notify RPC layer about reorgs. Filters/subscriptions can't emit `removed: true` logs per Ethereum spec.
25+
26+
**Approach**:
27+
- Add `ReorgNotification { common_ancestor: u64, removed_hashes: Vec<B256> }` type
28+
- Create `ChainEvent` enum wrapping `NewBlock` and `Reorg` variants
29+
- Change `ChainNotifier` broadcast channel from `NewBlockNotification` to `ChainEvent`
30+
- Node sends `ChainEvent::Reorg` during `on_host_revert()` before updating block tags
31+
- `SubscriptionTask` handles `ChainEvent::Reorg` by emitting logs with `removed: true`
32+
- `get_filter_changes` detects `latest < next_start_block` as reorg indicator, resets filter
33+
34+
**Files**: `crates/rpc/src/interest/mod.rs`, `crates/rpc/src/config/chain_notifier.rs`, `crates/rpc/src/interest/subs.rs`, `crates/rpc/src/interest/filters.rs`, `crates/rpc/src/eth/endpoints.rs`, `crates/node/src/node.rs`
35+
36+
---
37+
38+
## Best-Effort Read Consistency
39+
40+
**Problem**: RPC reads can span hot and cold storage at different points in time. Reorgs can make resolved block numbers stale between hot and cold queries.
41+
42+
**Approach**:
43+
- Document the consistency model in `StorageRpcCtx` rustdoc
44+
- Verify all cold queries use explicitly-resolved block numbers (already mostly the pattern)
45+
- Detect reorg-induced staleness in `get_filter_changes` (coupled with reorg tracking above)
46+
- Consider hash-based verification: resolve number from hot, fetch header hash, pass hash to cold for consistency check
47+
48+
**Files**: `crates/rpc/src/config/ctx.rs` (documentation), `crates/rpc/src/eth/endpoints.rs` (filter changes)

crates/node/Cargo.toml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,7 @@ signet-blobber.workspace = true
2323
signet-tx-cache.workspace = true
2424
signet-types.workspace = true
2525

26-
ajj.workspace = true
2726
alloy.workspace = true
28-
axum = "0.8.1"
29-
interprocess = { version = "2.2.2", features = ["tokio"] }
3027

3128
reth.workspace = true
3229
reth-exex.workspace = true
@@ -39,5 +36,4 @@ futures-util.workspace = true
3936
metrics.workspace = true
4037
reqwest.workspace = true
4138
tokio.workspace = true
42-
tower-http = { version = "0.6.2", features = ["cors"] }
4339
tracing.workspace = true

crates/node/src/lib.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,5 @@ pub use node::SignetNode;
2121

2222
mod rpc;
2323

24-
mod serve;
25-
2624
mod status;
2725
pub use status::NodeStatus;

crates/node/src/node.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::{NodeStatus, metrics, serve::RpcServerGuard};
1+
use crate::{NodeStatus, metrics};
22
use alloy::consensus::BlockHeader;
33
use eyre::Context;
44
use futures_util::StreamExt;
@@ -14,7 +14,7 @@ use signet_block_processor::{AliasOracleFactory, SignetBlockProcessorV1};
1414
use signet_evm::EthereumHardfork;
1515
use signet_extract::Extractor;
1616
use signet_node_config::SignetNodeConfig;
17-
use signet_rpc::{ChainNotifier, NewBlockNotification};
17+
use signet_rpc::{ChainNotifier, NewBlockNotification, RpcServerGuard};
1818
use signet_storage::{HistoryRead, HotKv, HotKvRead, UnifiedStorage};
1919
use signet_types::{PairedHeights, constants::SignetSystemConstants};
2020
use std::{fmt, sync::Arc};

crates/node/src/rpc.rs

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
1-
use crate::{
2-
SignetNode,
3-
serve::{RpcServerGuard, ServeConfig},
4-
};
5-
use reth::primitives::EthPrimitives;
1+
use crate::SignetNode;
2+
use reth::{args::RpcServerArgs, primitives::EthPrimitives};
63
use reth_node_api::{FullNodeComponents, NodeTypes};
74
use signet_block_processor::AliasOracleFactory;
8-
use signet_rpc::{StorageRpcConfig, StorageRpcCtx};
5+
use signet_rpc::{RpcServerGuard, ServeConfig, StorageRpcConfig, StorageRpcCtx};
96
use signet_storage::HotKv;
107
use signet_tx_cache::TxCache;
11-
use std::sync::Arc;
8+
use std::{net::SocketAddr, sync::Arc};
129
use tracing::info;
1310

1411
impl<Host, H, AliasOracle> SignetNode<Host, H, AliasOracle>
@@ -28,18 +25,48 @@ where
2825
}
2926

3027
async fn launch_rpc(&self) -> eyre::Result<RpcServerGuard> {
31-
let tasks = self.host.task_executor();
3228
let tx_cache =
3329
self.config.forward_url().map(|url| TxCache::new_with_client(url, self.client.clone()));
30+
31+
let args = self.config.merge_rpc_configs(&self.host)?;
32+
let rpc_config = rpc_config_from_args(&args);
33+
3434
let rpc_ctx = StorageRpcCtx::new(
3535
Arc::clone(&self.storage),
3636
self.constants.clone(),
37+
self.config.genesis().config.clone(),
3738
self.chain.clone(),
3839
tx_cache,
39-
StorageRpcConfig::default(),
40+
rpc_config,
4041
);
4142
let router = signet_rpc::router::<H>().with_state(rpc_ctx);
42-
let serve_config: ServeConfig = self.config.merge_rpc_configs(&self.host)?.into();
43-
serve_config.serve(tasks, router).await
43+
44+
let serve_config = serve_config_from_args(args);
45+
serve_config.serve(router).await.map_err(Into::into)
4446
}
4547
}
48+
49+
/// Extract [`StorageRpcConfig`] values from reth's host RPC settings.
50+
///
51+
/// Fields with no reth equivalent retain their defaults.
52+
fn rpc_config_from_args(args: &RpcServerArgs) -> StorageRpcConfig {
53+
let gpo = &args.gas_price_oracle;
54+
StorageRpcConfig::builder()
55+
.rpc_gas_cap(args.rpc_gas_cap)
56+
.max_tracing_requests(args.rpc_max_tracing_requests)
57+
.gas_oracle_block_count(gpo.blocks as u64)
58+
.gas_oracle_percentile(gpo.percentile as f64)
59+
.ignore_price(Some(gpo.ignore_price as u128))
60+
.max_price(Some(gpo.max_price as u128))
61+
.build()
62+
}
63+
64+
/// Convert reth [`RpcServerArgs`] into a reth-free [`ServeConfig`].
65+
fn serve_config_from_args(args: RpcServerArgs) -> ServeConfig {
66+
let http =
67+
if args.http { vec![SocketAddr::from((args.http_addr, args.http_port))] } else { vec![] };
68+
let ws = if args.ws { vec![SocketAddr::from((args.ws_addr, args.ws_port))] } else { vec![] };
69+
let ipc = if !args.ipcdisable { Some(args.ipcpath) } else { None };
70+
71+
ServeConfig { http, http_cors: args.http_corsdomain, ws, ws_cors: args.ws_allowed_origins, ipc }
72+
}

crates/rpc/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ serde.workspace = true
3030
dashmap = "6.1.0"
3131
revm-inspectors.workspace = true
3232
itertools.workspace = true
33+
axum = "0.8.1"
34+
tower-http = { version = "0.6.2", features = ["cors"] }
35+
interprocess = { version = "2.2.2", features = ["tokio"] }
3336

3437
[dev-dependencies]
3538
tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }

crates/rpc/src/config/ctx.rs

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,12 @@ use crate::{
88
eth::EthError,
99
interest::{FilterManager, SubscriptionManager},
1010
};
11-
use alloy::eips::{BlockId, BlockNumberOrTag};
11+
use alloy::{
12+
eips::{BlockId, BlockNumberOrTag},
13+
genesis::ChainConfig,
14+
};
1215
use signet_cold::ColdStorageReadHandle;
16+
use signet_evm::EthereumHardfork;
1317
use signet_hot::{
1418
HotKv,
1519
db::HotDbRead,
@@ -20,7 +24,10 @@ use signet_tx_cache::TxCache;
2024
use signet_types::constants::SignetSystemConstants;
2125
use std::sync::Arc;
2226
use tokio::sync::Semaphore;
23-
use trevm::revm::database::{DBErrorMarker, StateBuilder};
27+
use trevm::revm::{
28+
database::{DBErrorMarker, StateBuilder},
29+
primitives::hardfork::SpecId,
30+
};
2431

2532
/// Resolved block context for EVM execution.
2633
///
@@ -38,6 +45,8 @@ pub(crate) struct EvmBlockContext<Db> {
3845
/// [`State`]: trevm::revm::database::State
3946
/// [`DatabaseCommit`]: trevm::revm::database::DatabaseCommit
4047
pub db: trevm::revm::database::State<Db>,
48+
/// The EVM hardfork spec active at this block.
49+
pub spec_id: SpecId,
4150
}
4251

4352
/// RPC context backed by [`UnifiedStorage`].
@@ -64,6 +73,7 @@ impl<H: HotKv> Clone for StorageRpcCtx<H> {
6473
struct StorageRpcCtxInner<H: HotKv> {
6574
storage: Arc<UnifiedStorage<H>>,
6675
constants: SignetSystemConstants,
76+
chain_config: ChainConfig,
6777
chain: ChainNotifier,
6878
tx_cache: Option<TxCache>,
6979
config: StorageRpcConfig,
@@ -79,9 +89,13 @@ impl<H: HotKv> StorageRpcCtx<H> {
7989
/// The [`ChainNotifier`] provides block tag tracking and a broadcast
8090
/// sender for new block notifications. The subscription manager
8191
/// subscribes to this channel to push updates to WebSocket clients.
92+
///
93+
/// The `chain_config` is the rollup genesis chain configuration, used
94+
/// to determine the active EVM hardfork (spec ID) for each block.
8295
pub fn new(
8396
storage: Arc<UnifiedStorage<H>>,
8497
constants: SignetSystemConstants,
98+
chain_config: ChainConfig,
8599
chain: ChainNotifier,
86100
tx_cache: Option<TxCache>,
87101
config: StorageRpcConfig,
@@ -94,6 +108,7 @@ impl<H: HotKv> StorageRpcCtx<H> {
94108
inner: Arc::new(StorageRpcCtxInner {
95109
storage,
96110
constants,
111+
chain_config,
97112
chain,
98113
tx_cache,
99114
config,
@@ -130,6 +145,16 @@ impl<H: HotKv> StorageRpcCtx<H> {
130145
&self.inner.constants
131146
}
132147

148+
/// Access the chain configuration (rollup genesis forks).
149+
pub(crate) fn chain_config(&self) -> &ChainConfig {
150+
&self.inner.chain_config
151+
}
152+
153+
/// Determine the EVM [`SpecId`] for a given block header.
154+
pub(crate) fn spec_id_for_header(&self, header: &alloy::consensus::Header) -> SpecId {
155+
EthereumHardfork::active_hardforks_at_header(self.chain_config(), header).spec_id()
156+
}
157+
133158
/// Get the chain ID.
134159
pub fn chain_id(&self) -> u64 {
135160
self.inner.constants.ru_chain_id()
@@ -289,6 +314,8 @@ impl<H: HotKv> StorageRpcCtx<H> {
289314
header.gas_limit = self.config().rpc_gas_cap;
290315
}
291316

292-
Ok(EvmBlockContext { header, db })
317+
let spec_id = self.spec_id_for_header(&header);
318+
319+
Ok(EvmBlockContext { header, db, spec_id })
293320
}
294321
}

crates/rpc/src/config/rpc_config.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,24 @@ impl StorageRpcConfigBuilder {
210210
self
211211
}
212212

213+
/// Set the default gas price returned when no recent transactions exist.
214+
pub const fn default_gas_price(mut self, price: Option<u128>) -> Self {
215+
self.inner.default_gas_price = price;
216+
self
217+
}
218+
219+
/// Set the minimum effective tip to include in the oracle sample.
220+
pub const fn ignore_price(mut self, price: Option<u128>) -> Self {
221+
self.inner.ignore_price = price;
222+
self
223+
}
224+
225+
/// Set the maximum gas price the oracle will ever suggest.
226+
pub const fn max_price(mut self, price: Option<u128>) -> Self {
227+
self.inner.max_price = price;
228+
self
229+
}
230+
213231
/// Set the default bundle simulation timeout in milliseconds.
214232
pub const fn default_bundle_timeout_ms(mut self, ms: u64) -> Self {
215233
self.inner.default_bundle_timeout_ms = ms;

crates/rpc/src/debug/endpoints.rs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,10 @@ where
8080
DebugError::from(e)
8181
}));
8282

83-
let mut trevm = signet_evm::signet_evm(db, ctx.constants().clone())
84-
.fill_cfg(&CfgFiller(ctx.chain_id()))
85-
.fill_block(&header);
83+
let spec_id = ctx.spec_id_for_header(&header);
84+
let mut evm = signet_evm::signet_evm(db, ctx.constants().clone());
85+
evm.set_spec_id(spec_id);
86+
let mut trevm = evm.fill_cfg(&CfgFiller(ctx.chain_id())).fill_block(&header);
8687

8788
let mut txns = txs.iter().enumerate().peekable();
8889
for (idx, tx) in txns
@@ -165,9 +166,10 @@ where
165166
DebugError::from(e)
166167
}));
167168

168-
let mut trevm = signet_evm::signet_evm(db, ctx.constants().clone())
169-
.fill_cfg(&CfgFiller(ctx.chain_id()))
170-
.fill_block(&header);
169+
let spec_id = ctx.spec_id_for_header(&header);
170+
let mut evm = signet_evm::signet_evm(db, ctx.constants().clone());
171+
evm.set_spec_id(spec_id);
172+
let mut trevm = evm.fill_cfg(&CfgFiller(ctx.chain_id())).fill_block(&header);
171173

172174
// Replay all transactions up to (but not including) the target
173175
let mut txns = txs.iter().enumerate().peekable();

crates/rpc/src/eth/endpoints.rs

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -730,11 +730,11 @@ where
730730
let span = trace_span!("run_call", block_id = %id);
731731

732732
let task = async move {
733-
let EvmBlockContext { header, db } = response_tri!(ctx.resolve_evm_block(id));
733+
let EvmBlockContext { header, db, spec_id } = response_tri!(ctx.resolve_evm_block(id));
734734

735-
let trevm = signet_evm::signet_evm(db, ctx.constants().clone())
736-
.fill_cfg(&CfgFiller(ctx.chain_id()))
737-
.fill_block(&header);
735+
let mut trevm = signet_evm::signet_evm(db, ctx.constants().clone());
736+
trevm.set_spec_id(spec_id);
737+
let trevm = trevm.fill_cfg(&CfgFiller(ctx.chain_id())).fill_block(&header);
738738

739739
let trevm = response_tri!(trevm.maybe_apply_state_overrides(state_overrides.as_ref()))
740740
.maybe_apply_block_overrides(block_overrides.as_deref())
@@ -813,11 +813,11 @@ where
813813
let span = trace_span!("eth_estimateGas", block_id = %id);
814814

815815
let task = async move {
816-
let EvmBlockContext { header, db } = response_tri!(ctx.resolve_evm_block(id));
816+
let EvmBlockContext { header, db, spec_id } = response_tri!(ctx.resolve_evm_block(id));
817817

818-
let trevm = signet_evm::signet_evm(db, ctx.constants().clone())
819-
.fill_cfg(&CfgFiller(ctx.chain_id()))
820-
.fill_block(&header);
818+
let mut trevm = signet_evm::signet_evm(db, ctx.constants().clone());
819+
trevm.set_spec_id(spec_id);
820+
let trevm = trevm.fill_cfg(&CfgFiller(ctx.chain_id())).fill_block(&header);
821821

822822
let trevm = response_tri!(trevm.maybe_apply_state_overrides(state_overrides.as_ref()))
823823
.maybe_apply_block_overrides(block_overrides.as_deref())
@@ -864,11 +864,11 @@ where
864864
let span = trace_span!("eth_createAccessList", block_id = %id);
865865

866866
let task = async move {
867-
let EvmBlockContext { header, db } = response_tri!(ctx.resolve_evm_block(id));
867+
let EvmBlockContext { header, db, spec_id } = response_tri!(ctx.resolve_evm_block(id));
868868

869-
let trevm = signet_evm::signet_evm(db, ctx.constants().clone())
870-
.fill_cfg(&CfgFiller(ctx.chain_id()))
871-
.fill_block(&header);
869+
let mut trevm = signet_evm::signet_evm(db, ctx.constants().clone());
870+
trevm.set_spec_id(spec_id);
871+
let trevm = trevm.fill_cfg(&CfgFiller(ctx.chain_id())).fill_block(&header);
872872

873873
let trevm = response_tri!(trevm.maybe_apply_state_overrides(state_overrides.as_ref()))
874874
.maybe_apply_block_overrides(block_overrides.as_deref())

0 commit comments

Comments
 (0)