Don't use max cover on unaggregated atts nor check subgroup of valida…

…ted signatures (#12350) * Don't use max cover on unnaggregated atts * Do not validate signature on the attestation package * separate nil error checks * fix unit tests
prysmaticlabs · May 16, 2023 · be23773 · be23773
1 parent 29f6de1
commit be23773
Show file tree

Hide file tree

Showing 7 changed files with 142 additions and 22 deletions.
diff --git a/beacon-chain/operations/attestations/kv/aggregated.go b/beacon-chain/operations/attestations/kv/aggregated.go
@@ -53,27 +53,25 @@ func (c *AttCaches) aggregateUnaggregatedAttestations(ctx context.Context, unagg
 	// Track the unaggregated attestations that aren't able to aggregate.
 	leftOverUnaggregatedAtt := make(map[[32]byte]bool)
 	for _, atts := range attsByDataRoot {
-		aggregatedAtts := make([]*ethpb.Attestation, 0, len(atts))
-		processedAtts, err := attaggregation.Aggregate(atts)
+		aggregated, err := attaggregation.AggregateDisjointOneBitAtts(atts)
 		if err != nil {
-			return err
+			return errors.Wrap(err, "could not aggregate unaggregated attestations")
 		}
-		for _, att := range processedAtts {
-			if helpers.IsAggregated(att) {
-				aggregatedAtts = append(aggregatedAtts, att)
-			} else {
-				h, err := hashFn(att)
-				if err != nil {
-					return err
-				}
-				leftOverUnaggregatedAtt[h] = true
-			}
+		if aggregated == nil {
+			return errors.New("could not aggregate unaggregated attestations")
 		}
-		if err := c.SaveAggregatedAttestations(aggregatedAtts); err != nil {
-			return err
+		if helpers.IsAggregated(aggregated) {
+			if err := c.SaveAggregatedAttestations([]*ethpb.Attestation{aggregated}); err != nil {
+				return err
+			}
+		} else {
+			h, err := hashFn(aggregated)
+			if err != nil {
+				return err
+			}
+			leftOverUnaggregatedAtt[h] = true
 		}
 	}
-
 	// Remove the unaggregated attestations from the pool that were successfully aggregated.
 	for _, att := range unaggregatedAtts {
 		h, err := hashFn(att)
@@ -87,7 +85,6 @@ func (c *AttCaches) aggregateUnaggregatedAttestations(ctx context.Context, unagg
 			return err
 		}
 	}
-
 	return nil
 }
 

diff --git a/beacon-chain/rpc/eth/beacon/pool_test.go b/beacon-chain/rpc/eth/beacon/pool_test.go
@@ -1183,7 +1183,7 @@ func TestServer_SubmitAttestations_InvalidAttestationGRPCHeader(t *testing.T) {
 	require.Equal(t, true, ok, "could not retrieve custom error metadata value")
 	assert.DeepEqual(
 		t,
-		[]string{"{\"failures\":[{\"index\":0,\"message\":\"Incorrect attestation signature: signature must be 96 bytes\"}]}"},
+		[]string{"{\"failures\":[{\"index\":0,\"message\":\"Incorrect attestation signature: could not create signature from byte slice: signature must be 96 bytes\"}]}"},
 		v,
 	)
 }

diff --git a/crypto/bls/bls.go b/crypto/bls/bls.go
@@ -24,6 +24,13 @@ func PublicKeyFromBytes(pubKey []byte) (PublicKey, error) {
 	return blst.PublicKeyFromBytes(pubKey)
 }
 
+// SignatureFromBytesNoValidation creates a BLS signature from a LittleEndian byte slice.
+// It does not check validity of the signature, use only when the byte slice has
+// already been verified
+func SignatureFromBytesNoValidation(sig []byte) (Signature, error) {
+	return blst.SignatureFromBytesNoValidation(sig)
+}
+
 // SignatureFromBytes creates a BLS signature from a LittleEndian byte slice.
 func SignatureFromBytes(sig []byte) (Signature, error) {
 	return blst.SignatureFromBytes(sig)

diff --git a/crypto/bls/blst/signature.go b/crypto/bls/blst/signature.go
@@ -24,15 +24,35 @@ type Signature struct {
 	s *blstSignature
 }
 
-// SignatureFromBytes creates a BLS signature from a LittleEndian byte slice.
-func SignatureFromBytes(sig []byte) (common.Signature, error) {
+// signatureFromBytesNoValidation creates a BLS signature from a LittleEndian
+// byte slice. It does not validate that the signature is in the BLS group
+func signatureFromBytesNoValidation(sig []byte) (*blstSignature, error) {
 	if len(sig) != fieldparams.BLSSignatureLength {
 		return nil, fmt.Errorf("signature must be %d bytes", fieldparams.BLSSignatureLength)
 	}
 	signature := new(blstSignature).Uncompress(sig)
 	if signature == nil {
 		return nil, errors.New("could not unmarshal bytes into signature")
 	}
+	return signature, nil
+}
+
+// SignatureFromBytesNoValidation creates a BLS signature from a LittleEndian
+// byte slice. It does not validate that the signature is in the BLS group
+func SignatureFromBytesNoValidation(sig []byte) (common.Signature, error) {
+	signature, err := signatureFromBytesNoValidation(sig)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not create signature from byte slice")
+	}
+	return &Signature{s: signature}, nil
+}
+
+// SignatureFromBytes creates a BLS signature from a LittleEndian byte slice.
+func SignatureFromBytes(sig []byte) (common.Signature, error) {
+	signature, err := signatureFromBytesNoValidation(sig)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not create signature from byte slice")
+	}
 	// Group check signature. Do not check for infinity since an aggregated signature
 	// could be infinite.
 	if !signature.SigValidate(false) {

diff --git a/crypto/bls/blst/signature_test.go b/crypto/bls/blst/signature_test.go
@@ -207,6 +207,11 @@ func TestSignatureFromBytes(t *testing.T) {
 			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
 			err:   errors.New("could not unmarshal bytes into signature"),
 		},
+		{
+			input: []byte{0xac, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
+			name:  "Not in group",
+			err:   errors.New("signature not in group"),
+		},
 		{
 			name:  "Good",
 			input: []byte{0xab, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
@@ -227,6 +232,60 @@ func TestSignatureFromBytes(t *testing.T) {
 	}
 }
 
+func TestSignatureFromBytesNoValidation(t *testing.T) {
+	tests := []struct {
+		name  string
+		input []byte
+		err   error
+	}{
+		{
+			name: "Nil",
+			err:  errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Empty",
+			input: []byte{},
+			err:   errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Short",
+			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			err:   errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Long",
+			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			err:   errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Bad",
+			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			err:   errors.New("could not unmarshal bytes into signature"),
+		},
+		{
+			name:  "Not in group",
+			input: []byte{0xac, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
+		},
+		{
+			name:  "Good",
+			input: []byte{0xab, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			res, err := SignatureFromBytesNoValidation(test.input)
+			if test.err != nil {
+				assert.NotEqual(t, nil, err, "No error returned")
+				assert.ErrorContains(t, test.err.Error(), err, "Unexpected error returned")
+			} else {
+				assert.NoError(t, err)
+				assert.DeepEqual(t, 0, bytes.Compare(res.Marshal(), test.input))
+			}
+		})
+	}
+}
+
 func TestMultipleSignatureFromBytes(t *testing.T) {
 	tests := []struct {
 		name  string

diff --git a/crypto/bls/signature_batch_test.go b/crypto/bls/signature_batch_test.go
@@ -84,7 +84,7 @@ func TestVerifyVerbosely_VerificationThrowsError(t *testing.T) {
 	valid, err := set.VerifyVerbosely()
 	assert.Equal(t, false, valid, "SignatureSet is expected to be invalid")
 	assert.StringContains(t, "signature 'signature of bad0' is invalid", err.Error())
-	assert.StringContains(t, "error: could not unmarshal bytes into signature", err.Error())
+	assert.StringContains(t, "could not unmarshal bytes into signature", err.Error())
 	assert.StringNotContains(t, "signature 'signature of good0' is invalid", err.Error())
 }
 

diff --git a/proto/prysm/v1alpha1/attestation/aggregation/attestations/attestations.go b/proto/prysm/v1alpha1/attestation/aggregation/attestations/attestations.go
@@ -15,7 +15,7 @@ type attList []*ethpb.Attestation
 // significantly more expensive than the inner logic of AggregateAttestations so they must be
 // substituted for benchmarks which analyze AggregateAttestations.
 var aggregateSignatures = bls.AggregateSignatures
-var signatureFromBytes = bls.SignatureFromBytes
+var signatureFromBytes = bls.SignatureFromBytesNoValidation
 
 var _ = logrus.WithField("prefix", "aggregation.attestations")
 
@@ -36,6 +36,43 @@ func Aggregate(atts []*ethpb.Attestation) ([]*ethpb.Attestation, error) {
 	return MaxCoverAttestationAggregation(atts)
 }
 
+// AggregateDisjointOneBitAtts aggregates unaggregated attestations with the
+// exact same attestation data.
+func AggregateDisjointOneBitAtts(atts []*ethpb.Attestation) (*ethpb.Attestation, error) {
+	if len(atts) == 0 {
+		return nil, nil
+	}
+	if len(atts) == 1 {
+		return atts[0], nil
+	}
+	coverage, err := atts[0].AggregationBits.ToBitlist64()
+	if err != nil {
+		return nil, errors.Wrap(err, "could not get aggregation bits")
+	}
+	for _, att := range atts[1:] {
+		bits, err := att.AggregationBits.ToBitlist64()
+		if err != nil {
+			return nil, errors.Wrap(err, "could not get aggregation bits")
+		}
+		err = coverage.NoAllocOr(bits, coverage)
+		if err != nil {
+			return nil, errors.Wrap(err, "could not get aggregation bits")
+		}
+	}
+	keys := make([]int, len(atts))
+	for i := 0; i < len(atts); i++ {
+		keys[i] = i
+	}
+	idx, err := aggregateAttestations(atts, keys, coverage)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not aggregate attestations")
+	}
+	if idx != 0 {
+		return nil, errors.New("could not aggregate attestations, obtained non zero index")
+	}
+	return atts[0], nil
+}
+
 // AggregatePair aggregates pair of attestations a1 and a2 together.
 func AggregatePair(a1, a2 *ethpb.Attestation) (*ethpb.Attestation, error) {
 	o, err := a1.AggregationBits.Overlaps(a2.AggregationBits)