Skip to content

cl/sentinel: reject invalid ping and status requests - #23579

Open
domiwei wants to merge 3 commits into
mainfrom
kewei/fix-empty-ping-status-requests
Open

cl/sentinel: reject invalid ping and status requests#23579
domiwei wants to merge 3 commits into
mainfrom
kewei/fix-empty-ping-status-requests

Conversation

@domiwei

@domiwei domiwei commented Aug 26, 2026

Copy link
Copy Markdown
Member

Summary

  • decode Ping/1, Status/1, and Status/2 request bodies before returning success
  • return InvalidRequest for empty, truncated, oversized, malformed, and observably trailing requests
  • enforce fixed SSZ sizes and bounded Snappy input; Status/2 uses the 92-byte Fulu schema
  • update request lifecycle tests to close the write side and clean up handler contexts

Closes #23166.

Testing

  • go test ./cl/sentinel/communication/ssz_snappy ./cl/sentinel/handlers ./cl/sentinel -count=1
  • GOLANGCI_LINT_CACHE=/private/tmp/issue23166-golangci-cache make lint

@awskii awskii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Four files here also change in #23577ssz_snappy/encoding.go, handlers/heartbeats.go, handlers/rate_limiter_integration_test.go, sentinel_requests_test.go — and its encoding.go edits land in the same functions this PR rewrites. Whichever merges second rebases onto the other.

Comment thread cl/sentinel/communication/ssz_snappy/encoding.go Outdated
Comment thread cl/sentinel/communication/ssz_snappy/encoding.go Outdated
Comment thread cl/sentinel/communication/ssz_snappy/encoding.go Outdated
Comment thread cl/sentinel/communication/ssz_snappy/encoding_test.go Outdated

Copilot AI 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.

Pull request overview

Rejects malformed Ping and Status requests in Caplin’s SSZ-Snappy handlers.

Changes:

  • Adds fixed-size request decoding and invalid-request responses.
  • Bounds compressed input and checks trailing data.
  • Expands handler and lifecycle tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cl/sentinel/sentinel_requests_test.go Closes request writes before reading responses.
cl/sentinel/handlers/rate_limiter_integration_test.go Sends valid Ping requests in rate-limit tests.
cl/sentinel/handlers/heartbeats.go Validates Ping and Status request bodies.
cl/sentinel/handlers/heartbeats_test.go Tests valid and malformed requests.
cl/sentinel/communication/ssz_snappy/encoding.go Adds exact-size, bounded decoding.
cl/sentinel/communication/ssz_snappy/encoding_test.go Tests compressed limits and trailing frames.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cl/sentinel/communication/ssz_snappy/encoding.go Outdated
@yperbasis yperbasis added the Caplin Caplin: Consensus Layer, Beacon API label Aug 26, 2026
@domiwei
domiwei force-pushed the kewei/fix-empty-ping-status-requests branch from 3da4ce7 to 54e9d5f Compare August 26, 2026 14:59
@yperbasis
yperbasis requested a balanced review from Copilot August 26, 2026 15:26

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two protocol-boundary issues remain in the exact SSZ-Snappy decoder.

Comment thread cl/sentinel/communication/ssz_snappy/encoding.go Outdated
Comment thread cl/sentinel/communication/ssz_snappy/encoding.go Outdated

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@domiwei
domiwei requested a review from yperbasis August 27, 2026 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Caplin Caplin: Consensus Layer, Beacon API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[spec-violation]Caplin accepts empty Ping and Status request bodies

4 participants