Replace redundant BLS in QBFT with RSA #372

MatheusFranco99 · 2024-03-27T11:03:59Z

Overview

This PR adds an RSA check in the Validator.ProcessMessage function and drops redundant BLS signature checks in QBFT.

Changes

QBFT: Drop redundant checks for Proposal, Prepare, Commit and Round-Change messages
QBFT: Keep checks for message justification and decided messages
QBFT: Add a check to ensure that the signers belong to the committee
Operator: Add SSV PublicKey
Validator: Change the ProcessMessage function signature to receive SignedSSVMessage instead of SSVMessage and add an RSA signature check

Improvement

New total duty time per number of consensus rounds.

1 Round	2 Rounds	3 Rounds
67%	70%	71%

New consensus n-th round time.

1st Round	2nd Round	3rd Round
21%	73%	73%

GalRogozinski · 2024-03-27T11:51:41Z

qbft/commit.go

+	if !signedCommit.CheckSignersInCommittee(operators) {
+		return errors.New("signers not in committee")
+	}


nice! just wondering if MsgValidation also takes care of it?

I think it does. But I'm leaving it here because spec doesn't know and it's a light check

I think we should NEVER assume something happens in msg validation hence we don't need to take care in consensus.

GalRogozinski · 2024-03-27T12:38:21Z

qbft/prepare.go

+	if !signedPrepare.CheckSignersInCommittee(operators) {
+		return errors.New("signers not in committee")
+	}


just from sw engineering point of view
shouldn't we have some baseValidation that keeps on calling this

so we never forget to add this?
because it repeats itself

I think we should totally do that. Not only for this check but for many others such as height, msg.Validate(), etc. Since it would require some bigger refactoring, let's do it in a future PR? Just so we can dispatch this to implementation as soon as possible and as simple as possible.

GalRogozinski

Approved but you can still fix my comment

moshe-blox · 2024-03-27T13:17:59Z

qbft/round_change.go

@@ -213,6 +214,7 @@ func validRoundChangeForData(
 	height Height,
 	round Round,
 	fullData []byte,
+	verifySignature bool,


switch to dontVerifySignature?

on another thought, maybe we should go for the validateX() and validateXNoVerify() pattern?

moshe-blox · 2024-03-27T13:28:27Z

qbft/commit.go

@@ -127,7 +135,7 @@ func validateCommit(
 	proposedMsg *SignedMessage,
 	operators []*types.Operator,
 ) error {
-	if err := baseCommitValidation(config, signedCommit, height, operators); err != nil {
+	if err := baseCommitValidation(config, signedCommit, height, operators, false); err != nil {


if we go for the NoVerify suffix pattern, validateCommit could be validateCommitNoVerify to be explicit

moshe-blox

Let's merge this after #345 is on main

olegshmuelov · 2024-03-27T15:01:21Z

qbft/commit.go

@@ -94,7 +95,7 @@ func CreateCommit(state *State, config IConfig, root [32]byte) (*SignedMessage,
 	return signedMsg, nil
 }

-func baseCommitValidation(
+func baseCommitValidationNoVerification(


the config is not used here anymore, should we delete it?

Indeed. I'll remove it. Thanks

I just wonder if other readers know that "verification" we mean signatures...
I barely know this 😅

Maybe "IgnoreSignature" is better?

I decided to finally put an end to my questioning of the usages between the 2 terms and googled this:
https://www.connect2id.com/blog/validate-vs-verify-in-the-context-of-cryptography

TLDR: "verify" is the correct word in term of cryptography but "the Internet security community sometimes uses these two terms inconsistently"...

So I think "IgnoreSignature" is better because it won't confuse anyone

Yeah. "IgnoreSignature" is more clear. I'll update it. Thanks!

olegshmuelov · 2024-03-27T15:04:19Z

qbft/prepare.go

@@ -79,7 +80,7 @@ func getRoundChangeJustification(state *State, config IConfig, prepareMsgContain

 // validSignedPrepareForHeightRoundAndRoot known in dafny spec as validSignedPrepareForHeightRoundAndDigest
 // https://entethalliance.github.io/client-spec/qbft_spec.html#dfn-qbftspecification
-func validSignedPrepareForHeightRoundAndRoot(
+func validSignedPrepareForHeightRoundAndRootNoVerification(


the config is not used here anymore, should we delete it?

Indeed. I'll remove it. Thanks

olegshmuelov · 2024-03-27T15:10:11Z

qbft/round_change.go

+	if err := signedMsg.Validate(); err != nil {
+		return errors.Wrap(err, "roundChange invalid")


just to understand, it was a bug that we validated only the inner message?

if so, we have to add a test for message signers is empty, non unique signer & signer ID 0 not allowed

Yup, adding these tests.
Btw, the previous version couldn't produce any errors because, even though it should be signedMsg.Validate() and not signedMsg.Message.Validate(), the other checks were also verifying the conditions. But, of course, still better to call signedMsg.Validate()

It wasn't a bug that we only called the inner check..
It is actually a duplicate call as a shrewed dev once noticed in #326

It is not a bug, but all other base validation functions for the other message types call validate on the upper message, and this is the only inconsistent one. Even though, all of these calls are indeed duplicated 😅

olegshmuelov · 2024-03-28T08:34:41Z

qbft/commit.go

@@ -127,7 +145,7 @@ func validateCommit(
 	proposedMsg *SignedMessage,
 	operators []*types.Operator,
 ) error {
-	if err := baseCommitValidation(config, signedCommit, height, operators); err != nil {
+	if err := baseCommitValidationNoVerification(signedCommit, height, operators); err != nil {


one more cleanup can be done here
to not pass the config to validateCommit

olegshmuelov · 2024-03-28T08:39:58Z

since #342 was merged already

can we fix small issue in the current pr?

GalRogozinski · 2024-03-28T10:42:50Z

since #342 was merged already

can we fix small issue in the current pr?

This PR doesn't change runner at all...
Maybe in a subsequent PR when we change runner

olegshmuelov · 2024-03-28T11:11:24Z

since #342 was merged already
can we fix small issue in the current pr?

This PR doesn't change runner at all... Maybe in a subsequent PR when we change runner

created an issue
#376

GalRogozinski · 2024-04-04T15:06:10Z

types/testingutils/operator_signer.go

@@ -16,10 +17,14 @@ type testingOperatorSigner struct {

 var (
 	testingOperatorSignerInstance *testingOperatorSigner
+	opMu                          sync.Mutex


The lack of it randomly produces errors around tests

do we have many different instances with different keys?

if not maybe singleton is better?

This is the singleton

GalRogozinski · 2024-04-04T20:20:47Z

qbft/state.go

 }

 // GetSigner returns a Signer instance
-func (c *Config) GetSigner() types.SSVSigner {
+func (c *Config) GetSigner() types.SSVShareSigner {


nit:
you can also rename the function

Yes, better. Thanks

GalRogozinski · 2024-04-04T21:17:24Z

ssv/attester.go

+func (r *AttesterRunner) GetOperatorSigner() types.SSVOperatorSigner {
+	return r.operatorSigner
+}
+


does runner use the signer?
in the end we use the network to sign it

Indeed. Removing it. Thanks!

moshe-blox · 2024-04-07T13:04:50Z

qbft/state.go

@@ -31,7 +31,7 @@ type IConfig interface {
 }

 type Config struct {
-	Signer         types.SSVShareSigner
+	SSVShareSigner types.SSVShareSigner


this should probably be just ShareSigner like below, and the same for method names

tbh im not sure that we even need the word 'SSV' in the structs, but it's not too bad, lmk what you think @GalRogozinski @MatheusFranco99

I agree the word SSV here is redundant. I'll remove it, thx

y0sher · 2024-04-07T13:44:33Z

qbft/round_change.go

+	return nil
+}
+
+func validRoundChangeForDataWithVerification(


I don't want to nit pick on the naming, but WithVerification and IgnoreSignature don't sound related to each other when and fact the functionality should be identical with one difference.
I would go with either WithVerification and NoVerification
or IgnoreSignature and WithSignature/VerifySignature
but leaving it like now is a bit confusing imo

Definitely. Forgot about that. Thanks!

ssv/aggregator.go

ssv/validator.go

y0sher · 2024-04-07T14:13:05Z

types/signer.go

+	// AddSSVOperatorKey saves an SSV operator key
+	AddSSVOperatorKey(sk *rsa.PrivateKey) error
+	// RemoveSSVOperatorKey removes an SSV operator key
+	RemoveSSVOperatorKey(pubKey string)


These (Add/Remove) methods are redundant, don't make us satisfy this interface (😆); There is always just a single operator key per instance, so there's never a need to add and remove them.

types/signer.go

y0sher

some small stuff.

moshe-blox · 2024-04-09T09:50:06Z

ssv/validator.go

+	// Validate message
+	if err := signedSSVMessage.Validate(); err != nil {
+		return errors.Wrap(err, "invalid SignedSSVMessage")
+	}
+
+	// Decode the nested SSVMessage
+	msg := &types.SSVMessage{}
+	if err := msg.Decode(signedSSVMessage.Data); err != nil {
+		return errors.Wrap(err, "could not decode data into an SSVMessage")
+	}
+
+	// Verify SignedSSVMessage's signature
+	if err := v.SignatureVerifier.Verify(signedSSVMessage, v.Share.Committee); err != nil {
+		return errors.Wrap(err, "SignedSSVMessage has an invalid signature")
+	}


we should probably decode after verifying

Agree. Thanks!

moshe-blox · 2024-04-09T09:50:48Z

types/messages.go

@@ -191,7 +191,7 @@ func (msg *SignedSSVMessage) Decode(data []byte) error {
 // - Data length should not be 0
 func (msg *SignedSSVMessage) Validate() error {
 	if msg.OperatorID == 0 {
-		return errors.New("OperatorID in SignedSSVMessage is 0")
+		return errors.New("signer ID 0 not allowed")
 	}
 	if len(msg.Signature) == 0 {


is there a reason to not require the expected length of RSA signatures, or is that not the right place to do so, or not the right assumption for the spec to make?

Actually, in the beginning, I wanted to let it open. But I've also added the custom decoding function to be used by implementation that sets it to 256 bytes

signatureSize = 256 signature := encoded[signatureOffset : signatureOffset+signatureSize]

I have no opinion right now on whether we should check it here or leave the current check. 🤔

@GalRogozinski any reason why we shouldn't check that len(sig) == signatureSize in spec?

When you decode the data from pubsub you create a byte array of the expected length
maybe change SignedMessage to have Signature [256]byte

btw @MatheusFranco99 I remember we said the encoded length of the sig we use is actually greater correct?

Ok, I'll change it to 256 bytes
This was about an encoded key. For the signature, it's indeed 256

GalRogozinski

just final nits and that's it

GalRogozinski · 2024-04-09T12:07:41Z

types/signer.go

 type KeyManager interface {
 	BeaconSigner
-	SSVSigner
+	ShareSigner
 	// AddShare saves a share key
 	AddShare(shareKey *bls.SecretKey) error
 	// RemoveShare removes a share key


heads up for KeyManager...
I think it should be removed either in this PR or the next one (imo do it the next one so we can close this one quickly)

there can still be a KeyManager struct in tests or impl that simply implements them both

Yes, other PR

GalRogozinski · 2024-04-09T12:27:49Z

types/testingutils/operator_signer.go

@@ -16,10 +17,14 @@ type testingOperatorSigner struct {

 var (
 	testingOperatorSignerInstance *testingOperatorSigner
+	opMu                          sync.Mutex


do we have many different instances with different keys?

if not maybe singleton is better?

GalRogozinski · 2024-04-09T13:00:24Z

p2p/validation/msg_validation.go

-}
-
-func MsgValidation(runner ssv.Runner, msgValidator MessageValidator) MsgValidatorFunc {
+func MsgValidation(runner ssv.Runner) MsgValidatorFunc {


do you see value to keep this file here for now?
if no, you may delete it

doesn't have to be in this PR and not a blocker

GalRogozinski · 2024-04-09T13:05:34Z

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

+)
+
+// InconsistentNetworkSigner tests a SignedSSVMessage with inconsistent signer regarding its nested SignedPartialSignatureMessage
+func InconsistentNetworkSigner() tests.SpecTest {


change network to operator on all tests I think

y0sher · 2024-04-09T14:11:39Z

Q: are we signing rsa in consensus ? If we are verifying the we should probably also sign.

GalRogozinski · 2024-04-09T14:12:56Z

Q: are we signing rsa in consensus ? If we are verifying the we should probably also sign.

In spec we sign in validator.go
In impl from my POV you should sign where you pass all spec tests

MatheusFranco99 added 4 commits March 27, 2024 10:15

Drop BLS check where redundant

82f69a7

Check that the message signers belong to the instance committee

2910f43

Add signer in committee check for proposal

024df4e

Fix tests error msgs. Drop old tests

2a47333

MatheusFranco99 requested review from y0sher, GalRogozinski and olegshmuelov March 27, 2024 11:03

MatheusFranco99 self-assigned this Mar 27, 2024

GalRogozinski approved these changes Mar 27, 2024

View reviewed changes

MatheusFranco99 changed the title ~~Reduce signatures - drop redundant BLS in QBFT (no fork)~~ Reduce signatures - drop redundant BLS in QBFT Mar 27, 2024

moshe-blox reviewed Mar 27, 2024

View reviewed changes

moshe-blox approved these changes Mar 27, 2024

View reviewed changes

liorrutenberg requested a review from moshe-blox March 27, 2024 13:32

moshe-blox requested changes Mar 27, 2024

View reviewed changes

Refactor into WithVerification NoVerification functions

cbc6a91

olegshmuelov reviewed Mar 27, 2024

View reviewed changes

MatheusFranco99 added 2 commits March 27, 2024 15:48

Drop unnecessary config arguments

9ad459c

Add msg.Validate() tests for round-change

d772e69

MatheusFranco99 requested a review from olegshmuelov March 27, 2024 15:52

olegshmuelov approved these changes Mar 27, 2024

View reviewed changes

olegshmuelov mentioned this pull request Mar 27, 2024

Reduce Signatures - Drop Redundant BLS In QBFT ssvlabs/ssv#1352

Merged

olegshmuelov reviewed Mar 28, 2024

View reviewed changes

Remove unused argument

43cc9e5

Verify RSA signatures of broadcasted messages

0cc2d57

GalRogozinski reviewed Apr 4, 2024

View reviewed changes

Apply suggestions

6f8ddbe

MatheusFranco99 requested a review from GalRogozinski April 7, 2024 08:17

moshe-blox requested changes Apr 7, 2024

View reviewed changes

y0sher reviewed Apr 7, 2024

View reviewed changes

ssv/aggregator.go Show resolved Hide resolved

y0sher reviewed Apr 7, 2024

View reviewed changes

ssv/validator.go Show resolved Hide resolved

y0sher reviewed Apr 7, 2024

View reviewed changes

types/signer.go Outdated Show resolved Hide resolved

y0sher requested changes Apr 7, 2024

View reviewed changes

Add suggestions

5ee5e36

MatheusFranco99 requested review from y0sher and moshe-blox April 8, 2024 08:15

moshe-blox reviewed Apr 9, 2024

View reviewed changes

MatheusFranco99 added 3 commits April 9, 2024 11:30

Change decode and verify order

699d322

Rename SSVOperatorKeys to OperatorKeys in testing util

75edf5a

Define signature to have 256 bytes

c47c376

y0sher approved these changes Apr 9, 2024

View reviewed changes

GalRogozinski reviewed Apr 9, 2024

View reviewed changes

moshe-blox approved these changes Apr 9, 2024

View reviewed changes

Rename Network to Operator in tests

9d19c18

GalRogozinski approved these changes Apr 9, 2024

View reviewed changes

GalRogozinski merged commit 83d5ab4 into main Apr 9, 2024
2 checks passed

GalRogozinski deleted the reduce_bls_signatures_qbft_no_fork branch April 9, 2024 14:49

This was referenced Apr 10, 2024

Runner Sign RSA Before Broadcast #380

Merged

Pass MessageID To Broadcast #381

Merged

		if err := signedMsg.Validate(); err != nil {
		return errors.Wrap(err, "roundChange invalid")

Replace redundant BLS in QBFT with RSA #372

Replace redundant BLS in QBFT with RSA #372

Conversation

MatheusFranco99 commented Mar 27, 2024 • edited by guym-blox Loading

Overview

Changes

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

GalRogozinski left a 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

moshe-blox left a 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

olegshmuelov commented Mar 28, 2024

GalRogozinski commented Mar 28, 2024

olegshmuelov commented Mar 28, 2024

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 Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y0sher Apr 7, 2024 • edited Loading

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

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

MatheusFranco99 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

GalRogozinski left a 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 commented Apr 9, 2024

GalRogozinski commented Apr 9, 2024 • edited Loading

MatheusFranco99 commented Mar 27, 2024 •

edited by guym-blox

Loading

y0sher Apr 7, 2024 •

edited

Loading

y0sher Apr 7, 2024 •

edited

Loading

MatheusFranco99 Apr 9, 2024 •

edited

Loading

GalRogozinski commented Apr 9, 2024 •

edited

Loading