Msg Validation Rules for Boole Fork #2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates message validation rules for the Boole fork, introducing support for a new RoleAggregatorCommittee role and refactoring validation logic to handle committee-related roles more consistently.
Key Changes:
- Introduced
RoleAggregatorCommitteerole and updated validation rules to distinguish betweenRoleCommitteeandRoleAggregatorCommittee - Updated message size constants to accommodate larger messages (MaxMsgSize increased from ~4.9MB to ~9.1MB)
- Refactored beacon duty validation to focus only on proposer duties, removing sync committee aggregation duty checks
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| p2p/MessageValidation/Rules.md | Updated validation rules, constants, and logic to support the new AggregatorCommittee role; added helper function isCommitteeRole() to check committee-related roles; updated partial signature validation with role-specific limits |
| p2p/MessageValidation/ReceivedMessagesEstimation.md | Added deprecation warning noting the document is outdated due to Alan and Boole fork protocol changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > [!WARNING] | ||
| > This document is deprecated due to protocol changes as the Alan and Boole forks. | ||
There was a problem hiding this comment.
how hard it is to do a simulator that auto updates from spec you reckon 😅 ?
There was a problem hiding this comment.
you have simulators in ssv/research?
Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | -------------------- | ------------------------- | -------------- |--------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Signers in committee | ErrSignerNotInCommittee | Reject | Signers must belong to validator's or CommitteeID's committee. | | ||
| | Different Domain | ErrWrongDomain | Ignore | MsgID.Domain is different than self domain. | | ||
| | Invalid Role | ErrInvalidRole | Reject | MsgID.Role is wrong (not `RoleCommittee`, `RoleProposer`, `RoleValidatorRegistration`, `RoleVoluntaryExit`, or `RoleAggregatorCommittee`). | |
There was a problem hiding this comment.
Can we define that Alan roles are rejected during Boole and vice versa? (ssvlabs/ssv#2503 (comment))
There was a problem hiding this comment.
It's kind of defined implicitly I think ?
- the updated rule set contains
RoleAggregatorCommitteebut notRoleAggregator/RoleSyncCommitteeContribution - and although there is no explicit list of roles prior to this PR, if it were there - it would contain
RoleAggregator/RoleSyncCommitteeContributionbut notRoleAggregatorCommittee
But it would be nice for us devs to see "what changed" rather than trying to decipher the diff :) Maybe we could get that for the next fork ...
There was a problem hiding this comment.
we have a taks to migrate to spec repo...
We need to think about how to make it as easy as possible.
Maybe you guys canuse this thread to write recommendations about what you like/don't like in the current structure
There was a problem hiding this comment.
Maybe you guys canuse this thread to write recommendations about what you like/don't like in the current structure
Not strictly about the structure of these files, but I would think some sort of writeup in English about every rule added/changed/deleted (before vs after the fork) would be useful to have whenever we are doing these changes
GalRogozinski
dismissed stale reviews from iurii-ssv, dknopik, and nkryuchkov
via
49f11a9
| Type PartialSigMsgType -> Message count rules | ||
| Slot phase0.Slot -> Must belong to allowed spread, Satisfies a maximum number of duties per epoch for role, Must not be "old" | ||
| Messages []*PartialSignatureMessage -> Valid number of signatures (3 cases: committee duty, sync committee contribution, others) | ||
| Messages []*PartialSignatureMessage -> Valid number of signatures (3 cases: committee duty, aggregator committee duty, others) |
There was a problem hiding this comment.
I find what is written in parentehsis confusing?
GalRogozinski
left a comment
GalRogozinski
left a comment
There was a problem hiding this comment.
The list of changes from the SIP are hard to see here so I will be adding it to the description
Overview
This PR updates the message validation rules for the Boole fork.
SSVMessage.MsgIDmust include a CommitteeID encoded with a 16-byte 0x00 prefix to matchValidatorPublicKeylength. If the CommitteeID doesn't exist in the current network, it should ignore the message.ValidatorIndexinSignedPartialSignatureMessage.Message.Messagesis incorrect, considering theValidatorPublicKeyor theCommitteeIDin theMessageID, it should ignore the message.ValidatorIndexappears more than five times in aPartialSignatureMessages(both forPostConsensusPartialSigandAggregatorCommitteePartialSig), the message is rejected.Five comes from the fact that, at most, a validator may have one aggregator duty and four sync committee contribution duties (one for each subnet).
/ssv/<ethereum_network_name>/boole/<subnet>