Pre consensus fix #240

alonmuroch · 2023-05-16T15:00:04Z

fixes round change (not prev prepared) pre-consensus justification
fixing pre-consensus justification now calls baseRunner.decide

- not prepared round change will return instance start value - fix message create spec test

- some comments/ code fixes - add pre-consensus to randao test fixtures

- each duty version to const -

# Conflicts: # qbft/spectest/tests/create_message_spectest.go # ssv/spectest/all_tests.go # ssv/spectest/tests/runner/consensus/post_decided.go # ssv/spectest/tests/runner/consensus/post_finish.go # ssv/spectest/tests/runner/consensus/valid_decided.go # ssv/spectest/tests/runner/consensus/valid_decided_7_operators.go # ssv/spectest/tests/runner/full_happy_flow.go # ssv/spectest/tests/runner/full_happy_flow_sc.go # ssv/spectest/tests/runner/postconsensus/duplicate_msg.go # ssv/spectest/tests/runner/postconsensus/duplicate_msg_different_roots.go # ssv/spectest/tests/runner/postconsensus/inconsistent_beacon_signer.go # ssv/spectest/tests/runner/postconsensus/post_quorum.go # ssv/spectest/tests/runner/postconsensus/quorum.go # ssv/spectest/tests/runner/postconsensus/quorum_7_operators.go # ssv/spectest/tests/runner/postconsensus/unknown_signer.go # ssv/spectest/tests/runner/preconsensus/post_decided.go # types/testingutils/beacon_node.go # types/testingutils/runner_versioned.go # types/testingutils/ssv_msgs.go

MatheusFranco99

Looks good to me

alonmuroch · 2023-09-26T15:07:40Z

approved

ssv/runner.go

y0sher · 2024-01-22T14:40:25Z

ssv/partial_sig_container.go

-func (ps *PartialSigContainer) HasQuorum(root [32]byte) bool {
-	return uint64(len(ps.Signatures[rootHex(root)])) >= ps.Quorum
+// SignatureForRoot returns a map of signer and signature for a specific root
+func (ps PartialSignatureContainer) SignatureForRoot(root [32]byte) map[types.OperatorID][]byte {


not trying to nit pick but I think this function should be named

Suggested change

func (ps PartialSignatureContainer) SignatureForRoot(root [32]byte) map[types.OperatorID][]byte {

func (ps PartialSignatureContainer) SignaturesForRoot(root [32]byte) map[types.OperatorID][]byte {

y0sher · 2024-01-22T14:57:36Z

ssv/partial_sig_container.go

+	if len(ps) > 0 {
+		ret := make([][32]byte, 0)
+		for _, sigMsg := range ps {
+			for _, msg := range sigMsg.Message.Messages {
+				ret = append(ret, msg.SigningRoot)
+			}
+			break // only need to iterate first msg
+		}
+		return ret
+	}


we could do

if len(ps) =< 0 { return nil // or [][32]byte{} }

to reduce nesting and add readability

y0sher · 2024-01-22T15:04:32Z

ssv/pre_consensus_justification.go

@@ -39,8 +27,8 @@ func (b *BaseRunner) validatePreConsensusJustifications(data *types.ConsensusDat
 		return errors.New("wrong beacon role")
 	}

-	if data.Duty.Slot <= highestDecidedDutySlot {
-		return errors.New("duty.slot <= highest decided slot")
+	if qbft.Height(data.Duty.Slot) <= b.QBFTController.Height && b.QBFTController.Height != qbft.FirstHeight {


if we are on first height (which will never happen I guess) then its ok that the duty height is smaller than the controller height? I understand why its ok that its equal but not lower.

If it is small or equal it returns an error.
On first height it can only be equal.

you mean if firstHeight would be a number different than 0?
If it will be 1 then we won't need this special condition in all those places in the code

y0sher · 2024-01-22T15:07:35Z

ssv/runner.go

-			anyQuorum = true
-		}
+
+	if b.Share.HasQuorum(len(container)) {


it seems like HasQuorum just checks the number of sigs we have, don't we want to also check validity before we say we have quorum ?

for a message to enter the container it should pass validity checks

y0sher · 2024-01-22T15:14:00Z

..._comparison/tests_MultiMsgProcessingSpecTest/consensus_decided_invalid_value/aggregator.json

+												"Messages": [
+														{
+																"PartialSignature": "qFQDMkcUT9KZK3wO4GHZKlEZnJKWbhULlEMkiYC0igtS+snZxJ5tfnZ5oxKvIPRcBVMI0gP/rhHuEgGzpwBuVfuFYclsirb3DZVgPJcdVBjSkhkAt4polkJ/BKEHBdC5",
+																"SigningRoot": [


can we make this hex encoded like other roots/signatures/pubkeys?

not for this PR

y0sher · 2024-01-22T15:24:57Z

ssv/spectest/tests/msgcontainer/quorum_post_consensus_msg.go

+		MsgsToAdd: []*types.SignedPartialSignatureMessage{
+			testingutils.PostConsensusSyncCommitteeContributionMsg(ks.Shares[1], 1, ks),
+			testingutils.PostConsensusSyncCommitteeContributionMsg(ks.Shares[2], 2, ks),
+			testingutils.PostConsensusSyncCommitteeContributionMsg(ks.Shares[3], 3, ks),


should we test with messages other than synccommitteecontrib?

maybe...
when I look at this now not sure why this test is part of the PR (it is something Alon added)...
The PR should concentrate on preconsensus...

nevertheless I am adding you point in #264

y0sher · 2024-01-23T14:16:00Z

ssv/spectest/tests/msgcontainer/test.go

+	if len(test.PostReconstructedSignature) > 0 {
+		for i, m := range msg.Message.Messages {
+			sig, lastErr = c.ReconstructSignature(m.SigningRoot, testingutils.TestingValidatorPubKey[:])
+			require.EqualValues(t, test.PostReconstructedSignature[i], hex.EncodeToString(sig))
+		}
+		if len(test.ExpectedErr) == 0 {
+			require.NoError(t, lastErr)
+		} else {
+			require.EqualError(t, lastErr, test.ExpectedErr)
+		}
+	}


what about checking ExpectedErr even if postreconstructedsignature is empty? or there's no such test?

Currently there is a trick in tests that Alon did to have a non empty PostReconstructedSignature filled with empty strings.

In another test he kept the PostReconstructedSignature empty to not have an error...
So I'd rather not change his design at this point

y0sher · 2024-01-23T14:43:35Z

ssv/spectest/tests/runner/consensus/valid_decided_10_operators.go

-				Messages:                testingutils.SSVDecidingMsgsV(testingutils.TestSyncCommitteeContributionConsensusData, ks, types.BNRoleSyncCommitteeContribution),
-				PostDutyRunnerStateRoot: "a991b6470a8c7a55f4ce89aea91925c2d80a9a8c4258545cc2fb17cabc388719",
+				Messages:                testingutils.SSVDecidingMsgsV(testingutils.TestSyncCommitteeContributionConsensusData(ks), ks, types.BNRoleSyncCommitteeContribution),
+				PostDutyRunnerStateRoot: "ec47a2194bab81891bd581eaf427af211b53e13d1f77a6703eeb56a693caa878",


this is changed because preconsensus now affects the duty runner state?

the runner contains all msg containers that includes messages with preconsensus justifications, yes...
But I don't see in the logic where we add prec justification for this test... will look into it...

the commit 66c1585 has this change

This should be overridden by deneb imo and the root may need to change again...
I will resolve conflicts and then regenrate jsons and see what's going on

y0sher · 2024-01-23T14:46:56Z

ssv/spectest/tests/runner/full_happy_flow.go

+					testingutils.SSVDecidingMsgsV(testingutils.TestAggregatorConsensusData(ks), ks, types.BNRoleAggregator), // consensus
+					[]*types.SSVMessage{ // post consensus
+						testingutils.SSVMsgAggregator(nil, testingutils.PostConsensusAggregatorMsg(ks.Shares[1], 1)),
+						testingutils.SSVMsgAggregator(nil, testingutils.PostConsensusAggregatorMsg(ks.Shares[2], 2)),
+						testingutils.SSVMsgAggregator(nil, testingutils.PostConsensusAggregatorMsg(ks.Shares[3], 3)),
+					}...,


this duty now doesn't have pre consensus msgs?

testingutils.SSVDecidingMsgsV(testingutils.TestSyncCommitteeConsensusData, ks, types.BNRoleSyncCommittee), // consensus

this line adds precon messages
see pre_consensus_full_happy_flow

y0sher · 2024-01-23T15:59:52Z

ssv/spectest/tests/runner/pre_consensus_justifications/inconsisten_root_count.go

not from this pr but typo in file name

on one hand the fix for this doesn't belong in this PR...
on the other hand getting 3 busy people to approve a typo fix on a dedicated PR will take ages...

but it will add more conflicts so I won't fix

y0sher

Big PR. see my comments, mostly clarifications.

moshe-blox · 2024-02-13T08:51:45Z

ssv/partial_sig_container.go

-	return hex.EncodeToString(r[:])
+// AllSorted returns ordered by signer array of signed messages
+func (ps PartialSignatureContainer) AllSorted() []*types.SignedPartialSignatureMessage {
+	ret := make([]*types.SignedPartialSignatureMessage, 0)


allocate with capacity of len(ps)

will do because you copy it to impl even though ideally spec shouldn't care

moshe-blox · 2024-02-13T08:53:40Z

ssv/partial_sig_container.go

+	for _, m := range ps {
+		ret = append(ret, m)
+	}
+	sort.Slice(ret, func(i, j int) bool {


not relevant for spec, but same code length with less memory allocations:

Suggested change

sort.Slice(ret, func(i, j int) bool {

ret := make([]*types.SignedPartialSignatureMessage, len(ps))

for i, m := range ps {

ret[i] = m

}

why delete sort?

moshe-blox · 2024-02-13T09:10:33Z

ssv/aggregator.go

rename quorum to shouldProcess or the other way around to keep consistency?

personally i like quorum bc its self-explanatory

the problem is that sometimes you have a quorum but you don't process

alonmuroch and others added 30 commits May 16, 2023 17:47

qbft round change fix

9213261

- not prepared round change will return instance start value - fix message create spec test

hold pre-consensus msgs in container

b76a0c4

add pre-consensus justification when calling baseRunner.Decide

6c37340

comment

3ab8043

refactor msg container + initial tests

91ba801

Merge branch 'main' into pre-consensus-fix

cfb3953

new msg container spec tests

6149768

fixed get pre consensus justifications func

a69c0fc

add pre consensus to test fixtures

0266dd8

- some comments/ code fixes - add pre-consensus to randao test fixtures

add version and pre consensus to all consensus data fixtures

4beb72b

- each duty version to const -

prev

cc6fdff

ssv happy flow passes

5093469

merge fix qbft tests

92d01ab

merge ssv tests fix

eed7716

ssv full happy flow passes

55e916f

Merge branch 'main' into pre-consensus-fix

81871de

remove future msg

3a2f27a

remove backwards compatability

3792c8b

Merge branch 'meta/ignore-roots' into spec/remove_future_msg

72e5463

update states

0482edc

clean tests

1443cbb

regenerate ssv tests

35c8180

revert message validation comment

c8550c2

Merge branch 'main' into spec/remove_future_msg

752b291

more jsons

6019385

fix error messages

cf709d8

don't sync upon decided.go

f806ac8

Merge branch 'spec/remove_future_msg' into pre-consensus-fix

ad45eff

fix run_test

6667490

MatheusFranco99 approved these changes Sep 21, 2023

View reviewed changes

GalRogozinski requested a review from moshe-blox September 26, 2023 15:20

GalRogozinski approved these changes Sep 26, 2023

View reviewed changes

moshe-blox reviewed Oct 1, 2023

View reviewed changes

ssv/runner.go Show resolved Hide resolved

GalRogozinski added 5 commits October 2, 2023 12:35

change function name

c90b22c

Merge branch 'main' into pre-consensus-fix

0097b19

generate jsons

32a59cd

Merge branch 'main' into pre-consensus-fix

3fe7387

generate jsons

9b50318

y0sher reviewed Jan 22, 2024

View reviewed changes

y0sher reviewed Jan 23, 2024

View reviewed changes

y0sher requested changes Jan 23, 2024

View reviewed changes

GalRogozinski added 2 commits January 25, 2024 09:28

rename func

48f04e3

refactor roots

5609083

GalRogozinski mentioned this pull request Feb 5, 2024

Missing Tests #264

Open

5 tasks

moshe-blox reviewed Feb 13, 2024

View reviewed changes

moshe-blox approved these changes Feb 13, 2024

View reviewed changes

GalRogozinski added 2 commits February 21, 2024 11:26

len(ps)

690bee0

quorum -> should process

21dafa4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre consensus fix #240

Pre consensus fix #240

alonmuroch commented May 16, 2023 •

edited by GalRogozinski

Loading

MatheusFranco99 left a comment

alonmuroch commented Sep 26, 2023

y0sher Jan 22, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 22, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 22, 2024

GalRogozinski Jan 25, 2024

GalRogozinski Jan 25, 2024

y0sher Jan 22, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 22, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 22, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 23, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 23, 2024

GalRogozinski Feb 5, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 23, 2024

GalRogozinski Feb 5, 2024

y0sher Jan 23, 2024

GalRogozinski Feb 5, 2024

GalRogozinski Feb 5, 2024

y0sher left a comment

moshe-blox Feb 13, 2024

GalRogozinski Feb 21, 2024

moshe-blox Feb 13, 2024 •

edited

Loading

GalRogozinski Feb 21, 2024

moshe-blox Feb 13, 2024 •

edited

Loading

GalRogozinski Feb 21, 2024

	func (ps PartialSignatureContainer) SignatureForRoot(root [32]byte) map[types.OperatorID][]byte {
	func (ps PartialSignatureContainer) SignaturesForRoot(root [32]byte) map[types.OperatorID][]byte {

-	sort.Slice(ret, func(i, j int) bool {
+	ret := make([]*types.SignedPartialSignatureMessage, len(ps))
+	for i, m := range ps {
+		ret[i] = m
+	}

Pre consensus fix #240

Are you sure you want to change the base?

Pre consensus fix #240

Conversation

alonmuroch commented May 16, 2023 • edited by GalRogozinski Loading

MatheusFranco99 left a comment

Choose a reason for hiding this comment

alonmuroch commented Sep 26, 2023

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

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

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

y0sher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe-blox Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe-blox Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonmuroch commented May 16, 2023 •

edited by GalRogozinski

Loading

moshe-blox Feb 13, 2024 •

edited

Loading

moshe-blox Feb 13, 2024 •

edited

Loading