feat: add support for testing_ rpc namespace and testing_buildBlockV1#20094
feat: add support for testing_ rpc namespace and testing_buildBlockV1#20094mattsse merged 30 commits intoparadigmxyz:mainfrom
Conversation
|
@mattsse Should we support testing blob/sidecar transactions here ? |
mattsse
left a comment
There was a problem hiding this comment.
this looks pretty good already
left some suggestions
There was a problem hiding this comment.
these args we can exclude, we can make it so that --http.api also accepts testing
There was a problem hiding this comment.
Yeah we can exclude that and make it more simple
crates/rpc/rpc-api/src/testing.rs
Outdated
| /// Request payload for `testing_buildBlockV1`. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct TestingBuildBlockRequest { |
There was a problem hiding this comment.
let's name this V1
| pub struct TestingBuildBlockRequest { | |
| pub struct TestingBuildBlockRequestV1 { |
There was a problem hiding this comment.
these changes we can all undo because we can instantiate this on demand specifically for eth
similar to flashbots api:
reth/crates/ethereum/node/src/node.rs
Lines 307 to 310 in a62b46a
crates/rpc/rpc/src/testing.rs
Outdated
| async fn build_block( | ||
| &self, | ||
| request: TestingBuildBlockRequest, | ||
| ) -> RpcResult<ExecutionPayloadEnvelopeV4> { |
There was a problem hiding this comment.
this entire fn logic we need to spawn as blocking, ideally also add a semaphor for this so we can limit how many requests we allow at the same time
There was a problem hiding this comment.
In terms of the semaphor , which number do you think is appropriate by default ?
I just set the concurrent number to at least 1 and can be overriten if value provided
There was a problem hiding this comment.
I think let's go with just 1 by default for now
crates/ethereum/node/src/node.rs
Outdated
| let testing_requested = { | ||
| let cfg = container.modules.module_config(); | ||
| cfg.contains_http(&RethRpcModule::Other("testing".to_string())) || | ||
| cfg.contains_ws(&RethRpcModule::Other("testing".to_string())) || | ||
| cfg.contains_ipc(&RethRpcModule::Other("testing".to_string())) |
There was a problem hiding this comment.
I believe this is just merge_if_configured
There was a problem hiding this comment.
yeah should use merge_if_module_configured instead
crates/ethereum/node/src/node.rs
Outdated
| if testing_requested { | ||
| let evm_config: EthEvmConfig<ChainSpec> = | ||
| EthEvmConfig::new(testing_chain.clone()); | ||
| let builder = EthTestingBlockBuilder::new( | ||
| testing_provider, | ||
| evm_config, | ||
| EthereumBuilderConfig::new(), | ||
| ); | ||
| let testing_api = TestingApi::new(builder, testing_max_concurrent).into_rpc(); |
There was a problem hiding this comment.
imo we can always instantiate this and just call merge_if_configured
| /// Max concurrent `testing_buildBlockV1` requests (hidden, defaults to 1). | ||
| #[arg(long = "testing.max-concurrent", default_value_t = 1usize, hide = true)] | ||
| pub testing_max_concurrent: usize, |
There was a problem hiding this comment.
we can make some default assumptions here and dont really need this as a setting imo
There was a problem hiding this comment.
Ahh alright let's go by default
crates/rpc/rpc/src/testing.rs
Outdated
| pub trait TestingBlockBuilder: Send + Sync + std::fmt::Debug + 'static { | ||
| /// Build a block according to the testing API request. | ||
| async fn build_block( | ||
| &self, | ||
| request: TestingBuildBlockRequestV1, | ||
| ) -> RpcResult<ExecutionPayloadEnvelopeV4>; | ||
| } |
There was a problem hiding this comment.
I think this api could potentially be useful for other networks as well, so we can make an effort here to make this generic over the types, like N: NodePrimitives perhaps, or at least the actual TestingApi
crates/ethereum/node/src/node.rs
Outdated
| ChainSpec = ChainSpec, | ||
| Primitives = EthPrimitives, | ||
| Payload: EngineTypes<ExecutionData = ExecutionData>, | ||
| >, | ||
| Evm: ConfigureEvm<NextBlockEnvCtx = NextBlockEnvAttributes>, | ||
| >, | ||
| N::Provider: BlockReaderIdExt< | ||
| Block = EthBlock, | ||
| Header = <EthBlock as BlockTrait>::Header, | ||
| Transaction = TransactionSigned, | ||
| Receipt = Receipt, | ||
| > + StateProviderFactory | ||
| + ChainSpecProvider<ChainSpec = ChainSpec> | ||
| + Send | ||
| + Sync | ||
| + 'static, |
There was a problem hiding this comment.
I'd like to avoid these changes if possible, I think the entire block building logic is already sufficiently generic enough and we can make a few changes to the TestingApi to route the types accordingly
crates/ethereum/node/src/node.rs
Outdated
| let evm_config: EthEvmConfig<ChainSpec> = | ||
| EthEvmConfig::new(testing_chain.clone()); |
There was a problem hiding this comment.
all of these should be obtainable from the ctx type I believe
There was a problem hiding this comment.
Yeah have reduced the unnessary part
crates/rpc/rpc/src/testing.rs
Outdated
| let handle = Handle::current(); | ||
| let join_handle = | ||
| tokio::task::spawn_blocking(move || handle.block_on(builder.build_block(request))); |
There was a problem hiding this comment.
we can give this type a Box<dyn TaskExecutor> which can also obtain from the ctx where this type is created
crates/rpc/rpc/src/testing.rs
Outdated
| async fn build_block( | ||
| &self, | ||
| request: TestingBuildBlockRequest, | ||
| ) -> RpcResult<ExecutionPayloadEnvelopeV4> { |
There was a problem hiding this comment.
I think let's go with just 1 by default for now
Yeah before i was thinking to add it after getting feedbacks from you , now will add it |
6f1b5d2 to
7262e4a
Compare
…number * upstream: (203 commits) feat(node-core): make rpc server args customizable (paradigmxyz#20312) feat: add `account_history_in_rocksdb` field to `StorageSettings` (paradigmxyz#20282) feat(engine): Add BAL stub methods to ExecutionPayload and BlockOrPayload (paradigmxyz#20311) docs: fix misleading links (paradigmxyz#20300) ci: add more sccache (paradigmxyz#20316) feat: bump alloy-evm (paradigmxyz#20314) feat: allow larger ws frames on client side (paradigmxyz#20307) docs: add architecture diagrams to ExEx documentation (paradigmxyz#20193) feat: add semaphore for blocking IO requests (paradigmxyz#20289) ci: scale down depot runners (paradigmxyz#20295) perf: fetch header directly (paradigmxyz#20294) docs(exex): fix DebugApi comment (paradigmxyz#20296) feat: add support for testing_ rpc namespace and testing_buildBlockV1 (paradigmxyz#20094) chore: update engine_getBlobs metric (paradigmxyz#20290) chore(optimism): move predeploy constant to op-alloy (paradigmxyz#20181) docs: fix stages order and add missing EraStage (paradigmxyz#20283) docs: improve map_add_ons method documentation (paradigmxyz#20248) feat: add `transaction_hash_numbers_in_rocksdb` field to `StorageSettings` (paradigmxyz#20209) docs: clarify network mode, tx gossip and NAT (paradigmxyz#20247) feat: add support for debug_getBadBlock (paradigmxyz#20177) ...
This closes issue #20069 , summerized as below :
Added TestingApi and TestingBuildBlockRequest (camelCase, payloadAttributes+transactions+extraData) in rpc-ap
Implemented server-side handler and builder: TestingBlockBuilder trait and EthTestingBlockBuilder execute only the provided raw signed transactions (order preserved), reject blob tx without sidecars, apply payloadAttributes/extraData, and return ExecutionPayloadEnvelopeV4 without touching the canonical chain.
Wired conditional registration: hidden CLI flags --http.testing / --ws.testing add testing to module selection, and in Ethereum node we construct EthTestingBlockBuilder and merge TestingApi only when the testing module is explicitly requested. Security note added to API docs
Expose a hidden --testing.max-concurrent flag (default 1), and wire testing RPC instantiation to use a semaphore + spawn_blocking so heavy block builds don’t block the async runtime and have bounded concurrency
References :
testing_buildBlockV1.md ethereum/execution-apis#710
cc @mattsse