p2p: add message validation spec tests#618
Conversation
nkryuchkov
requested review from
GalRogozinski and
MatheusFranco99
as code owners
Greptile SummaryThis PR adds a new The implementation is well-structured and consistent with other spectest packages in the repo. The Confidence Score: 5/5Safe to merge; the validation hardening is correct and the spectest suite is well-structured with comprehensive coverage. All remaining findings are P2 style suggestions. The previously flagged RSA signature verification gap is now resolved, the single-signer constraint is enforced, and the new spectest infrastructure matches established repo patterns. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pubsub.Message] --> B[DecodePubsubMsg]
B -->|decode error| REJ1[ValidationReject]
B --> C{SSVMessage.GetType}
C -->|SSVConsensusMsgType| D[validateConsensusMsg]
C -->|SSVPartialSignatureMsgType| E[validatePartialSigMsg]
C -->|default| REJ2[ValidationReject]
D --> D1[qbft.NewProcessingMessage]
D1 -->|error| REJ3[ValidationReject]
D1 --> D2[contr.BaseMsgValidation]
D2 -->|error| REJ3
D2 --> D3{isDecided?}
D3 -->|yes| D4[qbft.ValidateDecided]
D4 -->|error| REJ3
D4 --> ACC[ValidationAccept]
D3 -->|no| D5{Height > contr.Height?}
D5 -->|yes| D6[validateFutureMsg]
D6 -->|error| REJ3
D6 --> ACC
D5 -->|no| D7{StoredInstances.FindInstance?}
D7 -->|found| D8[inst.BaseMsgValidation]
D8 -->|error| REJ3
D8 --> ACC
D7 -->|not found| REJ3
E --> E1[signedMsg.Validate]
E1 -->|error| REJ4[ValidationReject]
E1 --> E2{len OperatorIDs == 1?}
E2 -->|no| REJ4
E2 -->|yes| E3[types.Verify RSA sig]
E3 -->|error| REJ4
E3 --> E4[partialSigMsg.Decode + ValidateForSigner]
E4 -->|error| REJ4
E4 --> E5{PostConsensus?}
E5 -->|yes| E6[ValidatePostConsensusMsg]
E5 -->|no| E7[ValidatePreConsensusMsg]
E6 -->|error| REJ4
E7 -->|error| REJ4
E6 --> ACC
E7 --> ACC
Reviews (4): Last reviewed commit: "code review comments [2]" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai please review this again |
|
@greptileai This does not reproduce on the current branch.
I ran |
|
You're right, I was mistaken. The implementation is correct. The aggregate The |
|
@greptileai review it again |
|
@greptileai review it again |
|
we need to decide whether we should go with #491 or this |
momosh-ssv
left a comment
momosh-ssv
left a comment
There was a problem hiding this comment.
Two small nits on the partial-sig path — the hardening itself looks good.
| return errors.New("partial signature msg allows 1 signer") | ||
| } | ||
| if err := types.Verify(signedMsg, runner.GetBaseRunner().QBFTController.CommitteeMember.Committee); err != nil { | ||
| return types.WrapError(types.MessageSignatureInvalidErrorCode, fmt.Errorf("msg signature invalid: %w", err)) |
There was a problem hiding this comment.
Any reason we're wrapping with MessageSignatureInvalidErrorCode here? The only caller (MsgValidation) checks err != nil and returns ValidationReject, so the code never surfaces — same story in validateFutureMsg:128.
Either propagate it upward for logs/metrics or drop the wrap to match the other plain-error returns in this function.
| return err | ||
| } | ||
| if len(signedMsg.OperatorIDs) != 1 { | ||
| return errors.New("partial signature msg allows 1 signer") |
There was a problem hiding this comment.
Might be worth aligning this error string with validateFutureMsg:123 ("allows 1 signer"). An operator reading gossip-reject logs will hit both messages for effectively the same rule, and the slight difference in wording makes them look like distinct conditions.
5 participants
Summary
This PR adds a proper P2P spectest suite for the implemented P2P message-validation surface and brings
p2p/spectestin line with the repo’s other spectest packages.It also hardens
p2p/validation/msg_validation.goso gossip rejects malformed or structurally invalid signed messages earlier at the P2P boundary.What changed
P2P validation hardening
SignedSSVMessage.Validate()New P2P spectest suite
Added a new data-driven
p2p/specteststructure with:all_tests.gorun_test.goTestJsontestdoc/generate/tests/msgvalidation/generaltests/msgvalidation/consensustests/msgvalidation/preconsensustests/msgvalidation/postconsensusCoverage added
SignedSSVMessagerejects:FullDataGenerated artifacts
p2p/spectest/generate/tests/Verification
go generate ./p2p/spectest/generatego test ./p2p/...Task
F-ssv-spec-034