Fork aware max values in rpc#6847
Conversation
| (None, None) | ||
| }; | ||
|
|
||
| // TODO(pawan): this would break if a batch contains multiple epochs |
There was a problem hiding this comment.
Even if a batch contains multiple epochs (which it doesn't), this is a very niche case that would only break during fork boundaries.
jxs
left a comment
jxs
left a comment
There was a problem hiding this comment.
LGTM Pawan, left some suggestions :)
|
|
||
| match &req { | ||
| RequestType::BlocksByRange(request) => { | ||
| let max_allowed = spec.max_request_blocks(current_fork) as u64; |
There was a problem hiding this comment.
Have we ever thought about making the limit dependent on the protocol version or the active fork at the time of the request.slot range? It seems like we could inadvertently reject requests from clients that use different limits for pre-Deneb blocks.
There was a problem hiding this comment.
Hi Michael, can you elaborate on what you mean by request.slot range?
If it's pre Deneb max_request_blocks will return the value for pre Deneb right?
There was a problem hiding this comment.
We are always using the current fork according to wall clock time, so if the request is for pre-Deneb data but the current fork is post-Deneb, we will use the Deneb value.
There was a problem hiding this comment.
I think we don't do it that way to keep things consistent between byroot and byrange requests (since we cannot know the slot when we get root requests).
I asked other CL teams how they handle it and it seems like prysm and lodestar do by slot, but nimbus does it by current fork.
In practice, I don't think this matters that much because all clients request 1/2 epochs per by_range request which doesn't hit the limits.
michaelsproul
added
waiting-on-author
Co-authored-by: Michael Sproul <[email protected]>
pawanjay176
added
ready-for-review
pawanjay176
added
ready-for-merge
jxs
dismissed
michaelsproul’s stale review
suggestions have already been addressed
Since #6847, invalid `BlocksByRange`/`BlobsByRange` requests, which do not comply with the spec, are [handled in the Handler](https://github.com/sigp/lighthouse/blob/3d16d1080f5b93193404967dcb5525fa68840ea0/beacon_node/lighthouse_network/src/rpc/handler.rs#L880-L911). Any peer that sends an invalid request is penalized and disconnected. However, other kinds of invalid rpc request, which result in decoding errors, are just dropped. No penalty is applied and the connection with the peer remains. I have added handling for the `ListenUpgradeError` event to notify the application of an `RPCError:InvalidData` error and disconnect to the peer that sent the invalid rpc request. I also added tests for handling invalid rpc requests. Co-Authored-By: ackintosh <[email protected]>
Since #6847, invalid `BlocksByRange`/`BlobsByRange` requests, which do not comply with the spec, are [handled in the Handler](https://github.com/sigp/lighthouse/blob/3d16d1080f5b93193404967dcb5525fa68840ea0/beacon_node/lighthouse_network/src/rpc/handler.rs#L880-L911). Any peer that sends an invalid request is penalized and disconnected. However, other kinds of invalid rpc request, which result in decoding errors, are just dropped. No penalty is applied and the connection with the peer remains. I have added handling for the `ListenUpgradeError` event to notify the application of an `RPCError:InvalidData` error and disconnect to the peer that sent the invalid rpc request. I also added tests for handling invalid rpc requests. Co-Authored-By: ackintosh <[email protected]>
3 participants
Issue Addressed
N/A
Proposed Changes
In #6329 we changed
max_blobs_per_blockfrom a preset to a config value.We weren't using the right value based on fork in that PR. This is a follow up PR to use the fork dependent values.
In the proces, I also updated other places where we weren't using fork dependent values from the ChainSpec.
Note to reviewer: easier to go through by commit