From be23773924ba3d48f3feb106f4924e6f0c5cb5c8 Mon Sep 17 00:00:00 2001 From: Potuz Date: Tue, 16 May 2023 14:06:26 -0300 Subject: [PATCH] Don't use max cover on unaggregated atts nor check subgroup of validated 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 --- .../operations/attestations/kv/aggregated.go | 31 +++++----- beacon-chain/rpc/eth/beacon/pool_test.go | 2 +- crypto/bls/bls.go | 7 +++ crypto/bls/blst/signature.go | 24 +++++++- crypto/bls/blst/signature_test.go | 59 +++++++++++++++++++ crypto/bls/signature_batch_test.go | 2 +- .../aggregation/attestations/attestations.go | 39 +++++++++++- 7 files changed, 142 insertions(+), 22 deletions(-) diff --git a/beacon-chain/operations/attestations/kv/aggregated.go b/beacon-chain/operations/attestations/kv/aggregated.go index 94f7ce3e5c0d..790f4f50ec72 100644 --- 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 index 2b080e8dbf68..612ad18c15a7 100644 --- 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 index 6e8df3998796..4528c5143898 100644 --- 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 index 6e007e12ba2c..787eeba5c583 100644 --- a/crypto/bls/blst/signature.go +++ b/crypto/bls/blst/signature.go @@ -24,8 +24,9 @@ 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) } @@ -33,6 +34,25 @@ func SignatureFromBytes(sig []byte) (common.Signature, error) { 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 index 7bdfe05570e2..6437bc44123a 100644 --- 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 index 0429d1f9caaf..48a8530d9f8b 100644 --- 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 index 4059016d2aef..be3aadb9cd90 100644 --- 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)