Skip to content

fix(reqresp): map malformed request payloads to invalid request#9645

Open
Osraka wants to merge 2 commits into
ChainSafe:unstablefrom
Osraka:osraka/invalid-reqresp-deserialize
Open

fix(reqresp): map malformed request payloads to invalid request#9645
Osraka wants to merge 2 commits into
ChainSafe:unstablefrom
Osraka:osraka/invalid-reqresp-deserialize

Conversation

@Osraka

@Osraka Osraka commented Jul 11, 2026

Copy link
Copy Markdown

Summary

Maps malformed req/resp request-body deserialization failures to INVALID_REQUEST instead of letting them fall through as generic handler errors.

This keeps the existing request decoding path unchanged, but wraps the app-level SSZ deserialization done by beacon-node req/resp handlers. That prevents malformed request bytes, such as malformed DataColumnSidecarsByRoot payloads, from being reported as server errors.

Refs #9398.

Notes

  • This does not change DataColumn custody/filtering behavior.
  • This does not change the current behavior for structurally valid but empty columns requests.
  • The PR is limited to malformed SSZ request-body handling.

Testing

  • pnpm exec vitest run --project unit packages/beacon-node/test/unit/network/reqresp/handlers/index.test.ts packages/beacon-node/test/unit/network/peers/discover.test.ts packages/reqresp/test/unit/encoders/requestDecode.test.ts packages/reqresp/test/unit/response/index.test.ts
  • pnpm --filter @lodestar/beacon-node lint
  • pnpm --filter @lodestar/beacon-node check-types
  • pnpm --filter @lodestar/beacon-node build

I also tried pnpm exec vitest run --project e2e packages/beacon-node/test/e2e/network/reqresp.test.ts, but this local checkout was installed with --ignore-scripts and runs Node v22 while the repo expects Node v24. The test suite failed during collection on a missing classic-level native binding before reaching the req/resp tests.

@Osraka Osraka requested a review from a team as a code owner July 11, 2026 07:18

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a deserializeRequestBody helper function to safely wrap request deserialization in a try-catch block, throwing a ResponseError with RespStatus.INVALID_REQUEST upon failure. All request handlers have been updated to use this helper, and a corresponding unit test has been added. Feedback suggests optimizing performance by passing the deserializable type and data buffer directly to avoid closure allocations in hot paths, as well as improving error safety by checking if the caught exception is an instance of Error before accessing its message.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +32 to +38
function deserializeRequestBody<T>(deserialize: () => T): T {
try {
return deserialize();
} catch (e) {
throw new ResponseError(RespStatus.INVALID_REQUEST, (e as Error).message);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Performance & Safety Improvement

  1. Avoid Closure Allocations in Hot Paths:
    Currently, deserializeRequestBody takes a closure () => T. Since this helper is executed on every incoming p2p request, creating a new closure () => ... for every request introduces unnecessary memory allocations and garbage collection overhead in high-throughput network paths.

    We can avoid this by passing the deserializable type and the data buffer directly to the helper.

  2. Safe Error Handling:
    Casting e as Error and accessing .message is unsafe if a non-Error object (or null/undefined) is thrown. Using e instanceof Error ? e.message : String(e) is much safer and guarantees a valid string message.

With this change, the usages can be simplified from:

const body = deserializeRequestBody(() => ssz.phase0.BeaconBlocksByRangeRequest.deserialize(req.data));

to:

const body = deserializeRequestBody(ssz.phase0.BeaconBlocksByRangeRequest, req.data);
interface Deserializable<T> {
  deserialize(data: Uint8Array): T;
}

function deserializeRequestBody<T>(type: Deserializable<T>, data: Uint8Array): T {
  try {
    return type.deserialize(data);
  } catch (e) {
    throw new ResponseError(RespStatus.INVALID_REQUEST, e instanceof Error ? e.message : String(e));
  }
}

@Osraka

Osraka commented Jul 11, 2026

Copy link
Copy Markdown
Author

Addressed Gemini’s review in the follow-up commit. deserializeRequestBody now takes the SSZ type plus request bytes directly to avoid per-request closure allocation, and the catch block handles non-Error throws safely with e instanceof Error ? e.message : String(e). Re-ran the focused unit test, beacon-node lint, and beacon-node typecheck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants