Partial Signature Verification Aggregation #369

MatheusFranco99 · 2024-03-21T11:15:30Z

Overview

This PR implements the reduction of signatures optimization for partial signature messages as described in this SIP.

Changes

Drop SignedPartialSignatureMessage's signature verification.
Stop verifying each beacon signature in favour of verifying the quorum of signatures through the reconstructed signature.
Add fall back function in case the reconstructed signature is wrong.
In case of duplicated different signatures (same signer), verify both and keep the correct one (if any).

Changed tests

Unknown signer: error message
Invalid beacon signature: add a quorum of messages to detect the invalid signature
Invalid signature: drop expected error regarding SignedPartialSignatureMessage's signature

New tests

Invalid quorum then valid quorum: produces an invalid quorum and then sends a fourth valid message to get a valid quorum

Improvement

New total duty time per number of consensus rounds.

1 Round	2 Rounds	3 Rounds
50%	80%	88%

New partial signature quorum processing time: 17%.

GalRogozinski · 2024-03-25T09:54:53Z

ssv/runner_signatures.go

+	// Check if signer is in committee
+	signerInCommittee := false
+	for _, operator := range b.Share.Committee {
+		if operator.OperatorID == signedMsg.Signer {
+			signerInCommittee = true
+			break
 		}
 	}
+	if !signerInCommittee {
+		return errors.New("unknown signer")
+	}


hmmm

This is currently checked by MsgValidation ERR_PSIG_INVALID_SIG_ID?
Since MsgValidation isn't part of spec it is fine that you added this..

But now I made up my mind that MsgValidation should be an inseperable part of spec.
Once we add tests for this, we won't have to do those double checks

I agree that message validation should belong to the spec

GalRogozinski · 2024-03-25T10:30:51Z

types/messages_encoding.go

+// MarshalSSZ ssz marshals the SignedSSVMessage object
+func (s *SignedSSVMessage) MarshalSSZ() ([]byte, error) {
+	return ssz.MarshalSSZ(s)
+}
+
+// MarshalSSZTo ssz marshals the SignedSSVMessage object to a target array
+func (s *SignedSSVMessage) MarshalSSZTo(buf []byte) (dst []byte, err error) {


ok...
So we need to decide how to do this...

In #345 we added the SignedSSVMessage but we merged it to the fork branch because of this very encoding change...
I made this decision because I decided that the ssz encoding is anyhow the correct (consistent) way to go and I didn't take into account that this PR would need this.

I think we should rework #345 to remove the ssz encoding and add flat encoding so no fork

Removed SignedSSVMessage from this PR.

GalRogozinski · 2024-03-25T10:32:38Z

ssv/spectest/tests/runner/postconsensus/invalid_msg_sig.go

@@ -9,10 +9,9 @@ import (
 	"github.com/bloxapp/ssv-spec/types/testingutils"
 )

-// InvalidMessageSignature tests PartialSignatureMessage signature invalid
+// InvalidMessageSignature tests PartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage.Signature is no longer checked


Suggested change

// InvalidMessageSignature tests PartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage.Signature is no longer checked

// InvalidMessageSignature tests PartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage. Signature is no longer checked. Wait for quorum before verifying.

The "no space" is intended. I wanted it to be: "SignedPartialSignatureMessage. Signature"

GalRogozinski · 2024-03-25T10:35:48Z

ssv/spectest/tests/runner/postconsensus/invalid_quorum_then_valid_quorum_sc.go

+
+// invalidThenQuorumSyncCommitteeContributionSC returns state comparison object for the invalid then quorum SyncCommitteeContribution versioned spec test
+// runner should finish since quorum is achieved
+func invalidQuorumThenValidQuorumSyncCommitteeContributionSC() *comparable.StateComparison {


Just a friendly reminder that SC files were replaced by generated jsons.
However if you think this is a core test (looks like one) and that coding out the SC helps in verifying then this is great!!

GalRogozinski · 2024-03-25T10:38:11Z

ssv/spectest/tests/runner/postconsensus/invalid_then_quorum.go

@@ -14,7 +14,6 @@ import (
 // InvalidThenQuorum  tests a runner receiving an invalid message then a valid quorum, terminating successfully
 func InvalidThenQuorum() tests.SpecTest {


What's the difference between this and InvalidQuorumThenQuorum?

InvalidQuorumThenQuorum: tests an initial invalid quorum and, then, sends valid messages until a valid quorum is reached.

InvalidThenQuorum: test an invalid partial signature message being received and then, later, a valid quorum is reached.

GalRogozinski · 2024-03-25T10:39:23Z

ssv/spectest/tests/runner/preconsensus/invalid_msg_sig.go

@@ -8,7 +8,7 @@ import (
 	"github.com/bloxapp/ssv-spec/types/testingutils"
 )

-// InvalidMessageSignature tests SignedPartialSignatureMessage signature invalid
+// InvalidMessageSignature tests SignedPartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage.Signature is no longer checked


Suggested change

// InvalidMessageSignature tests SignedPartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage.Signature is no longer checked

// InvalidMessageSignature tests SignedPartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage. Signature is no longer checked.

// We wait for quorum to verify sig.

The "no space" is intended. I wanted it to be: "SignedPartialSignatureMessage. Signature"

GalRogozinski · 2024-03-25T10:41:16Z

ssv/spectest/tests/runner/preconsensus/invalid_then_quorum.go

@@ -32,7 +32,6 @@ func InvalidThenQuorum() tests.SpecTest {
 				OutputMessages: []*types.SignedPartialSignatureMessage{
 					testingutils.PreConsensusContributionProofMsg(ks.Shares[1], ks.Shares[1], 1, 1), // broadcasts when starting a new duty
 				},
-				ExpectedError: "failed processing sync committee selection proof message: invalid pre-consensus message: could not verify Beacon partial Signature: wrong signature",


shouldn't the error happen on the first quorum?

The first invalid signature is overwritten by the correct one before a quorum is reached. Once the quorum is reached, all signatures stored are correct and no error is created

GalRogozinski

What about QBFT messages?

MatheusFranco99 · 2024-03-25T14:29:22Z

@GalRogozinski This PR is for partial signature messages

GalRogozinski

Looks good. We also need the SignedSSVMessage with no encoding change. In a different PR

y0sher · 2024-03-28T13:10:15Z

ssv/runner_validations.go

+	signatures := container.GetSignatures(root)
+
+	for operatorID, signature := range signatures {
+		if err := b.verifyBeaconPartialSignature(operatorID, signature, root); err != nil {
+			container.Remove(operatorID, root)
+		}
+	}


shouldn't we do another check here? or after calling FallBackAndVerifyEachSignature, what if we had a quorum of 4, then we realized one is invalid, don't we want to reconstruct again with the 3 left?
or this is not possible since its a must we'll try first when have 3?

I guess nothing else to do here regarding notifying the runner we no longer have quorum? since the quorum is calculated on the fly using the container?

we must have 3

yes

MatheusFranco99 added 30 commits January 10, 2024 11:11

Add SignedSSVMessage

3ea4b69

Add SSZ encoding

3aebee8

Test structures for SignedSSVMessage

0fc89ba

Test utils and encoding test

f919fda

Tests

5d151fb

Generate tests

7965ba8

Adapt message validation

1c3ace2

Fix expected error; Bring back commented tests

e67c4b5

Add comment on signature

e2e279b

Merge branch 'main' into signed-ssv-msg

43da24c

Adjust ssz-max in SignedSSVMessage

9b1750d

Include RSA check in test

c74a5c1

Generate JSONs

b9469eb

Optimistic approach in pre-consensus messages

399dd77

Extend optimistic approach to pre-consensus

86ac7df

Extend to post-consensus

21f4d47

Add approach to validator reg. and exit duties

97765fd

Add errors

f229766

Rename fall back function

6da8d6d

Check duplicated signature case

1f8b1b0

Add in-committee check

15f5e31

Fix unknown signer error messages

a9d1580

Drop expected error in invalid signature since it's deprecated

a7d3d15

Change invalid beacon sig test to trigger error once quorum is reached

cfa6c21

Drop errors in invalid then quorum test

b45fd73

GenerateTests

836a06b

Fall back and verify for all roots

ad95766

Test: Invalid quorum then valid quorum

464b763

Test: invalid quorum then valid quorum for pre-consensus

aaa0b28

Fix name

603a7ab

Generate tests

fdcf38c

MatheusFranco99 requested a review from GalRogozinski March 21, 2024 11:15

MatheusFranco99 self-assigned this Mar 21, 2024

Fix lint

db72966

GalRogozinski reviewed Mar 25, 2024

View reviewed changes

GalRogozinski requested changes Mar 25, 2024

View reviewed changes

MatheusFranco99 added 2 commits March 25, 2024 14:17

remove commits from signed ssv message branch

03fa670

Fix revert issues. Generate tests

6f407fb

GalRogozinski approved these changes Mar 27, 2024

View reviewed changes

Simplify if clause

5e592ba

MatheusFranco99 changed the title ~~Reduce signatures in partial signatures steps - No fork~~ Partial Signature Verification Aggregation Mar 27, 2024

nkryuchkov mentioned this pull request Mar 27, 2024

Partial Signature Verification Aggregation ssvlabs/ssv#1351

Merged

moshe-blox approved these changes Mar 27, 2024

View reviewed changes

liorrutenberg approved these changes Mar 27, 2024

View reviewed changes

liorrutenberg requested a review from alonmuroch March 27, 2024 13:34

y0sher reviewed Mar 28, 2024

View reviewed changes

Merge branch 'main' into reduce_signatures_partial_sig

96807d4

GalRogozinski merged commit ebb6014 into main Mar 28, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial Signature Verification Aggregation #369

Partial Signature Verification Aggregation #369

MatheusFranco99 commented Mar 21, 2024 •

edited

Loading

GalRogozinski Mar 25, 2024

MatheusFranco99 Mar 25, 2024

GalRogozinski Mar 25, 2024

MatheusFranco99 Mar 25, 2024

GalRogozinski Mar 25, 2024

MatheusFranco99 Mar 25, 2024

GalRogozinski Mar 25, 2024

GalRogozinski Mar 25, 2024

MatheusFranco99 Mar 25, 2024

GalRogozinski Mar 25, 2024

MatheusFranco99 Mar 25, 2024

GalRogozinski Mar 25, 2024

MatheusFranco99 Mar 25, 2024

GalRogozinski left a comment

MatheusFranco99 commented Mar 25, 2024

GalRogozinski left a comment •

edited

Loading

y0sher Mar 28, 2024 •

edited

Loading

GalRogozinski Mar 28, 2024

	// InvalidMessageSignature tests PartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage.Signature is no longer checked
	// InvalidMessageSignature tests PartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage. Signature is no longer checked. Wait for quorum before verifying.

		@@ -14,7 +14,6 @@ import (
		// InvalidThenQuorum tests a runner receiving an invalid message then a valid quorum, terminating successfully
		func InvalidThenQuorum() tests.SpecTest {

	// InvalidMessageSignature tests SignedPartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage.Signature is no longer checked
	// InvalidMessageSignature tests SignedPartialSignatureMessage signature invalid. No error is generated since the SignedPartialSignatureMessage. Signature is no longer checked.
	// We wait for quorum to verify sig.

Partial Signature Verification Aggregation #369

Partial Signature Verification Aggregation #369

Conversation

MatheusFranco99 commented Mar 21, 2024 • edited Loading

Overview

Changes

Changed tests

New tests

Improvement

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

GalRogozinski left a comment

Choose a reason for hiding this comment

MatheusFranco99 commented Mar 25, 2024

GalRogozinski left a comment • edited Loading

Choose a reason for hiding this comment

y0sher Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatheusFranco99 commented Mar 21, 2024 •

edited

Loading

GalRogozinski left a comment •

edited

Loading

y0sher Mar 28, 2024 •

edited

Loading