Message Validation #491

MatheusFranco99 · 2024-08-22T12:46:20Z

Overview

This PR updates the message validation module.

It adds a MessageValidator structure that implements the ValidatorForTopic function, as defined by the libp2p framework.

Its main validation function starts a chain of validation checks for the pub-sub, syntax, semantics, QBFT and duty logic rules.

Documentation and message rate estimation

The PR also updates the message validation and scoring documentation in p2p/SPEC.md and p2p/SCORING.md. The go code for the message rate estimation is added (along with tests) since this estimation is referenced by the scoring documentation.

Tests

This PR also adds tests for each implemented rule, as can be seen in the following list.

PubSub

PubSub message with empty data
PubSub message with a data bigger than the allowed size
PubSub message that can't decode a SignedSSVMessage

Syntax validation

General semantics validation

Consensus semantics validation

Consensus QBFT logic validation

Consensus duty logic validation

Unexpected consensus message for ValidatorRegistration
Unexpected consensus message for VoluntaryExit
Round too high

Partial signature semantics validation

Multiple signers
With FullData
Invalid type
Type mismatch with role
No PartialSignatureMessages
Wrong partial signature size. This test can't be implemented due to ssz encoding errors
Inconsistent signers
Validator index mismatch considering the validator public key

Partial signature duty logic validation

Type count
Too many signatures
Triple ValidatorIndex

General duty logic validation

Wrong signature

Wrong signature

Valid

Happy flow

* Filter shares for slot `CommitteeRunner` based on validators that have duty for that slot. * Filter duty and create share map according to owned validators * Add test: start duty with no shares for duty's validators * Add test: happy flow for committee with fraction of duty's validators * Generate JSON tests * Apply suggestions --------- Co-authored-by: MatheusFranco99 <[email protected]>

* Update go1.20 to go1.22 * Update go.sum with mod tidy

* Update dependencies * Fix lint issue * Generate JSON tests to trigger actions * Update fastssz * Generate JSON tests and align ssz error * Revert go-eth2-client version change * Revert fastssz upgrade * Generate SSZ and JSON tests

* Solve potential file inclusion via variable * Fix file permission (0644 to 0600) * Add nosec comment for PRNG (pseudo-random number generator) used for testing * Fix lint issue on nil check in []byte type * Update permission from 0444 to 0600 * Update 0444 to 0400

* Remove nolint comment and export timeout variables * Drop unnecessary nolint * Add comment * Fix lint issue

* Add share length validation in runner construction * Align to error handling in runners constructions * Add validation to committee runner * Add runner construction tests * Refactor runner construction in testingutil to deal with creation errors * Generate JSON tests * Fix lint issue * Fix comments

* Remove redundant validation * Align error string

* Sort signers in decided message * Add test for sorted signers in decided msg * Generate JSON tests * Fix lint issue

* Stop processing consensus messages after instance is decided * Align error in qbft tests * Align errors in ssv tests * Generate JSON tests * Fix lint issue

* Remove leftover err check * Align argument variable name to type

alan-ssvlabs · 2024-09-01T00:15:28Z

Maybe p2p/SPEC.md needs some update at line 268-283 about Basic Validation

MatheusFranco99 · 2024-09-02T10:37:44Z

@alan-ssvlabs Indeed. Thanks!

…oring values

MatheusFranco99 · 2024-09-02T14:41:12Z

@GalRogozinski @alan-ssvlabs

In the p2p/SCORING.md file I have a code snippet for the message rate estimation. What do you guys think of creating a file with this code? The fact that p2p scoring needs it may be enough reason to add it. Also, the SSV repo uses exactly this function.

MatheusFranco99 · 2024-09-04T09:17:47Z

@GalRogozinski @alan-ssvlabs Moved the msg estimation code to a go file and added some tests.

GalRogozinski · 2024-10-13T10:04:21Z

Fixes #453

GalRogozinski

Looks very good!
Just a few comments

GalRogozinski · 2024-10-13T10:20:45Z

p2p/messagerate/message_rate_estimation.go

+// Expected number of messages per duty step
+
+func consensusMessages(n int) int {
+	return 1 + n + n + 2 // 1 Proposal + n Prepares + n Commits + 2 Decideds (average)


the average of 2 decides depends on n, no?

GalRogozinski · 2024-10-13T10:28:37Z

p2p/spectest/tests/messagerate/test.go

+func (test *MessageRateTest) Run(t *testing.T) {
+	for _, testCase := range test.TestCases {
+
+		// Build the list of committees
+		committees := make([]*messagerate.Committee, 0)
+		for _, committeesWithValidators := range testCase.CommitteesConfig {
+			committees = append(committees, testingutils.TestingDisjointCommittees(committeesWithValidators.NumCommittees, committeesWithValidators.NumValidators)...)
+		}
+
+		// Check result
+		expectedValue := testCase.ExpectedMessageRate
+		result := messagerate.EstimateMessageRateForTopic(committees)
+		require.InDelta(t, expectedValue, result, allowedError)
+	}
+}


I am thinking whether those tests are spectests?

The point of spec tests is to make sure the spec didn't break and for nodes to test spec alignment.
Are those tests doing anything like this? You wrote that this was added since the documentation referenced the message rate.
Still... I think this belongs to the research repository where we do simulations

GalRogozinski · 2024-10-13T10:58:39Z

p2p/validation/config.go

+
+// Protocol configuration for the MessageValidator
+type Config interface {
+	ProposerForHeightAndRound(height qbft.Height, round qbft.Round) types.OperatorID


You probably need to know the committeeLength?

Usually for a single function we declare a function type.
What you did here works as well but maybe we should be consistent.
I would simply add a function to validator.go and add in a comment that its output is defined by the QBFT leader selection function for clarity

GalRogozinski · 2024-10-13T11:29:12Z

p2p/validation/duty_fetcher.go

+type DutyFetcher interface {
+	HasProposerDuty(validatorIndex phase0.ValidatorIndex, slot phase0.Slot) bool
+	HasSyncCommitteeContributionDuty(validatorIndex phase0.ValidatorIndex, slot phase0.Slot) bool
+	HasSyncCommitteeDuty(validatorIndex phase0.ValidatorIndex, slot phase0.Slot) bool
+}


This looks like something that would be as part of our BeaconNode interface

GalRogozinski · 2024-10-13T11:35:54Z

p2p/validation/network_data_fetcher.go

+	ExistingValidator(validatorPK types.ValidatorPK) bool
+	ActiveValidator(validatorPK types.ValidatorPK) bool
+	ValidatorLiquidated(validatorPK types.ValidatorPK) bool
+	ExistingCommitteeID(committeeID types.CommitteeID) bool


For those methods we query the contract right?
And for the others we don't, right? So maybe should be 2 interfaces?

I also think we should add comments for interfaces methods, especially ones we only implement as test methods

GalRogozinski · 2024-11-26T21:52:10Z

qbft/spectest/tests/controller/latemsg/late_commit.go

@@ -39,6 +39,7 @@ func LateCommit() tests.SpecTest {
 				ControllerPostState: sc.ExpectedState,
 			},
 		},
+		ExpectedError: "not processing consensus message since instance is already decided",


How come all these messages were added?

GalRogozinski · 2024-11-26T21:53:31Z

qbft/commit.go

+	// Sort the OperatorIDs and Signatures in the SignedSSVMessage
+
+	pairs := make([]struct {
+		OpID types.OperatorID
+		Sig  types.Signature
+	}, len(ret.OperatorIDs))
+
+	for i, id := range ret.OperatorIDs {
+		pairs[i] = struct {
+			OpID types.OperatorID
+			Sig  types.Signature
+		}{OpID: id, Sig: ret.Signatures[i]}
+	}
+
+	// Sort the slice of pairs
+	sort.Slice(pairs, func(i, j int) bool {
+		return pairs[i].OpID < pairs[j].OpID
+	})
+
+	// Extract the sorted IDs and Signatures back into separate slices
+	for i, pair := range pairs {
+		ret.OperatorIDs[i] = pair.OpID
+		ret.Signatures[i] = pair.Sig
+	}


qbft changes because of message validation?

GalRogozinski · 2024-11-26T21:55:17Z

ssv/aggregator.go

+) (Runner, error) {
+
+	if len(share) != 1 {
+		return nil, errors.New("must have one share")
+	}
+


this has nothing to do with the PR right?

y0sher and others added 19 commits July 22, 2024 16:23

Spec - check att and sync duties exist before submitting (ssvlabs#468)

6921f58

Meta - Update to go1.22 (ssvlabs#474)

96429fb

* Update go1.20 to go1.22 * Update go.sum with mod tidy

Meta - Update dependencies (ssvlabs#483)

8edc50a

* Update dependencies * Fix lint issue * Generate JSON tests to trigger actions * Update fastssz * Generate JSON tests and align ssz error * Revert go-eth2-client version change * Revert fastssz upgrade * Generate SSZ and JSON tests

Meta - Drop unnecessary nolint comments (ssvlabs#477)

642d9d2

* Remove nolint comment and export timeout variables * Drop unnecessary nolint * Add comment * Fix lint issue

Meta - Drop redundant error (ssvlabs#475)

5daa242

Spec - Drop redundant validation for decided messages (ssvlabs#476)

b0f2b4d

* Remove redundant validation * Align error string

Spec - Sort signers in decided message (ssvlabs#484)

c61218a

* Sort signers in decided message * Add test for sorted signers in decided msg * Generate JSON tests * Fix lint issue

Spec - Stop processing after decided (ssvlabs#487)

9abeeec

* Stop processing consensus messages after instance is decided * Align error in qbft tests * Align errors in ssv tests * Generate JSON tests * Fix lint issue

Spec - Drop leftover error check (ssvlabs#469)

496d839

* Remove leftover err check * Align argument variable name to type

Add README with validation rules

af0414f

Add message validation implementation

1b9004f

Add message validation spec test

5956c72

Add tests

5f7aa4e

Generate JSON tests

78c4563

Export max size values for msg validation checks

8cfb29a

Generate JSON tests

2e2bfca

MatheusFranco99 added the spec label Aug 22, 2024

MatheusFranco99 requested a review from alan-ssvlabs August 22, 2024 12:46

MatheusFranco99 self-assigned this Aug 22, 2024

MatheusFranco99 requested a review from GalRogozinski August 22, 2024 12:46

Add a code documentation file

3ce2fef

MatheusFranco99 added 2 commits September 2, 2024 15:36

Add more details to the validation README

d84bed0

Update p2p/SPEC and p2p/SCORING with validation source and updated sc…

012509c

…oring values

Move message rate estimation snippet to a go file

0c26060

Add tests for the message rate estimation

90f3fb7

GalRogozinski added the Networking P2P networking specs label Oct 13, 2024

GalRogozinski reviewed Oct 13, 2024

View reviewed changes

GalRogozinski force-pushed the dev branch from ec462ef to cf804b6 Compare November 12, 2024 13:44

GalRogozinski reviewed Nov 26, 2024

View reviewed changes

GalRogozinski mentioned this pull request Dec 1, 2024

Validators per operator cap: 500 to 1000 #513

Merged

GalRogozinski force-pushed the dev branch 2 times, most recently from b5bd1d3 to bf0ff16 Compare December 19, 2024 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message Validation #491

Message Validation #491

MatheusFranco99 commented Aug 22, 2024 •

edited

Loading

alan-ssvlabs commented Sep 1, 2024

MatheusFranco99 commented Sep 2, 2024

MatheusFranco99 commented Sep 2, 2024

MatheusFranco99 commented Sep 4, 2024

GalRogozinski commented Oct 13, 2024

GalRogozinski left a comment

GalRogozinski Oct 13, 2024

GalRogozinski Oct 13, 2024

GalRogozinski Oct 13, 2024

GalRogozinski Oct 13, 2024

GalRogozinski Oct 13, 2024

GalRogozinski Nov 26, 2024

GalRogozinski Nov 26, 2024

GalRogozinski Nov 26, 2024

Message Validation #491

Are you sure you want to change the base?

Message Validation #491

Conversation

MatheusFranco99 commented Aug 22, 2024 • edited Loading

Overview

Documentation and message rate estimation

Tests

PubSub

Syntax validation

General semantics validation

Consensus semantics validation

Consensus QBFT logic validation

Consensus duty logic validation

Partial signature semantics validation

Partial signature duty logic validation

General duty logic validation

Wrong signature

Valid

alan-ssvlabs commented Sep 1, 2024

MatheusFranco99 commented Sep 2, 2024

MatheusFranco99 commented Sep 2, 2024

MatheusFranco99 commented Sep 4, 2024

GalRogozinski commented Oct 13, 2024

GalRogozinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatheusFranco99 commented Aug 22, 2024 •

edited

Loading