Client: make JSON-RPC payload limits configurable for eth and engine servers#3993
Client: make JSON-RPC payload limits configurable for eth and engine servers#3993ether-wan wants to merge 8 commits into
Conversation
jochem-brouwer
left a comment
jochem-brouwer
left a comment
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)
| 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 🫡
|
|
||
| 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
| .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 😄 👍
| 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 Report✅ All 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:
|
acolytec3
left a comment
acolytec3
left a comment
There was a problem hiding this comment.
So sorry this never got finally merged. Approving now and will merge once CI passes.
Thanks again for this PR.
| public static readonly MINPEERS_DEFAULT = 1 | ||
| public static readonly MAXPEERS_DEFAULT = 25 | ||
| public static readonly DNSADDR_DEFAULT = '8.8.8.8' | ||
| public static readonly RPC_ETH_MAXPAYLOAD_DEFAULT = '11mb' |
There was a problem hiding this comment.
The previous default was 11mb, have also set this again for the eth namespace. 5mb would be low enough for almost all use cases, but not for blobs (this test failed on the CI). It should now pass 😄 👍
There was a problem hiding this comment.
Ok, it still fails, have to take a deeper look here 🤔
4 participants
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.