Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Share length validation #478

Merged
merged 9 commits into from
Aug 20, 2024
Merged

Share length validation #478

merged 9 commits into from
Aug 20, 2024

Conversation

MatheusFranco99
Copy link
Contributor

Overview

This PR adds a validation to the runners' construction function (except by CommitteeRunner) to assert that the share argument has only one entry (only one share).

@MatheusFranco99 MatheusFranco99 self-assigned this Aug 13, 2024
GalRogozinski
GalRogozinski reviewed Aug 14, 2024
View reviewed changes
ssv/aggregator.go Outdated Show resolved Hide resolved
@GalRogozinski GalRogozinski added Networking P2P networking specs spec and removed Networking P2P networking specs labels Aug 14, 2024
@GalRogozinski GalRogozinski mentioned this pull request Aug 14, 2024
Open
9 tasks
y0sher
y0sher approved these changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

Approving this but with an *.

  • @GalRogozinski @MatheusFranco99
    I think if this check is required maybe its time to think of separating the base for committee and other runners. Since other runners will never recv more than one share, it looks weird that their func signature can recv a map of shares.

@MatheusFranco99 MatheusFranco99 mentioned this pull request Aug 19, 2024
Closed
@MatheusFranco99
Copy link
Contributor Author

@y0sher I agree.
I have opened an issue for that to be done later on

alan-ssvlabs
alan-ssvlabs approved these changes Aug 19, 2024
View reviewed changes
ssv/spectest/tests/runner/construction/many_shares.go Outdated
ks := testingutils.KeySetMapForValidators(10)
shares := testingutils.ShareMapFromKeySetMap(ks)

// No errors since one share must be valid for all runners
Copy link

Choose a reason for hiding this comment

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

Is this comment right for this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed not. Updating. Thanks for the catch!

ssv/spectest/tests/runner/construction/no_shares.go Outdated

shares := map[phase0.ValidatorIndex]*types.Share{}

// No errors since one share must be valid for all runners
Copy link

Choose a reason for hiding this comment

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

and here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating, thanks!

@MatheusFranco99 MatheusFranco99 merged commit 02beeb9 into ssvlabs:dev Aug 20, 2024
2 checks passed
@MatheusFranco99 MatheusFranco99 deleted the share-length-validation branch August 20, 2024 10:43
GalRogozinski pushed a commit that referenced this pull request Sep 12, 2024
@MatheusFranco99 @GalRogozinski
Spec - Share length validation (#478)
f87147c 
* 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
GalRogozinski pushed a commit that referenced this pull request Nov 12, 2024
@alan-ssvlabs @y0sher @MatheusFranco99
Spec - Move domainType from CommitteeMember and Share to Network (#494)
ec462ef 
* remove domaintype from committeeMember

* remove domaintype from share

* add config for ssv containing domainType

* add custom unmarshal for baserunner to avoid error in unmarshalling config

* move domainType to Network

* add testing domain in NewTestingNetwork function

* Spec - filter committee duties per slot validator duties (#467)

* 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 <48058141+MatheusFranco99@users.noreply.github.com>

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

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

* Update go1.20 to go1.22

* Update go.sum with mod tidy

* Meta - Update dependencies (#483)

* 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 - Fix static analysis issues (#480)

* 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

* Meta - Drop unnecessary nolint comments (#477)

* Remove nolint comment and export timeout variables

* Drop unnecessary nolint

* Add comment

* Fix lint issue

* Spec - Share length validation (#478)

* 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

* Meta - Drop redundant error (#475)

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

* Remove redundant validation

* Align error string

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

* Sort signers in decided message

* Add test for sorted signers in decided msg

* Generate JSON tests

* Fix lint issue

* Spec - Stop processing after decided (#487)

* 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 (#469)

* Remove leftover err check

* Align argument variable name to type

* Spec - Secure key storage (#481)

* Implement secure key storage using PKCS8

* Fix lint issue

* Switch back PKCS8 to PKCS1

* Meta - Remove residual DKG (#502)

* Remove DKG signature type

* Remove DKG msg type

* Remove DKGOperators field from TestKeySet

* Remove unused ecdsaKeys field from TestingKeyStorage

* Remove unused "ecdsaSKFromHex" function

* Generate JSON tests

* Spec - Add GoSec action and fix issues (#505)

* Add github action and makefile command

* Fix issues in round robin proposer function

* Fix bad PutUint32 in GetCommitteeID

* Fix issue with HasQuorum and HasPartialQuorum

* Add role sanitization in GetRoleType and NewMessageType

* Add sanitization to BeaconNetwork methods

* Add sanitization in testingutils

* Add sanitization to height usage in test files

* Fix uint64 conversion in runner/postconsensus/valid_msg test

* Sanitize ValidatorIndex conversion

* Update action name

* Fix tests to use valid RunnerRoles

* Generate SSZ

* Generate JSON tests

* Revert the change on GetCommitteeID

* Add nosec G115 to GetCommitteeID

* revert the merge because it was merged with origin main by accident

---------

Co-authored-by: rehs0y <lyosher@gmail.com>
Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com>
GalRogozinski pushed a commit that referenced this pull request Nov 12, 2024
@alan-ssvlabs @y0sher
@MatheusFranco99 @GalRogozinski
Spec - Move domainType from CommitteeMember and Share to Network (#494)
cf804b6 
* remove domaintype from committeeMember

* remove domaintype from share

* add config for ssv containing domainType

* add custom unmarshal for baserunner to avoid error in unmarshalling config

* move domainType to Network

* add testing domain in NewTestingNetwork function

* Spec - filter committee duties per slot validator duties (#467)

* 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 <48058141+MatheusFranco99@users.noreply.github.com>

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

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

* Update go1.20 to go1.22

* Update go.sum with mod tidy

* Meta - Update dependencies (#483)

* 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 - Fix static analysis issues (#480)

* 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

* Meta - Drop unnecessary nolint comments (#477)

* Remove nolint comment and export timeout variables

* Drop unnecessary nolint

* Add comment

* Fix lint issue

* Spec - Share length validation (#478)

* 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

* Meta - Drop redundant error (#475)

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

* Remove redundant validation

* Align error string

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

* Sort signers in decided message

* Add test for sorted signers in decided msg

* Generate JSON tests

* Fix lint issue

* Spec - Stop processing after decided (#487)

* 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 (#469)

* Remove leftover err check

* Align argument variable name to type

* Spec - Secure key storage (#481)

* Implement secure key storage using PKCS8

* Fix lint issue

* Switch back PKCS8 to PKCS1

* Meta - Remove residual DKG (#502)

* Remove DKG signature type

* Remove DKG msg type

* Remove DKGOperators field from TestKeySet

* Remove unused ecdsaKeys field from TestingKeyStorage

* Remove unused "ecdsaSKFromHex" function

* Generate JSON tests

* Spec - Add GoSec action and fix issues (#505)

* Add github action and makefile command

* Fix issues in round robin proposer function

* Fix bad PutUint32 in GetCommitteeID

* Fix issue with HasQuorum and HasPartialQuorum

* Add role sanitization in GetRoleType and NewMessageType

* Add sanitization to BeaconNetwork methods

* Add sanitization in testingutils

* Add sanitization to height usage in test files

* Fix uint64 conversion in runner/postconsensus/valid_msg test

* Sanitize ValidatorIndex conversion

* Update action name

* Fix tests to use valid RunnerRoles

* Generate SSZ

* Generate JSON tests

* Revert the change on GetCommitteeID

* Add nosec G115 to GetCommitteeID

* revert the merge because it was merged with origin main by accident

---------

Co-authored-by: rehs0y <lyosher@gmail.com>
Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alan-ssvlabs alan-ssvlabs alan-ssvlabs approved these changes

@GalRogozinski GalRogozinski GalRogozinski approved these changes

@y0sher y0sher y0sher approved these changes

Assignees

@MatheusFranco99 MatheusFranco99

Labels
spec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MatheusFranco99 @y0sher @GalRogozinski @alan-ssvlabs