Skip to content

message/validation: avoid nil SSVMessage panic in Validate defer#2707

Open
nkryuchkov wants to merge 1 commit intostagefrom
fix-message-validation-nil-ssv-panic
Open

message/validation: avoid nil SSVMessage panic in Validate defer#2707
nkryuchkov wants to merge 1 commit intostagefrom
fix-message-validation-nil-ssv-panic

Conversation

@nkryuchkov
Copy link
Contributor

I caught a panic running p2p tests locally

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x1038fbb88]

github.com/ssvlabs/ssv-spec/types.(*SSVMessage).GetID(...)
  .../ssv-spec/types/messages.go:108
github.com/ssvlabs/ssv/message/validation.(*messageValidator).Validate.func1()
  .../message/validation/validation.go:132
github.com/ssvlabs/ssv/message/validation.(*messageValidator).Validate(...)
  .../message/validation/validation.go:138
...
FAIL    github.com/ssvlabs/ssv/network/p2p    6.689s

@nkryuchkov nkryuchkov requested review from a team as code owners February 26, 2026 17:50
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 26, 2026

Greptile Summary

Fixes a nil pointer panic in message validation by extracting role determination logic into a safe helper function.

  • Extracted messageRole() helper that checks for both decodedMessage == nil and decodedMessage.SSVMessage == nil before accessing GetID()
  • Applied the helper in both the defer block (where the panic occurred) and handleValidationSuccess() for consistency
  • Added test coverage for the nil SSVMessage scenario using require.NotPanics()
  • Follows existing nil-check pattern used in logger_fields.go

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - it's a straightforward panic fix with proper nil checks and test coverage
  • Score reflects a well-implemented bug fix: the solution extracts defensive nil-checking logic into a reusable helper, adds test coverage for the panic scenario, and follows existing patterns in the codebase
  • No files require special attention

Important Files Changed

Filename Overview
message/validation/validation.go Extracted role determination into messageRole() helper with proper nil checks to prevent panic in defer block
message/validation/errors.go Updated to use new messageRole() helper for consistency
message/validation/validation_test.go Added test coverage for nil SSVMessage scenario to verify the panic fix

Last reviewed commit: e7696dc

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codecov
Copy link

codecov bot commented Feb 26, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.3%. Comparing base (20ffb01) to head (e7696dc).

Files with missing lines Patch % Lines
message/validation/validation.go 60.0% 1 Missing and 1 partial ⚠️
message/validation/errors.go 0.0% 1 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@iurii-ssv iurii-ssv left a comment

Choose a reason for hiding this comment

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

LGTM

@mbonenberger
Copy link

We’re seeing this exact panic in production after upgrading operators to v2.4.1.

The stack trace matches the one described here (SSVMessage.GetID() → message validation).

This is currently affecting operators in the Lido SimpleDVT environment, so confirming this fix would be very helpful.

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.

3 participants