-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: add support for testing_ rpc namespace and testing_buildBlockV1 #20094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@mattsse Should we support testing blob/sidecar transactions here ? |
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks pretty good already
left some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these args we can exclude, we can make it so that --http.api also accepts testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's name this V1
| pub struct TestingBuildBlockRequest { | |
| pub struct TestingBuildBlockRequestV1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can exclude this from here, and instead instantiate and install here
reth/crates/ethereum/node/src/node.rs
Lines 311 to 314 in a62b46a
| container | |
| .modules | |
| .merge_if_module_configured(RethRpcModule::Eth, eth_config.into_rpc())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| container.modules.merge_if_module_configured( | |
| RethRpcModule::Flashbots, | |
| validation_api.into_rpc(), | |
| )?; |
| async fn build_block( | ||
| &self, | ||
| request: TestingBuildBlockRequest, | ||
| ) -> RpcResult<ExecutionPayloadEnvelopeV4> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This aims to reoslve this 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
cc @mattsse