Client: make JSON-RPC payload limits configurable for eth and engine servers #3993
Conversation
There was a problem hiding this comment.
Looks good! Could you point all these constants (which are hardcoded strings in some places) to only one point where this constant is actually defined?
(I know for other options we also hardcode the same string at multiple places, we should also fix that at some point)
packages/client/bin/repl.ts
Outdated
| @@ -42,6 +42,8 @@ const setupClient = async ( | |||
| jwtSecret: '', | |||
| rpcEngineAuth: false, | |||
| rpcCors: '', | |||
| rpcEthMaxPayload: args.rpcEthMaxPayload ?? '5mb', | |||
| rpcEngineMaxPayload: args.rpcEngineMaxPayload ?? '15mb', | |||
There was a problem hiding this comment.
I'm fine with these limits, but could you motivate this choice?
There was a problem hiding this comment.
Could you also point these to RPC_ENGINE_MAXPAYLOAD_DEFAULT (also for eth)?
There was a problem hiding this comment.
I'm fine with these limits, but could you motivate this choice?
For the eth RPC size limit, I based it on the average block sizes observed on Etherscan. The highest daily average recorded was ~0.25 MB, so I multiplied it by 20 in anticipation of more intensive use of block space.
For the engine RPC size limit, according to EIP-4844 the total maximum blob data per block is ~0.75 MB, and I also multiplied it by 20.
This provides a safety margin while still avoiding excessive resource usage. Maybe that's too much, what do you think? 🤔
There was a problem hiding this comment.
So the eth RPC engine is intended to be used by "third parties" and it is OK to expose this port to others. For instance providers like Infura/Quicknode also expose those. Also note that this is the limit of incoming messages, so actual requests. Not the reports. So for eth one could request eth_getBlock and this request itself would be very small (just a block hash or number). But the response will be bigger (block itself).
For engine, this is a trusted RPC endpoint and one can only reach it with authentication. This is also the endpoint from which the consensus layer (CL) will send new payloads (new block-like requests). So we can up that limit.
I'm fine with these limits but just wanted to check your motivation, thanks 😄 👍
There was a problem hiding this comment.
Thanks for the clarification!
I’ll make sure to better explain my motivations in future PRs 🫡
packages/client/src/config.ts
Outdated
| @@ -356,7 +366,7 @@ export class Config { | |||
| public readonly events: EventEmitter<EventParams> | |||
|
|
|||
| public static readonly CHAIN_DEFAULT = Mainnet | |||
| public static readonly SYNCMODE_DEFAULT = SyncMode.Full | |||
| public static readonly SYNCMODE_DEFAULT = SyncMode.None | |||
There was a problem hiding this comment.
Sorry changed it while testing, fixing it now
packages/client/bin/utils.ts
Outdated
| .option('rpcEngineMaxPayload', { | ||
| describe: 'Define max JSON payload size for engine RPC requests', | ||
| string: true, | ||
| default: '15mb', |
There was a problem hiding this comment.
Could you point this to RPC_ENGINE_MAXPAYLOAD_DEFAULT? Also for rpcEthMaxPayload 😄 👍
| @@ -74,6 +75,7 @@ describe('[Util/RPC]', () => { | |||
| const httpServer = createRPCServerListener({ | |||
| server, | |||
| withEngineMiddleware: { jwtSecret: new Uint8Array(32) }, | |||
| maxPayload: '15mb', | |||
| }) | |||
| const wsServer = createWsRPCServerListener({ | |||
| server, | |||
There was a problem hiding this comment.
Could you add a test here to verify that for both engine/eth the config is handled correctly? You can likely just set it to some low limit and then try to push more data over the RPC than the limit to verify that it fails.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This PR closes Issue #3981 by introducing two new configuration options:
rpcEthMaxPayload&rpcEngineMaxPayloadThese options allow setting different maximum JSON-RPC payload sizes for the
eth/debugandengineRPC server.What news ?
Added
rpcEthMaxPayloadandrpcEngineMaxPayloadfields in the Config class.Extended the CLI options to allow users to configure these payload limits.
Updated the RPC server initialization to use different limits depending on the server type.
Adjusted REPL and utils accordingly.