CommitteeRunner: allow signatures of unknown validators #442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… validators for root"
…und for a root in CommitteeRunner
ssv/committee_runner.go
Outdated
Comment on lines
237
to
242
| if !exists { | ||
| anyErr = errors.Wrap(err, "could not find beacon object for validator") | ||
| continue | ||
| } | ||
| sszObject, rexists := validatorObjs[root] | ||
| if !rexists { | ||
| if !rexists { |
There was a problem hiding this comment.
sszObject, exists := validatorObjs[root] ; if !exists {
This will limit exists scope to the if block. Then we won't need the nasty rexists trick.
There was a problem hiding this comment.
Pushed a change. Is that what you meant?
There was a problem hiding this comment.
not exactly...
if you do it on a one line with ; you will also limit the scope of exists to the if block...
Currently I think (not sure) you are overshadowing the previous exists and this may cause subtle bugs
Comment on lines
+50
to
+54
| var shareSample *types.Share | ||
| for _, share := range b.Share { | ||
| shareSample = share | ||
| break | ||
| } |
There was a problem hiding this comment.
😅
no way around it.. I guess
moshe-blox
reviewed
Jul 3, 2024
| testingutils.GetSSZRootNoError(testingutils.TestingSignedAttestation(ks)), | ||
| }, | ||
| DontStartDuty: true, | ||
| ExpectedError: expectedError, |
There was a problem hiding this comment.
Returning an error usually means that the execution failed because something unexpected happened.
(for example os.Open would only return an error if the file did not open for some unexpected reason)
I'd suggest to not return an error and instead check that the submitted attestations are only for the known validators.
There was a problem hiding this comment.
Removing the error in CommitteeRunner and making explicit the ValidatorIndex associated with the submitted attestation.
moshe-blox
approved these changes
Jul 3, 2024
moshe-blox
reviewed
Jul 3, 2024
Comment on lines
+49
to
+57
| // Get committee (unique for runner) | ||
| var shareSample *types.Share | ||
| for _, share := range b.Share { | ||
| shareSample = share | ||
| break | ||
| } | ||
| if shareSample == nil { | ||
| return errors.New("can not get committee because there is no share in runner") | ||
| } |
There was a problem hiding this comment.
if BaseRunner requires operator ids, it should probably just require them in NewBaseRunner, and then we won't have to do this "hack"
…sociated with the submitted beacon root.
GalRogozinski
approved these changes
Jul 3, 2024
GalRogozinski
deleted the
allow-unknown-validator-signature-in-committee-runner
branch
GalRogozinski
added a commit
that referenced
this pull request
Jul 14, 2024
* init wip * process consensus msg wip * Replace to "attestation and sync committee" tests. Duties tests * new share and operator * cluster validations * add methods to operator and share * validation with message ID * beacon types encoding * use operator instead of share * more changes to cluster runner * beacon_types.go + partial_sig * Extend SignedSSVMessage to multiple signers and add FullData * Update tests and utils to multiple signers change * post consensus * use operators * shares * shares * more error fixes * Deprecate SignedMessage and use SignedSSVMessage * Deprecate SignedMessage in testingutil * Deprecate SignedMessage in tests * Generate JSON tests * Deprecate SignedPartialSignature to use PartialSignatureMessages * Deprecate SignedPartialSignature in testingutils * Deprecate SignedPartialSignature in tests * Fix lint issues. Add change to msg validation * Generate JSON tests * more error fixes * Apply renaming suggestions * Generate JSON tests * fix one more conflict * fix more conflicts * resolved merge conflicts * comment out code in validator.go * remove operator id from signer * rename * more changes * more changes * Deprecate ShareSigner, KeyManager and DKGSigner * runner roles * use operator ID in messages again * fix preconsensus justifications * cluster runner finds validators * use committee to verify partial sig * renaming, some helpers added, adopted to work with ssv.RoleCluster * added missing .QBFTController to access the share. Added primitive getValPubKeyByValIdx impl * Implemented valid test for runners/new_duty * added decided.go tests * rename cluster to committee * add GetOperatorSigner * fix runner signatures * add operator signer to validator registration * Decode CD for BeaconVote * delete legacy runners * rename files * fixed most of the /runners/newduty/ tests. #384 * stop duties * stop duties * cutoff round * fix newduty/test.go * added changes to post_invalid_decided.go * fixed some compilation errors * fix committee_runner broadcasting * fix committee_runner broadcasting * value check * New message structures with RSA signature (#382) * Extend SignedSSVMessage to multiple signers and add FullData * Update tests and utils to multiple signers change * Deprecate SignedMessage and use SignedSSVMessage * Deprecate SignedMessage in testingutil * Deprecate SignedMessage in tests * Generate JSON tests * Deprecate SignedPartialSignature to use PartialSignatureMessages * Deprecate SignedPartialSignature in testingutils * Deprecate SignedPartialSignature in tests * Fix lint issues. Add change to msg validation * Generate JSON tests * Apply renaming suggestions * Generate JSON tests * Deprecate ShareSigner, KeyManager and DKGSigner * Fix merge issues * use OPERATORID Co-authored-by: Matus Kysel <MatusKysel@users.noreply.github.com> * use struct map * delete unused getters --------- Co-authored-by: Gal Rogozinski <galrogogit@gmail.com> Co-authored-by: Matus Kysel <MatusKysel@users.noreply.github.com> * add comments * no need for comment actually * validations for post consensus msg * work with beaconVote and consensus_data in seperate * delete unneccessary fields * fic errors * Fix StartDuty * fixed circular dependencies * partially fixed /ssv * fix testingutil * fix types * removed partial quorum & fixed qbft tests * fixed run_test.go * Fix msg processing spectest in ssv * fixed valcheck/test.go * Fix ssv runner tests * fixed lint errors * Implement panics in runners * Fix type tests; Delete old attestation and sync committee CD tests * Fix qbft test specs and roots * Generate JSON tests * Solve some errors in ssv tests (fix unknown validator check, add validator index field, fix cgo issue, fix nil pointer issues) * Add custom json marshal for runner state. Fix typo that broke tests * Fix typos, implementation and testing utils * Fix ssv test issues. Fix consensus data fields * Fix typo. Comment old attestation valcheck tests * Generate JSON tests. Removed unused functions for lint * minor runner fixes * Add custom encoding for start duty test (to deal with the duty interface) * fixed linter error + commented all non-working tests * Re-enable all tests and generate JSON * Refactor testing duties * Consensus not started and decided tests * Add tests: duplciate duty, finished, first height, not decided * Add tests: post (future, invalid, wrong) decided * Make validator receive Duty to start duty (and not BeaconDuty) * Make MsgProcessingSpecTest handle duty interface. Add valid consensus msg test * Fix CommitteeRunner type assertion * Fix sync committee post-consensus signed object * Add consensus valid decided test * Generate JSON tests * Add tests: valid decided 7, 10, 13 * Fix expected root and object calculation. Test: post finish attestation * Add consensus tests: post finish, post decided, past msg and invalid signature * Add tests: invalid decided, future msg, future decided and future decided no instance * Generate JSON tests * Add post consensus tests: valid, unknown signer, unordered roots * Deprecate DomainCommittee. Add test: too many roots * Add post consensus tests: quorum and too few roots * Add post consensus tests: pre decided, post quorum, post finish, no running duty, nil msg * Add post consensus tests: invalid msg (slot, msg, operator sig), invalid then quorum, invalid quorum then valid quorum * Add post consensus tests: invalid (msg sig, root, value, beacon sig), incosistent signer, duplicate msg (diff root, diff root then quorum) * Generate JSON tests * Fix lint (unreachable code) * Add new post-consensus test: wrong validator index * rename bloxapp->ssvlabs * solve more conflicts * generate ssz * more bloxapp->ssvlabs * fix error * remove double line * fix test * generate jsons * fix qbft roots and generate jsons * add TODO * review comments rename sender * better comment * fix typo * change int to round * functions not in use * clusterID -> committee ID * mistmatch typo * rename in generate.go * commit suspicious files * PR comments * use encoder interface * Alan Domain (#394) * Add Alan Domain * add to mainnet forks * Fix value check (#396) * use ValidaotrPK * update value_check.go * fix error * fix tests * add attestation_slashable check to post_consensus * fix signature * fix committee_runner.go * add test stubs * fix all tests * nolint * new tests.json * fix slashable.go * make value_check fail if there's a majority of slashable attestations * add committeeIndex * check only in consensus data * delete unneeded test * fix -1 * use same committeeIndex * Fix role mapping in MapDutyToRunnerRole (#397) * Fix role mapping in MapDutyToRunnerRole * Replace types.MapDutyToRunnerRole(data.Duty.Type) with data.Duty.RunnerRole() * Convert unit test for MapDutyToRunnerRole into duty spec tests * Cluster consensus tests (#391) * Define wrong beacon vote in testingutils * Add validator pubkeys in testingutils * Add tests: start duties (attestation, sync committee, mixed, non duties) * Add tests: maximum possible duties, valid CD, wrong CD, decided * Generate JSON tests * Add testing utils for 500 validators shares * Add testing util for committee runner with multiple shares * Allow PartialSignatureMessages to have at most 1000 signature messages * Add post consensus message for decided test with 500 validators * Generate JSON tests * Fix lint * Apply suggestions * Fix CommitteeMsgID to use ClusterID * Make MsgProcessingSpecTest use Committee object for CommitteeRunner instead of Validator * Add validation and nil check for Committee * Adjust tests errors. Add happy flow test * Generate JSON tests * Introduces spec test for Committee. Refactor msg and beacon root comparison * Refactor TestingShare * CommitteeRunnerWithShareMap and BaseCommittee testing utils * Add share to Committee * Add MultiTest for Committee * Change committee tests directory and spec type * Generate JSON tests * Refactor test to encompass multiple number of validators in one test file * Fix committee runner: track submitted duties, validators for sync committee, beacon object for sync committee * Add test: partially invalid quorum (bad root) then valid quorum * Generate JSON tests * Change CommitteeRunner to check validity of reconstructed signature for all duties * Add test: partially invalid quorum (bad beacon sig) then valid quorum * Fix lint issue due to merge * Add test: Happy flow for the committee spec test * Fix CommitteeRunner: return BeaconObject based on validator and root * Add test: mixed committees * Testing util: add slot to post consensus sync committee message * Fix Committee: Track highest duty slot for a general duty type * Add test: past msg. Fix committee spec test * Change CommitteeRunner: Drop slot from submitted duties map and reset it upon new duty * Apply suggestions * Fix lint issues * Remove deprecated syncMsg.ValidatorIndex fix * Rename past msg test. Add stub tests * Test: CommitteeRunner - Proposal with consensus data (#399) * Refactor previous test names to use beacon vote * Add test: proposal with consensus data * Generate JSON tests * Tests: Past msg for committee (#403) * Add test: past msg for finished duty * Add test: past msg for duty that does not exist * Tests: BeaconVote encoding and value check tests (#402) * Add encoding test * Extend KeyManager testing util to store slashable data root per validator * Extend value check test to allow using slashable root per validator * Add tests: majority and minority slashable * Add duty value check test cases for the committee role * Apply suggestions * zip json (#407) * Tests: Cluster consensus - Several duties (#401) * Refactor existing committee tests to the new committeesingleduty package * Implement singleton pattern for TestingVerifier for speed-up * Extend "compare output" functionality to handle msgs asynchronicity * Use new network for each new committee runner; fix slot/height in some testingutil messages * Add test: sequenced decided duties * Add test: sequenced happy flow duties * Fix: avoid concurrent read and writes * Fix Committee: Add nil check to BeaconDuty * Add test: shuffled decided duties * Compare output messages in asynchornous order only for committee spec test (go back to original compare function) * Reduce size of tests.json by removing big tests * Add test: shuffled happy flow duties with the same validators * Generate JSON tests * Refactor input, output and beacon root to testing utils * Refactor committee tests utils (input, output, beacon roots) * Add test: shuffled happy flow duties with different validators * Add test: failed duties then successful duties * Improve test comment * Enable test cases with 500 validators * Add cache to TestingKeyManager to speed up tests * add json.gz --------- Co-authored-by: Gal Rogozinski <galrogogit@gmail.com> * up go-eth2-client (#408) * Added Committee Validation to Message (#395) * rename clusterID to committeeID * add missing validate message * fix bad copy * fix runner * fix base runner * fix getID for future decided no instance * generate jsons * fix lint * add test * generate jsons * mov wrong_message_id.go * add jsons * generate json again * non-empty messages * Revert "rename clusterID to committeeID" This reverts commit 7272f3d. * fix back to bad name * regenerate jsons * regenerate more jsons * Cluster consensus fixes (#390) * small fixes * fix errors * add comment * delete expected error from tests * add error and fix test * add expected error * add more expected error * fix committee_runner.go and tests * restore_pastmsg * Remove test cases with 500 validators (#409) Co-authored-by: Gal Rogozinski <galrogogit@gmail.com> * Validate empty duties (#410) * validate beacon duties are not empty * add no duty test * fix comment and don't use multispectest * Allow committee duties to run in parallel (#412) * delete filter * add slashing protection for attestations * working version of test * add comment * add to all tests * remove isStopped * fix ssz encoding * add jsons * key manager clone * remove expected error from test * fix shuffled haapy_flow_duties * generate jsons * properly generate, marshal and unmarshal json * Refactor TestingKeyManager to hold an immutable structure (TestingKeyStorage) that holds the keys * Remove double-check to solve race condition * Fix race condition in singleton double-check * Remove extra newline --------- Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com> * Fix DomainType in CommitteeRunner (#414) * Fix DomainType used in MsgID * Remove TODO text * Fix race condition in testing verifier * Use sync.Once for singleton constructor * Remove unnecessary lock usage in creation * Fix/api (#416) * It could be that roots will be missing.. we still want to proceed * make GetAttestations parameters optional * Fix runner state in MessageProcessingSpecTest (#413) * Fix runner's empty state in tests * Generate JSON tests * Revert to use always test.Runner for state. Use test.Runner in CreateRunnerFn * Refactor BaseCommitteeWithRunnerSample to BaseCommitteeWithCreatorFieldsFromRunner * Fix quorum (#417) * change position of variable so name will make sense * new way to get quorum * return uint64 for ssz * make ssz * fix errors * add operator test and delete unneccessary share test * delete share tests * change all tests * generate jsons * fix test * remove quorum from runner * add threshold to newduty test * generate jsons * generate more jsons * fix json * fix proposal and prepare * fix roundchange and commit * fix rest of tests * fix types * more jsons * Rename structures (#420) * Rename Operator->CommitteeMember and CommitteeMember->Operator * Remove fixed post root strings * Generate JSON tests * Generate SSV JSON tests * Align structures' comments * Remove old comment * Rename qbft.Controller.Share to CommitteeMember * Rename qbft.State.Share to CommitteeMember * Generate JSON tests. Fix BaseRunner's Share's JSON mapping * remove TODOs * Remove needless constructor (#424) * refactor function * remove needless constructor * refactor signing * Alan - New message sizes (#398) * use domain from controller (#425) * BeaconNode: Submit multiple attestation duties (#428) * Update BN interface to submit multiple attestations * Adjust CommitteeRunner to submit multiple attestations at once * Generate JSON tests * Apply suggestions * Change Spec.MD (#426) * use committee mapping * remove all sync protocols * rename BeaconDuty -> ValidatorDuty * delete unused function * ConsensusData update (#431) * Change BeaconVote location * Rename ConsensusData to ValidatorConsensusData * Propagate ConsensusData renaming to ValidatorConsensusData * Propagate renaming to testingutils and maxmsgsize tests * Rename ConsensusDataTest to ValidatorConsensusDataTest * Propagate renaming to ssv tests * Rename directory from consensusdata to validatorconsensusdata * Generate JSON tests * rename argument in NewMsgID function * Move json utils to a testutils file (#432) * Move all qbft encoding, decoding and root methods to a separate json_testutils file * Move all ssv encoding, decoding and root methods to a separate json_testutils file * add comment * use encoder interface (#438) * Remove convenience functions (#435) * Remove CreateValidatorConsensusData convenience function * Remove GetOperatorIDs function from SignedSSVMessage * Remove SSVMessageToSignedSSVMessage convenience function * Remove PartialSignatureMessagesToSignedSSVMessage convenience function * Remove NewBeaconVote convenience function * Calculate CommitteeID when creating message IDs (#433) * add DomainType to CommitteeMember * give domain value in tests and fix generate * generate files * Use PEM exclusively (#430) * use pem * generate jsons * update ssz size * update ssz size * Delete preconsensus justifications (#436) * add DomainType to CommitteeMember * give domain value in tests and fix generate * generte files * delete preconsensus justifications * remove justifications from consensus_data.go * remove justifications from tests * one more fix * generate files * fix lint * fix message size * fix encoding * correct comment * generate files * fix size again * generate files * Update ssv/committee_runner.go Co-authored-by: rehs0y <lyosher@gmail.com> * Add ProcessingMessage structure to QBFT (#440) * Add ProcessingMessage structure * Use ProcessingMessage in MsgContainer and State * Use ProcessingMessage in qbft Instance and associated functions * Use ProcessingMessage in qbft controller * Update qbft tests with ProcessingMessage * Update ssv tests with ProcessingMessage * Use ProcessingMessage in p2p msg validation * Generate JSON tests * Use ProcessingMessage validation (unify validations) * Adjust test error string * Remove ProcessingMessage from ssz generation * Remove unnecessary decoding check * Use GetShare to avoid duplicated code (#443) * Use GetShare to avoid duplicated code * Use GetShare to avoid duplicated code * Refactor PartialSigContainer.HasSigner: avoid duplication code and rename (#444) * Use GetSignature in HasSigner to avoid duplicated code * Rename HasSinger to HasSignature since we're looking for a signature * Fix RunnerRole argument name (#445) * Use naming in complex nested map (#446) * Use naming to improve readability of nested map * Add comment to better explain nested map * Apply suggestion * remove ECRecover * Complete SignedSSVMessage tests (#441) * Add SignedSSVMessage missing tests * Rename file according to test name * Add missing SignedSSVMessages tests in qbft * Add missing SignedSSVMessages tests in ssv postconsensus * Add missing SignedSSVMessages tests in ssv preconsensus * Generate JSON tests * Remove SignedSSVMessage tests from QBFT/messages * Add SignedSSVMessage validation tests in SSV/Runner/Consensus * Generate JSON tests * Generate JSON tests * CommitteeRunner: allow signatures of unknown validators (#442) * Do validator index validation, in psig messages, for validator duties only * Drop expected error (unknown validator index) for committee runners * Add quorum of invalid validator index test to trigger "could not find validators for root" * Allow unsyced validators set: use "continue" if a validator is not found for a root in CommitteeRunner * Test quorum for unknown validator index and a known one (tests the continue command) * Generate JSON tests * Apply suggestion * Remove unknown validator edge case error * Remove deprecated error in tests. Make explicit the ValidatorIndex associated with the submitted beacon root. * Take RSA Signer and Verification out of configuration (#447) * make operator signer concrete type * add RSA verifier * move verify code * use signer and verifier * more fixes * fix tests * fix config * more fixes * fix qbft tool * sign correctly * generate-jsons * remove testing verifier * generate jsons * omit json * omit operator signer from json * fix jsons * Cleanup (#450) * remove empty files * remove unused crypto functions * move deposit data file to testutils * Remove leftover: HighestDutySlotMap in CommitteeAlias (#456) * Remove deprecated HighestDutySlotMap field from CommitteeAlias * Generate JSON tests * TestingKeyManager: Slashing by Slot (#449) * Use slots instead of data root to do slashing check * Adjust ValCheck SpecTest to use slashable slots * Adjust test cases * Add test case for a valid BeaconVote with a slot that is different than the slashable slot * Fix ValueCheck in testing runners (#457) * add share pubkey to proposer check in tests * add share pubkey to runner in testutils * fix test runner to use correct km in valcheck * Change baseRunnerWithShareMap to use ValueCheck with the same KeyManager * Remove extra unnecessary value check * Revert "Remove extra unnecessary value check" This reverts commit 4027756. * Drop extra redundant slashing check * Fix slashingRoot added for concurrent slashing decideds check * Align new error in test * Remove unused functions --------- Co-authored-by: Gal Rogozinski <galrogogit@gmail.com> * remove comment * add comment * moved decided value to runner * Simplify comparison methods (#461) * Fix CompareSignedSSVMessageOutputMessages. Refactor and simplify comparison methods * Generate JSON tests * Refactor to remove duplicated code * update ssz * make generate-ssz * use global cutoff time of 12 --------- Co-authored-by: Lior Rutenberg <liorr@blox.io> Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com> Co-authored-by: Anton Korpusenko <antokorp@gmail.com> Co-authored-by: Matus Kysel <MatusKysel@users.noreply.github.com> Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com> Co-authored-by: rehs0y <lyosher@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
If, in a committee, there is, for some reason, a disparity between known validators, we can hit some edge condition that will prevent the creation of a Beacon object. Take a look at this example:
In the case where an operator fails whenever it sees a message with an unknown validator, no Beacon Object for validator A will ever be created. There could be more realistic scenarios where we lose some fault tolerance where not all operators create the beacon object.
Fix
This PR changes the CommitteeRunner to allow signatures of unknown validators. For that, we made the following changes:
validatePartialSigMsgForSlotnow doesn't check if the validator index exists in the share map.validateValidatorIndexInPartialSigMsgfunction called to validate ValidatorIndex for validator runners.CommitteeRunner.ProcessPostConsensus, instead of returning an error in case of an unknown (validator, root) pair, we call "continue" to process remaining (root, validator) pairs. This is due to the edge case in which an operator may not be up to date and not have a (validator, root) pair, even though it received a quorum of signatures for such root.Tests: