Sort signers in decided message #484

MatheusFranco99 · 2024-08-15T09:07:39Z

Overview

This PR adds the feature of sorting the signers in the decided message.

It adds a "sorted decided" test to test the sorting upon receiving commit messages in an unsorted order.

Closes #472

GalRogozinski · 2024-08-15T09:44:50Z

ssv/runner_validations.go

@@ -50,7 +50,7 @@ func (b *BaseRunner) ValidatePostConsensusMsg(runner Runner, psigMsgs *types.Par
 	}

 	// TODO https://github.com/ssvlabs/ssv-spec/issues/142 need to fix with this issue solution instead.
-	if b.State.DecidedValue == nil || len(b.State.DecidedValue) == 0 {


that's from another PR no?
you can resolve when solving conflicts

Due to the linter update, I have been fixing this lint issue in all the recent PRs so that actions would pass

GalRogozinski · 2024-08-15T09:48:17Z

qbft/spectest/tests/commit/sorted_decided.go

+)
+
+// SortedDecided tests the creation of the decided message that should have sorted signers
+func SortedDecided() tests.SpecTest {


no output messages?

Notice that ControllerSpecTest.OutputMessages is unused. For this test, we actually use ControllerSpecTest.RunInstanceData.ExpectedDecidedState.BroadcastedDecided to test the broadcasted decided message.

Notice that not only does none of the implemented tests define it but the spec test itself doesn't use it.

As I see it, the reason is that the controller spec test wants to test decided values and the decided messages for a list of instances (and not the instance's broadcasted messages).

I think that we should either drop this field or implement it in the tests if wanted.

y0sher

lgtm

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

* 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>

MatheusFranco99 added 3 commits August 15, 2024 10:05

Sort signers in decided message

6d9eb0f

Add test for sorted signers in decided msg

2d59be1

Generate JSON tests

00547f1

MatheusFranco99 requested a review from GalRogozinski August 15, 2024 09:07

MatheusFranco99 self-assigned this Aug 15, 2024

Fix lint issue

7e24bdb

GalRogozinski reviewed Aug 15, 2024

View reviewed changes

GalRogozinski requested changes Aug 15, 2024

View reviewed changes

GalRogozinski mentioned this pull request Aug 15, 2024

ControllerSpecTest doesn't use OutPutMessagesField #486

Open

GalRogozinski approved these changes Aug 15, 2024

View reviewed changes

GalRogozinski requested a review from alan-ssvlabs August 15, 2024 19:21

GalRogozinski added the spec label Aug 15, 2024

y0sher approved these changes Aug 19, 2024

View reviewed changes

alan-ssvlabs approved these changes Aug 19, 2024

View reviewed changes

Merge branch 'dev' into sort-decided-signers

0a09998

MatheusFranco99 merged commit c61218a into ssvlabs:dev Aug 20, 2024
2 checks passed

MatheusFranco99 deleted the sort-decided-signers branch August 20, 2024 10:41

GalRogozinski pushed a commit that referenced this pull request Sep 12, 2024

Spec - Sort signers in decided message (#484)

c9a618c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort signers in decided message #484

Sort signers in decided message #484

MatheusFranco99 commented Aug 15, 2024 •

edited

Loading

GalRogozinski Aug 15, 2024

MatheusFranco99 Aug 15, 2024 •

edited

Loading

GalRogozinski Aug 15, 2024

MatheusFranco99 Aug 15, 2024

MatheusFranco99 Aug 15, 2024

y0sher left a comment

Sort signers in decided message #484

Sort signers in decided message #484

Conversation

MatheusFranco99 commented Aug 15, 2024 • edited Loading

Overview

GalRogozinski Aug 15, 2024

Choose a reason for hiding this comment

MatheusFranco99 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

GalRogozinski Aug 15, 2024

Choose a reason for hiding this comment

MatheusFranco99 Aug 15, 2024

Choose a reason for hiding this comment

MatheusFranco99 Aug 15, 2024

Choose a reason for hiding this comment

y0sher left a comment

Choose a reason for hiding this comment

MatheusFranco99 commented Aug 15, 2024 •

edited

Loading

MatheusFranco99 Aug 15, 2024 •

edited

Loading