Review PR

packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts

@@ -28,7 +28,7 @@ import {
   ReqRespMethod,
   RequestTypedContainer,
   Version,
-  getRequestSzzTypeByMethod,
+  getRequestSszTypeByMethod,
   responseSszTypeByMethod,
 } from "./types.js";
 import {collectSequentialBlocksInRange} from "./utils/collectSequentialBlocksInRange.js";
@@ -321,7 +321,7 @@ export class ReqRespBeaconNode extends ReqResp implements IReqRespBeaconNode {
   ): AsyncIterable<ResponseIncoming> {
     // Remember prefered encoding
     const encoding = this.peersData.getEncodingPreference(peerId.toString()) ?? Encoding.SSZ_SNAPPY;
-    const requestType = getRequestSzzTypeByMethod(method);
+    const requestType = getRequestSszTypeByMethod(method);
     const requestEncoded = requestType ? requestType.serialize(request as never) : EMPTY_REQUEST;
     return super.sendRequest(peerId, method, versions, encoding, requestEncoded);
   }

packages/beacon-node/src/network/reqresp/protocols.ts

@@ -1,12 +1,6 @@
 import {ContextBytesFactory, ContextBytesType, Encoding} from "@lodestar/reqresp";
 import {ForkDigestContext} from "@lodestar/config";
-import {
-  ProtocolNoHandler,
-  ReqRespMethod,
-  Version,
-  getRequestSzzTypeByMethod,
-  responseSszTypeByMethod,
-} from "./types.js";
+import {ProtocolNoHandler, ReqRespMethod, Version, requestSszTypeByMethod, responseSszTypeByMethod} from "./types.js";
 import {rateLimitQuotas} from "./rateLimit.js";
 
 /* eslint-disable @typescript-eslint/naming-convention */
@@ -114,7 +108,7 @@ function toProtocol(protocol: ProtocolSummary) {
     encoding: Encoding.SSZ_SNAPPY,
     contextBytes: toContextBytes(protocol.contextBytesType, config),
     inboundRateLimits: rateLimitQuotas[protocol.method],
-    requestSizes: getRequestSzzTypeByMethod(protocol.method),
+    requestSizes: requestSszTypeByMethod[protocol.method],
     responseSizes: (fork) => responseSszTypeByMethod[protocol.method](fork, protocol.version),
   });
 }

packages/beacon-node/src/network/reqresp/rateLimit.ts

@@ -1,6 +1,7 @@
 import {MAX_REQUEST_BLOCKS, MAX_REQUEST_LIGHT_CLIENT_UPDATES} from "@lodestar/params";
 import {InboundRateLimitQuota} from "@lodestar/reqresp";
-import {ReqRespMethod} from "./types.js";
+import {ReqRespMethod, RequestBodyByMethod} from "./types.js";
+import {requestSszTypeByMethod} from "./types.js";
 
 export const rateLimitQuotas: Record<ReqRespMethod, InboundRateLimitQuota> = {
   [ReqRespMethod.Status]: {
@@ -23,18 +24,22 @@ export const rateLimitQuotas: Record<ReqRespMethod, InboundRateLimitQuota> = {
   [ReqRespMethod.BeaconBlocksByRange]: {
     // Rationale: https://github.com/sigp/lighthouse/blob/bf533c8e42cc73c35730e285c21df8add0195369/beacon_node/lighthouse_network/src/rpc/mod.rs#L118-L130
     byPeer: {quota: MAX_REQUEST_BLOCKS, quotaTimeMs: 10_000},
+    getRequestCount: getRequestCountFn(ReqRespMethod.BeaconBlocksByRange, (req) => req.count),
   },
   [ReqRespMethod.BeaconBlocksByRoot]: {
     // Rationale: https://github.com/sigp/lighthouse/blob/bf533c8e42cc73c35730e285c21df8add0195369/beacon_node/lighthouse_network/src/rpc/mod.rs#L118-L130
     byPeer: {quota: 128, quotaTimeMs: 10_000},
+    getRequestCount: getRequestCountFn(ReqRespMethod.BeaconBlocksByRoot, (req) => req.length),
   },
   [ReqRespMethod.BlobsSidecarsByRange]: {
     // TODO DENEB: For now same value as BeaconBlocksByRange https://github.com/sigp/lighthouse/blob/bf533c8e42cc73c35730e285c21df8add0195369/beacon_node/lighthouse_network/src/rpc/mod.rs#L118-L130
     byPeer: {quota: MAX_REQUEST_BLOCKS, quotaTimeMs: 10_000},
+    getRequestCount: getRequestCountFn(ReqRespMethod.BlobsSidecarsByRange, (req) => req.count),
   },
   [ReqRespMethod.BeaconBlockAndBlobsSidecarByRoot]: {
     // TODO DENEB: For now same value as BeaconBlocksByRoot https://github.com/sigp/lighthouse/blob/bf533c8e42cc73c35730e285c21df8add0195369/beacon_node/lighthouse_network/src/rpc/mod.rs#L118-L130
     byPeer: {quota: 128, quotaTimeMs: 10_000},
+    getRequestCount: getRequestCountFn(ReqRespMethod.BeaconBlockAndBlobsSidecarByRoot, (req) => req.length),
   },
   [ReqRespMethod.LightClientBootstrap]: {
     // As similar in the nature of `Status` protocol so we use the same rate limits.
@@ -43,6 +48,7 @@ export const rateLimitQuotas: Record<ReqRespMethod, InboundRateLimitQuota> = {
   [ReqRespMethod.LightClientUpdatesByRange]: {
     // Same rationale as for BeaconBlocksByRange
     byPeer: {quota: MAX_REQUEST_LIGHT_CLIENT_UPDATES, quotaTimeMs: 10_000},
+    getRequestCount: getRequestCountFn(ReqRespMethod.LightClientUpdatesByRange, (req) => req.count),
   },
   [ReqRespMethod.LightClientFinalityUpdate]: {
     // Finality updates should not be requested more than once per epoch.
@@ -55,3 +61,18 @@ export const rateLimitQuotas: Record<ReqRespMethod, InboundRateLimitQuota> = {
     byPeer: {quota: 2, quotaTimeMs: 12_000},
   },
 };
+
+// Helper to produce a getRequestCount function
+function getRequestCountFn<T extends ReqRespMethod>(
+  method: T,
+  fn: (req: RequestBodyByMethod[T]) => number
+): (reqData: Uint8Array) => number {
+  const type = requestSszTypeByMethod[method];
+  return (reqData: Uint8Array) => {
+    try {
+      return (type && fn(type.deserialize(reqData))) ?? 1;
+    } catch (e) {
+      return 1;
+    }
+  };
+}

packages/beacon-node/src/network/reqresp/types.ts

@@ -1,7 +1,7 @@
 import {Type} from "@chainsafe/ssz";
 import {ForkLightClient, ForkName, isForkLightClient} from "@lodestar/params";
 import {Protocol} from "@lodestar/reqresp";
-import {allForks, altair, deneb, phase0, ssz} from "@lodestar/types";
+import {Root, allForks, altair, deneb, phase0, ssz} from "@lodestar/types";
 
 export type ProtocolNoHandler = Omit<Protocol, "handler">;
 
@@ -23,20 +23,19 @@ export enum ReqRespMethod {
 }
 
 // To typesafe events to network
-type RequestBodyByMethod = {
+export type RequestBodyByMethod = {
   [ReqRespMethod.Status]: phase0.Status;
   [ReqRespMethod.Goodbye]: phase0.Goodbye;
   [ReqRespMethod.Ping]: phase0.Ping;
   [ReqRespMethod.Metadata]: null;
-  // Do not matter
-  [ReqRespMethod.BeaconBlocksByRange]: unknown;
-  [ReqRespMethod.BeaconBlocksByRoot]: unknown;
-  [ReqRespMethod.BlobsSidecarsByRange]: unknown;
-  [ReqRespMethod.BeaconBlockAndBlobsSidecarByRoot]: unknown;
-  [ReqRespMethod.LightClientBootstrap]: unknown;
-  [ReqRespMethod.LightClientUpdatesByRange]: unknown;
-  [ReqRespMethod.LightClientFinalityUpdate]: unknown;
-  [ReqRespMethod.LightClientOptimisticUpdate]: unknown;
+  [ReqRespMethod.BeaconBlocksByRange]: phase0.BeaconBlocksByRangeRequest;
+  [ReqRespMethod.BeaconBlocksByRoot]: phase0.BeaconBlocksByRootRequest;
+  [ReqRespMethod.BlobsSidecarsByRange]: deneb.BlobsSidecarsByRangeRequest;
+  [ReqRespMethod.BeaconBlockAndBlobsSidecarByRoot]: deneb.BeaconBlockAndBlobsSidecarByRootRequest;
+  [ReqRespMethod.LightClientBootstrap]: Root;
+  [ReqRespMethod.LightClientUpdatesByRange]: altair.LightClientUpdatesByRange;
+  [ReqRespMethod.LightClientFinalityUpdate]: null;
+  [ReqRespMethod.LightClientOptimisticUpdate]: null;
 };
 
 type ResponseBodyByMethod = {
@@ -56,33 +55,22 @@ type ResponseBodyByMethod = {
 };
 
 /** Request SSZ type for each method and ForkName */
-// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/explicit-function-return-type
-export function getRequestSzzTypeByMethod(method: ReqRespMethod) {
-  switch (method) {
-    case ReqRespMethod.Status:
-      return ssz.phase0.Status;
-    case ReqRespMethod.Goodbye:
-      return ssz.phase0.Goodbye;
-    case ReqRespMethod.Ping:
-      return ssz.phase0.Ping;
-    case ReqRespMethod.Metadata:
-    case ReqRespMethod.LightClientFinalityUpdate:
-    case ReqRespMethod.LightClientOptimisticUpdate:
-      return null;
-    case ReqRespMethod.BeaconBlocksByRange:
-      return ssz.phase0.BeaconBlocksByRangeRequest;
-    case ReqRespMethod.BeaconBlocksByRoot:
-      return ssz.phase0.BeaconBlocksByRootRequest;
-    case ReqRespMethod.BlobsSidecarsByRange:
-      return ssz.deneb.BlobsSidecarsByRangeRequest;
-    case ReqRespMethod.BeaconBlockAndBlobsSidecarByRoot:
-      return ssz.deneb.BeaconBlockAndBlobsSidecarByRootRequest;
-    case ReqRespMethod.LightClientBootstrap:
-      return ssz.Root;
-    case ReqRespMethod.LightClientUpdatesByRange:
-      return ssz.altair.LightClientUpdatesByRange;
-  }
-}
+export const requestSszTypeByMethod: {
+  [K in ReqRespMethod]: RequestBodyByMethod[K] extends null ? null : Type<RequestBodyByMethod[K]>;
+} = {
+  [ReqRespMethod.Status]: ssz.phase0.Status,
+  [ReqRespMethod.Goodbye]: ssz.phase0.Goodbye,
+  [ReqRespMethod.Ping]: ssz.phase0.Ping,
+  [ReqRespMethod.Metadata]: null,
+  [ReqRespMethod.BeaconBlocksByRange]: ssz.phase0.BeaconBlocksByRangeRequest,
+  [ReqRespMethod.BeaconBlocksByRoot]: ssz.phase0.BeaconBlocksByRootRequest,
+  [ReqRespMethod.BlobsSidecarsByRange]: ssz.deneb.BlobsSidecarsByRangeRequest,
+  [ReqRespMethod.BeaconBlockAndBlobsSidecarByRoot]: ssz.deneb.BeaconBlockAndBlobsSidecarByRootRequest,
+  [ReqRespMethod.LightClientBootstrap]: ssz.Root,
+  [ReqRespMethod.LightClientUpdatesByRange]: ssz.altair.LightClientUpdatesByRange,
+  [ReqRespMethod.LightClientFinalityUpdate]: null,
+  [ReqRespMethod.LightClientOptimisticUpdate]: null,
+};
 
 export type ResponseTypeGetter<T> = (fork: ForkName, version: number) => Type<T>;
 

packages/reqresp/src/response/index.ts

@@ -79,9 +79,8 @@ export async function handleRequest({
 
         logger.debug("Req  received", logCtx);
 
-        // TODO: Consider counting max count by request for byRange and byRoot
-        // const requestCount = protocol?.inboundRateLimits?.getRequestCount?.(requestBody) ?? 1;
-        const requestCount = 1;
+        // Max count by request for byRange and byRoot
+        const requestCount = protocol?.inboundRateLimits?.getRequestCount?.(requestBody) ?? 1;
 
         if (!rateLimiter.allows(peerId, protocolID, requestCount)) {
           throw new RequestError({code: RequestErrorCode.REQUEST_RATE_LIMITED});

packages/reqresp/src/types.ts

@@ -50,6 +50,11 @@ export interface InboundRateLimitQuota {
    * Will be tracked regardless of the peer
    */
   total?: RateLimiterQuota;
+  /**
+   * Some requests may be counted multiple e.g. getBlocksByRange
+   * for such implement this method else `1` will be used default
+   */
+  getRequestCount?: (req: Uint8Array) => number;
 }
 
 export type ReqRespRequest = {