Identify invalid signature within batch verification (#11582) #11741

dyng · 2022-12-08T07:53:03Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

The signatures of newly received blocks are collected and verified as a batch, while it's perfect if everything is ok, we can't tell which signature is invalid if batch verification fails.

Which issues(s) does this PR fix?

Fixes #11582

Other notes for review

This PR is not yet complete because I need to discuss with reviewer to determine where VerifyVerbosely is supposed to be used.

…1582)

dyng · 2022-12-11T14:15:35Z

@terencechain could you have some time to review this PR? I'm not sure if VerifyVerbose is a right approach to original issue. If so, I'll then replace Verify to VerifyVerbosely in ExecuteStateTransition and onBlockBatch. Thanks.

terencechain · 2022-12-12T02:08:30Z

beacon-chain/core/blocks/signature.go

@@ -216,11 +218,14 @@ func createAttestationSignatureBatch(
 			return nil, errors.Wrap(err, "could not get signing root of object")
 		}
 		msgs[i] = root
+
+		descs[i] = "attestation signature"


Instead of hard-coding these descriptions, maybe we can use enums

It looks better, thanks!

terencechain · 2022-12-12T02:10:18Z

@terencechain could you have some time to review this PR? I'm not sure if VerifyVerbose is a right approach to original issue. If so, I'll then replace Verify to VerifyVerbosely in ExecuteStateTransition and onBlockBatch. Thanks.

I dont think VerifyVerbosely should be the default but rather a feature flag --verify-sig-verbose or something similar

nisdas · 2022-12-12T03:41:48Z

crypto/bls/signature_batch.go

@@ -25,6 +32,11 @@ func (s *SignatureBatch) Join(set *SignatureBatch) *SignatureBatch {
 	s.Signatures = append(s.Signatures, set.Signatures...)
 	s.PublicKeys = append(s.PublicKeys, set.PublicKeys...)
 	s.Messages = append(s.Messages, set.Messages...)
+	if s.Descriptions != nil && set.Descriptions != nil {


Remove this nil check, since all signature sets now have a description attached to it.

Because SignatureBatch is such a fundamental struct so I hesitated to make a big change, then ended up a compromised solution that SignatureBatch can have either full description list or nil. But I think your comment is absolutely correct, it should be far more elegant if every signature has a description attached. I'll make the change as you commented. Thanks for your advice!

nisdas · 2022-12-12T03:43:58Z

crypto/bls/signature_batch.go

+			if err != nil {
+				_, e := fmt.Fprintf(&sb, "signature '%s' is invalid."+
+					" signature: %v, public key: %v, message: %v, error: %v\n", desc, sig, pubKey, msg, err)
+				if e != nil {


don't ignore errors here, also why are we using Fprintf here ? Wrap the error and then return it.

Here I used strings.Builder for better performance (not a big problem though). strings.Builder follows StringWriter interface that emits error but actually always returns nil as error, so I decided to ignore it. I'll consider to use string concatenation instead.

nisdas · 2022-12-12T03:46:25Z

crypto/bls/signature_batch.go

+		valid, err := VerifySignature(sig, msg, pubKey)
+		if !valid {
+			var desc string
+			if len(s.Descriptions) != len(s.Signatures) {


These checks are going to be ugly and a source of bugs, it would be much better to always have the same number of descriptions and signatures in a set.

nisdas · 2022-12-12T03:47:03Z

beacon-chain/core/blocks/signature.go

 	}, nil
 }

 // verifies the signature from the raw data, public key and domain provided.
 func verifySignature(signedData, pub, signature, domain []byte) error {
-	set, err := signatureBatch(signedData, pub, signature, domain)
+	set, err := signatureBatch(signedData, pub, signature, domain, "")


better to name it as "unknown" rather than an empty string

As @terencechain's comment, I'll use enums instead. Thanks!

nisdas · 2022-12-12T03:50:18Z

crypto/bls/signature_batch.go

+				}
+			} else {
+				_, e := fmt.Fprintf(&sb, "signature '%s' is invalid."+
+					" signature: %v, public key: %v, message: %v\n", desc, sig, pubKey, msg)


Better to represent msg as hex rather than an array of bytes when printing the error message. Same applies to signature and public keys. Also you need to marshal the public key into bytes otherwise you end up printing pointers to the struct which isn't useful

Thanks! I'll make error message more readable.

nisdas · 2022-12-12T03:52:06Z

crypto/bls/signature_batch.go

@@ -82,6 +145,10 @@ func (s *SignatureBatch) RemoveDuplicates() (int, *SignatureBatch, error) {
 	sigs := s.Signatures[:0]
 	pubs := s.PublicKeys[:0]
 	msgs := s.Messages[:0]
+	var descs []string
+	if s.Descriptions != nil {


same thing here, do not have a nil check here. The correct thing to do would be to make sure all signature sets always have an attached description.

dyng · 2022-12-13T05:31:03Z

I have done the changes, please take a review when you have time. Thanks! @nisdas @terencechain

nisdas · 2022-12-15T03:51:37Z

crypto/bls/constants.go

@@ -5,3 +5,31 @@ const DomainByteLength = 4

 // CurveOrder for the BLS12-381 curve.
 const CurveOrder = "52435875175126190479447740508185965837690552500527637822603658699938581184513"
+
+// List of descriptions for different kinds of signatures
+const (


This is the wrong place to declare these descriptions. The bls library should have no knowledge of what is an application construct( bls change, randao, etc)

Thanks for your comment! I have moved these constants to signing package, but left AggregatedSignature in bls package as it is a local knowledge belongs to bls package.

nisdas · 2022-12-16T10:03:48Z

crypto/bls/signature_batch.go

@@ -140,6 +195,7 @@ func (s *SignatureBatch) AggregateBatch() (*SignatureBatch, error) {
 			b.PublicKeys = []PublicKey{aggPub}
 			b.Signatures = [][]byte{aggSig.Marshal()}
 			b.Messages = [][32]byte{copiedRt}
+			b.Descriptions = []string{AggregatedSignature}


one worry is that you get information loss with this, since you lose the ability to determine which type of signature was invalid. Its instead just a generic AggregatedSignature Usually messages with the same root, will reference the same types of messages(attestations,sync committee,etc) . But we can maybe fix this in a follow up PR.

I thought about the information loss, but still used a generic description here for two reason:

the signatures themself are aggregated and we can hardly identify invalid individual signature.

AggregateBatch() is called in validateWithBatchVerifier() but not VerifyVerbosely(), currently only VerifyVerbosely() is responsible to nail down failed signature.

But I'm not sure if I have fully understood the whole picture of block processing (hence original issue #11582), correct me if I am wrong! Thanks! Maybe as you said, we can fix this later.

nisdas · 2022-12-16T10:51:45Z

crypto/bls/signature_batch.go

@@ -119,12 +172,14 @@ func (s *SignatureBatch) AggregateBatch() (*SignatureBatch, error) {
 			currBatch.Signatures = append(currBatch.Signatures, s.Signatures[i])
 			currBatch.Messages = append(currBatch.Messages, s.Messages[i])
 			currBatch.PublicKeys = append(currBatch.PublicKeys, s.PublicKeys[i])
+			currBatch.Descriptions = append(currBatch.Descriptions, s.Descriptions[i])


we should also do a length check for number of descriptions, otherwise this will panic

I have added length validation for descriptions. Thanks for the comment!

I reordered the empty check and length check because I think length mismatch of signatures and messages should always be an error even in case that signatures are empty and messages are non-empty.

nisdas

Thanks for addressing all the comments, great work on this feature !

nisdas · 2022-12-19T09:00:29Z

Build is failing @dyng

crypto/bls/signature_batch_test.go:657:9: Unhandled error for function call  (errcheck)
crypto/bls/signature_batch_test.go:681:9: Unhandled error for function call  (errcheck)

dyng · 2022-12-20T09:42:50Z

@nisdas I have fixed build failure, sorry for that (I should build locally at first). Thanks for your review and precious comments!

Identify invalid signature within batch verification (prysmaticlabs#1…

238069d

…1582)

dyng requested a review from a team as a code owner December 8, 2022 07:53

dyng requested review from potuz, terencechain and symbolpunk December 8, 2022 07:53

dyng added 3 commits December 11, 2022 21:28

Fix issues found by linter

50c96a1

Merge branch 'develop' into feature-individual-signature-verification

684039a

Make deepsource happy

0a5badb

terencechain reviewed Dec 12, 2022

View reviewed changes

nisdas requested changes Dec 12, 2022

View reviewed changes

dyng added 4 commits December 12, 2022 15:58

Add signature description enums

e1a14eb

Make descriptions a mandatory field

500a850

More readable error message of invalid signatures

9c0001a

Add 'enable-verbose-sig-verification' option

7f66dfe

dyng changed the title ~~[WIP] Identify invalid signature within batch verification (#11582)~~ Identify invalid signature within batch verification (#11582) Dec 13, 2022

Fix format

fb62cdd

dyng force-pushed the feature-individual-signature-verification branch from 1295f6e to fb62cdd Compare December 13, 2022 09:20

Merge branch 'develop' into feature-individual-signature-verification

623964a

nisdas reviewed Dec 15, 2022

View reviewed changes

Move descriptions to package signing

1651d49

nisdas reviewed Dec 16, 2022

View reviewed changes

nisdas and others added 2 commits December 16, 2022 19:17

Merge branch 'develop' into feature-individual-signature-verification

1ce3a78

Add more validation and test cases

6d50cb3

nisdas previously approved these changes Dec 19, 2022

View reviewed changes

Merge branch 'develop' into feature-individual-signature-verification

e921882

Fix build failure

fdab347

dyng dismissed nisdas’s stale review via fdab347 December 20, 2022 09:33

Merge branch 'develop' into feature-individual-signature-verification

a4e8d60

nisdas approved these changes Dec 20, 2022

View reviewed changes

nisdas merged commit e431521 into prysmaticlabs:develop Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identify invalid signature within batch verification (#11582) #11741

Identify invalid signature within batch verification (#11582) #11741

dyng commented Dec 8, 2022

dyng commented Dec 11, 2022

terencechain Dec 12, 2022

dyng Dec 12, 2022

terencechain commented Dec 12, 2022

nisdas Dec 12, 2022

dyng Dec 12, 2022

nisdas Dec 12, 2022

dyng Dec 12, 2022

nisdas Dec 12, 2022

nisdas Dec 12, 2022

dyng Dec 12, 2022

nisdas Dec 12, 2022

dyng Dec 12, 2022

nisdas Dec 12, 2022

dyng commented Dec 13, 2022

nisdas Dec 15, 2022

dyng Dec 15, 2022

nisdas Dec 16, 2022

dyng Dec 16, 2022 •

edited

Loading

nisdas Dec 16, 2022

dyng Dec 16, 2022

nisdas left a comment

nisdas commented Dec 19, 2022

dyng commented Dec 20, 2022

Identify invalid signature within batch verification (#11582) #11741

Identify invalid signature within batch verification (#11582) #11741

Conversation

dyng commented Dec 8, 2022

dyng commented Dec 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Dec 12, 2022

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

dyng commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyng Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

nisdas commented Dec 19, 2022

dyng commented Dec 20, 2022

dyng Dec 16, 2022 •

edited

Loading