Fix DataColumnSidecarsByRoot request msg schema #9972
Conversation
|
@zilm13 I'd like your input on this one. I just noticed that although we didn't have the hard limit on the schema, we did have an explicit RPC validation for the length on I am wondering if you have any insights on why we did it this way, instead of having it as part of the request schema as we usually do. |
zilm13
left a comment
zilm13
left a comment
There was a problem hiding this comment.
LGTM
Looks like we missed it on changes, thank you for finding!
| } | ||
|
|
||
| @Override | ||
| public Optional<RpcException> validateRequest( |
There was a problem hiding this comment.
Removing this because this will be done as part of decoding the request on Eth2IncomingRequestHandler (when it decodes the ByteBuf into the ssz container, it will enforce the sizes)
| final String protocolId, final DataColumnSidecarsByRootRequestMessage request) { | ||
| final int maxRequestIdentifiers = specConfigFulu.getMaxRequestBlocksDeneb(); | ||
| if (request.size() > maxRequestIdentifiers) { | ||
| requestCounter.labels("count_too_big").inc(); |
There was a problem hiding this comment.
We do lose this counter on count_too_big. Do we even care @zilm13 ?
There was a problem hiding this comment.
Metrics is not a big loss, but we don't penalize peer in any manner, don't count request happened, just disconnect and that's all. In case of real DoS it could be disadvantage
...ku/spec/datastructures/networking/libp2p/rpc/DataColumnSidecarsByRootRequestMessageTest.java
Outdated
Show resolved
Hide resolved
3 participants
PR Description
Adjust the length of the list of DataColumnsByRootIdentifier to match the definition in the spec.
Spec reference: https://github.com/ethereum/consensus-specs/pull/4284/files
I believe this was missed after ethereum/consensus-specs#4284 was merged.
Also, we are removing the current validation on DataColumnSidecarsByRootMessageHandler.validateRequest(), given this will be done as part of decoding the request on
Eth2IncomingRequestHandler(when it decodes the ByteBuf into the ssz container it will enforce the sizes).Fixed Issue(s)
N/A
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Switch request list max size to
getMaxRequestBlocksDeneband remove explicit handler validation, relying on SSZ decoding; update tests accordingly.DataColumnSidecarsByRootRequestMessageSchema: set list max length fromSpecConfigFulu.getMaxRequestBlocksDeneb.DataColumnSidecarsByRootMessageHandler: removevalidateRequestsize check and relatedSpecConfigFuluusage; minor javadoc/metric string tidy.schema.getMaxLength()togetMaxRequestBlocksDeneb.Written by Cursor Bugbot for commit beab547. This will update automatically on new commits. Configure here.